Change logic for how we cache GeneratorDrivers to avoid initialization #81323
+66
−75
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
This makes a few general changes to how we cache drivers so we don't re-initialize them multiple times when we're first loading a solution. 1. Rather than the cache being a ProjectId -> cached driver map, we instead just have the object holding the cached driver held directly by the ProjectState. 2. Rather than clearing out the cached driver once we think we won't need it, we just continually refresh the cached driver with the latest version. This simplifies the logic and also ensures we won't ever clear something out that we would need later, but still keeps us from holding onto old state. 3. Rather than explicitly clearing out the initialized driver when we have to rerun generators, we keep it around.
ToddGrun
reviewed
Nov 19, 2025
| // 1. If some other fork of this Solution needs a GeneratorDriver created, it'll have one that's probably more update to date. | ||
| // This is obviously speculative -- if it's a really old Solution fork it might not help, but can't hurt for the more common cases. | ||
| // 2. It ensures that we're not holding an old GeneratorDriver alive, which itself may hold onto state that's no longer applicable. | ||
| this.ProjectState.GeneratorDriverCache.UpdateCacheWithForGeneratorDriver(generatorDriver); |
CyrusNajmabadi
approved these changes
Nov 19, 2025
| public void UpdateCacheWithForGeneratorDriver(GeneratorDriver driver) | ||
| { | ||
| ImmutableInterlocked.TryRemove(ref _driverCache, projectId, out _); | ||
| _driverCache = AsyncLazy.Create(driver); |
Member
There was a problem hiding this comment.
Cute. Definitely doc this field to explain this will happen. Ah. I see you did doc this well
| // Clear out the cache of any previously initialized GeneratorDriver. Otherwise we might reuse a | ||
| // driver which will not count as a new "run" in some of our unit tests. We have tests that very explicitly count | ||
| // and assert the number of invocations of a generator. | ||
| GeneratorDriverCache.EmptyCacheForProject(projectId); |
Member
|
Thanks for this. It's a lot cleaner/clearer! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
This makes a few general changes to how we cache drivers so we don't re-initialize them multiple times when we're first loading a solution.