Don't hold onto some SourceTexts if multiple changes happen#53579
Conversation
src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.CompilationTracker.cs
Outdated
Show resolved
Hide resolved
.../Portable/Workspace/Solution/SolutionState.CompilationAndGeneratorDriverTranslationAction.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
💭 Is it possible to end up in a state where two TouchDocumentAction instances are separated by something else? In such a case, is it sufficient to simply ignore all but the last TouchDocumentAction?
There was a problem hiding this comment.
@sharwell: good question! Yep, this could be made smarter. A case where you have a Solution snapshot, and keep forking it with alternate edits to A.cs and B.cs won't consolidate those together with this, even though the earlier replacements don't really matter. There's some icky edge cases though more generally with how the code works. Consider a case where the user does the following changes, forking a Solution each time.
- Replaces the contents of A.cs.
- Removes A.cs.
- Adds A.cs back with the same document ID.
- Replaces the contents of A.cs again.
(Is this sense of operations sensical? No. Which probably means there's code out there that does it. 😄 )
Clearly the change in step 1 doesn't matter, however when we run the translation action for the second step, it's implemented as "remove the syntax tree that should be there for A.cs", but if step 1 doesn't run, it'd be the wrong tree. A smarter algorithm would probably be "we can merge any two TouchDocumentActions as long as we don't move them across any other action", but I'd want to see a case where that mattered before we wrote the fancier code and tests.
|
@jinujoseph for 16.11 approval; this is fixing a memory usage issue which a customer is hitting for their extension. Change is small enough and is pretty targeted so not to worrisome on the risk side of things. |
If we are processing edits through TryApplyChanges for closed files, we'll make a series of changes in rapid succession: we'll have one change that may open the file in an invisible editor, then we'll edit the file, and then close again. The Solution object would then be holding onto the old Compilation with several changes queued, which would root the intermediate states that still pointed the editor contents and their change lists which can be large. Once a Compilation was requested for this new file, we'd process all the changes and free the memory, but if the file is closed we might not get to that right away.
e3dc96d to
5cc6571
Compare
If we are processing edits through TryApplyChanges for closed files, we'll make a series of changes in rapid succession: we'll have one change that may open the file in an invisible editor, then we'll edit the file, and then close again. The Solution object would then be holding onto the old Compilation with several changes queued, which would root the intermediate states that still pointed the editor contents and their change lists which can be large. Once a Compilation was requested for this new file, we'd process all the changes and free the memory, but if the file is closed we might not get to that right away.