Delete the lazy caching of GetLatestProjectVersion()#41412
Merged
jasonmalinowski merged 1 commit intodotnet:release/dev16.5from Feb 6, 2020
Merged
Conversation
Solution.GetLatestProjectVersion() returns the highest project version of any of the Projects in the Solution. This is lazily computed, but the lazy computation has a fatal flaw. It's implemented with a Lazy<VersionStamp>, and if a Solution is changed and no project was changed (say, you only modified a document's text), the existing Lazy<VersionStamp> is reused. This causes us to root an older SolutionState or ProjectState until somebody were to call GetLatestProjectVersion() and cause the Lazy<VersionStamp> to be evaluated. Except...that would never happen. It turns out that nothing calls GetLatestProjectVersion() in Visual Studio anywhere that I can find. The only call is in one unit test that calls it and then ignores the result. Thus, we'd simply leak the SolutionState/ProjectState until some code ran that knew it could explicitly state the value instead of reuse the existing Lazy<VersionStamp>. Looking through history, it appears that this used to be used more regularly as a part of an older implementation of the ProjectDependencyGraph, but that was removed in abd7654. Since nobody calls this, and the actual computation (just looping through all projects to find the newest stamp) is fairly trivial, I'm just going to delete all the lazy handling and just inline the computation. In the unlikely case that this ever becomes a hot path, we can revisit the laziness at that time.
47f4089 to
252064f
Compare
CyrusNajmabadi
approved these changes
Feb 5, 2020
ryzngard
approved these changes
Feb 5, 2020
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
6 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.
Solution.GetLatestProjectVersion() returns the highest project version of any of the Projects in the Solution. This is lazily computed, but the lazy computation has a fatal flaw. It's implemented with a Lazy, and if a Solution is changed and no project was changed (say, you only modified a document's text), the existing Lazy is reused. This causes us to root an older SolutionState or ProjectState until somebody were to call GetLatestProjectVersion() and cause the Lazy to be evaluated.
Except...that would never happen. It turns out that nothing calls GetLatestProjectVersion() in Visual Studio anywhere that I can find. The only call is in one unit test that calls it and then ignores the result. Thus, we'd simply leak the SolutionState/ProjectState until some code ran that knew it could explicitly state the value instead of reuse the existing Lazy.
Looking through history, it appears that this used to be used more regularly as a part of an older implementation of the ProjectDependencyGraph, but that was removed in abd7654.
Since nobody calls this, and the actual computation (just looping through all projects to find the newest stamp) is fairly trivial, I'm just going to delete all the lazy handling and just inline the computation. In the unlikely case that this ever becomes a hot path, we can revisit the laziness at that time.