Opened 9 months ago

Closed 9 months ago

#35230 closed Cleanup/optimization (fixed)

Cache ForeignObjectRel.get_accessor_name()

Reported by: Adam Johnson Owned by: Adam Johnson
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

This method computes the name for the accessor descriptor, used in many places. Its results are stable, unless the optional model parameter is provided, used only in forms.

Adding a cached property accessor_name for the default value can speed up many code paths. In particular, I profiled the system checks for a project and found it was taking ~2% of the runtime, which can be basically eliminated with caching. Results:

Before: 6625 calls taking 2ms
After: 9 calls taking ~0ms

Change History (4)

comment:1 by Simon Charette, 9 months ago

Triage Stage: UnreviewedAccepted

This is worth doing but if anyone runs into issues with this change future when running data migrations the root cause is likely tickets such as #27737 until #29898 is completed. It is well known that operating from rendered models can cause all of sorts of issues with regard to caching of relationship attributes, it should be a reason to avoid using rendered models though and not to avoid caching relationship attributes.

It will be interesting to see if this has a noticeable effect on the benchmarks.

comment:2 by Adam Johnson, 9 months ago

Thanks for the note Simon.

Yes, it will be interesting to see the benchmark results. For a quick check, I ran a bunch of model tests with ./runtests.py --parallel 1 "*models* under hyperfine and got the following results.

Before:

`
Time (mean ± σ): 2.745 s ± 0.031 s [User: 2.628 s, System: 0.100 s]
Range (min … max): 2.697 s … 2.796 s 10 runs
`

After:

`
Time (mean ± σ): 2.727 s ± 0.021 s [User: 2.611 s, System: 0.099 s]
Range (min … max): 2.699 s … 2.764 s 10 runs
`

The mean is lower but a statistical T test calculates this as not statistically significant.

Version 0, edited 9 months ago by Adam Johnson (next)

comment:3 by Mariusz Felisiak, 9 months ago

Owner: changed from nobody to Adam Johnson
Triage Stage: AcceptedReady for checkin

comment:4 by GitHub <noreply@…>, 9 months ago

Resolution: fixed
Status: assignedclosed

In 5e80390a:

Fixed #35230 -- Added cached ForeignObjectRel.accessor_name.

Note: See TracTickets for help on using tickets.
Back to Top