Performance: cache ClassDef._find_metaclass() per node - #3166
Performance: cache ClassDef._find_metaclass() per node#3166Pierre-Sassoulas wants to merge 2 commits into
Conversation
The recursive metaclass lookup walked the MRO on every call. Cache the result per node so the walk runs once, short-circuiting re-entry through a ``_COMPUTING_METACLASS`` sentinel to preserve cycle protection. Only the no-context call is cached: the cache key is ``context is None``, so an explicit ``context`` still forces a fresh walk. ``TestFindMetaclassCaching`` covers the four branches: a cached ``ClassDef`` result is reused, a ``None`` result is cached explicitly so the MRO walk does not repeat, an explicit ``context`` neither reads nor writes the cache, and re-entry while the sentinel is parked returns ``None`` to break the cycle. Refs #1115
Merging this PR will not alter performance
Comparing 
Footnotes
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3166 +/- ##
=======================================
Coverage 93.67% 93.67%
=======================================
Files 93 93
Lines 11645 11657 +12
=======================================
+ Hits 10908 10920 +12
Misses 737 737
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
No visible result in the CI benchmark but total function calls for declared_metaclass() and ancestors() goes down as expected. |
DanielNoord
left a comment
DanielNoord
left a comment
There was a problem hiding this comment.
Is the additional complexity worth it if we don't see a meaningful impact on benchmarks? I understand we're trying to improve performance but this will hurt maintainability in the long run and without clear indications of benchmark improvements that doesn't necessarily weigh up.
The inference code base is already quite hard to maintain...
|
The benchmark is pretty rough, it's not often we're going to see 5+% shaved or added from a normal non problematic run of 20s on flake8. (We did or fixed something pretty terrible when that happen). We still need to think about performance implication and look at the cprofile to optimize problematic code with big codebase and deep recursion. Inference is where the performances gains are all going to happen now imo. |
Agreed, but in this case I am wondering if the added complexity really weighs up against the maintainability costs. Is this recursion really an issue? |
2 participants
Type of Changes
Description
Split out of #3048, as requested — one optimization, one commit, tests included.
The recursive metaclass lookup walked the MRO on every call. Cache the result per node so the walk runs once, short-circuiting re-entry through a
_COMPUTING_METACLASSsentinel to preserve the cycle protection thatseenprovides in the slow path.Only the no-context call is cached: the cache key is
context is None, so an explicitcontextstill forces a fresh walk.TestFindMetaclassCachingcovers the four branches: a cachedClassDefresult is reused, aNoneresult is cached explicitly so the MRO walk does not repeat, an explicitcontextneither reads nor writes the cache, and re-entry while the sentinel is parked returnsNoneto break the cycle.Refs #1115