Type subsumption cache, fix keys to reduce memory use #18875
+167
−165
Conversation
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
|
T-Gro
added
the
NO_RELEASE_NOTES
T-Gro
reviewed
Sep 2, 2025
T-Gro
reviewed
Sep 2, 2025
T-Gro
reviewed
Sep 2, 2025
T-Gro
reviewed
Sep 2, 2025
| /// Usage: | ||
| /// let getValueFor = WeakMap.getOrCreate (fun key -> expensiveInit key) | ||
| /// let v = getValueFor someKey | ||
| let getOrCreate valueFactory = |
There was a problem hiding this comment.
Why is it called getOrCreate ? Doesn't it always create a new table?
There was a problem hiding this comment.
I hope not. It calls ConditionalWeakTable.GetValue factory overload, so it should get the existing attached property or create a new one.
|
Just to have this data somewhere: as of current commit hit ratio measured over ComponentTests net10 is 75.8%, no eviction fails. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The type subsumption cache was causing problems with the legacy 32 bit test projects.
This was swept under a rug for a few months, using a cache capacity override in tests.
In the course of #18872 I had to revisit this because it showed up again.
I captured a memory snapshot during tests to see what's going on.
Turns out the
TTypeobjects used as cache keys were the problem.They can reference a lot of additional lazy initialized data, including ginormous circular references. Now when we put them as cache keys, that can extend their life well beyond their natural lifespan. This was not a big problem memory wise in normal use, but somehow 32 bit tests couldn't handle it and had hard time GCing this.
For example If I weakly attached the cache to
TcGlobals. This was correctly GC'd in other test projects but never collected in VS tests.Anyway, the fix is to use a synthetic key, created as a slim structural representation of
TTypein such a way as to guarantee equality soundness.I checked (by capturing telemetry from the test run) that the cache still works, hit ratio is still good and evictions happen ok.