Keep a single memoization layer for located reflections - #5974
Merged
Conversation
Located reflections were cached twice - by MemoizingReflector (keyed by name) and again by MemoizingSourceLocator (keyed by identifier), pinning every reflection object graph in memory twice per worker process. The locator-level cache could not simply be dropped: created reflections capture the reflector passed to SourceLocator::locateIdentifier() and resolve parent classes, interfaces and prototypes through it, bypassing MemoizingReflector entirely - the memoizing locator was the only cache absorbing those internal lookups. MemoizingReflector now locates identifiers itself instead of wrapping DefaultReflector, so created reflections capture the memoizing reflector and route internal lookups through the same name-keyed cache. The MemoizingSourceLocator wrappers are removed along with the cache.memoizingSourceLocatorEntriesCountMax parameter and the originalBetterReflectionReflector service. The stub validator container relied on the removed locator wrapper as its only memoization, so it now overrides betterReflectionSourceLocator with the stub source locator instead of swapping reflectors. In self-analysis this reduces total worker peak memory from ~2.5 GB to ~2.1 GB with unchanged elapsed time. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017DjgMHgfvwhD4ogdTaRtLp
It was only needed so that the stub validator container could point NodeScopeResolver to a different reflector. Since the stub validator now overrides the source locator instead, the service is just an alias of betterReflectionReflector, which is what a plain Reflector type-hint autowires to anyway. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017DjgMHgfvwhD4ogdTaRtLp
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Located reflections were cached twice — by
MemoizingReflector(keyed by name) and again by theMemoizingSourceLocatorwrapper (keyed by identifier), pinning every reflection object graph in memory twice per worker process.The locator-level cache could not simply be dropped: created reflections capture the reflector passed to
SourceLocator::locateIdentifier()and resolve parent classes, interfaces and prototypes through it, bypassingMemoizingReflectorentirely — the memoizing locator was the only cache absorbing those internal lookups (removing it naively OOMs workers because every parent/interface lookup re-locates from scratch).Instead of wrapping
DefaultReflector,MemoizingReflectornow locates identifiers itself and passes$thisto the source locator, so created reflections capture the memoizing reflector and route their internal lookups through the same name-keyed cache. This makes theMemoizingSourceLocatorwrappers redundant for real:BetterReflectionSourceLocatorFactory,StubSourceLocatorFactoryandTestCaseSourceLocatorFactory, together with thecache.memoizingSourceLocatorEntriesCountMaxparameter (measurements showed capping that layer was counterproductive anyway),originalBetterReflectionReflectorservice is gone,betterReflectionSourceLocator!with the stub source locator instead of swapping reflectors —stubReflectoris gone too.Results (self-analysis, warm caches, 8 workers; "Used memory" = sum of per-worker peaks):
Full test suite passes. No BetterReflection change needed — works with the released 6.70.0.3.
🤖 Generated with Claude Code
https://claude.ai/code/session_017DjgMHgfvwhD4ogdTaRtLp