Roslyn: Source control switching with large files results in lots of file change events which results in lots of file re-reading

[sharwell]

Version Used: Visual Studio 2019 Preview 1.1.

🔗 One heap demonstrating this issue can be found in https://devdiv.visualstudio.com/DevDiv/_workitems/edit/758630.

Steps to Reproduce:

Unknown, but likely one of the causes is source control branch switching.

A large number of RemoveDocumentAction instances are held in memory, each of which points to a DocumentState. The state causes tremendous memory overhead leading to slow processing of actions and eventual OutOfMemoryException crashes.

Expected Behavior:

RemoveDocumentAction does not contain a reference to a parsed syntax tree.

Actual Behavior:

RemoveDocumentAction does contain a reference to a parsed syntax tree.

[samrueby]

You commented on my feedback saying my crash dump was related to this issue. I don't think I can effectively give you steps to reproduce, but I am able to consistently reproduce this in my environment.

[sharwell]

@samrueby Yes, your crash dump was a leading source of information in creating this action item. 👍 I have also observed a similar situation myself (involving the same types), which I believe occurred after switching source control branches.

[samrueby]

@sharwell Awesome. Let me know if I can help.

[jasonmalinowski]

And @samrueby we already have a fix in mind for this one -- your report was what made us realize this needed fixing. So thanks!

[samrueby]

@jasonmalinowski thank you!

[jasonmalinowski]

@sharwell I was planning on tackling this in 16.1, any concerns? I don't think anything regressed here and you'd probably hit this in 15.9 as well with the right patterns or projects large enough.

[jasonmalinowski]

(it's not that I don't think it's important, we just have "oops, this is broken badly" bugs to fix first)

[samrueby]

I've been hitting this since 2017. So while I'd love to have it fixed I agree I don't think it was a recent regression.

[sharwell]

Sounds perfectly reasonable 👍

[samrueby]

Hey all- any chance of having this in the next preview? ☺

[ivanbasov]

@sharwell , @jasonmalinowski , @jinujoseph this one is currently scheduled for preview 2. Are we still going to fix it here? Should we re-schedule it?

[samrueby]

[I would be so grateful- I literally hit this tens of times a day.]

[jasonmalinowski]

@samrueby When you're switching branches and you hit this, how many files do you think are "different" between those two branches? The dump you shared awhile back looked like it was on the scale of hundreds, but it's possible we hadn't processed all of them so it might have been more still coming.

[jasonmalinowski]

@samrueby So digging through the dump file you uploaded, it looks like you have a few very large generated files, is that correct? Do those also have some assembly level attributes in them?

We have a threshold that once a file is "big enough" (greater than 4 KB) we do some optimizations which avoids us holding the entire contents of the file in memory. There's one exception to that rule: if the file has assembly-level attributes we hold onto it which was a special case intended for AssemblyInfo.cs-like files, since the underling IntelliSense engine has to repeatedly reference that file. It looks like that special case is tripping us up on your big files so we're thinking we have to hold those entire files in memory, along with their parse trees.

There also appears to be another problem where we have more copies of the parse tree for those files so I'm still investigating that, but it's possible that changing your code structure a bit might work around the issue and least unblock you.

[jasonmalinowski]

For anybody following along at home on the technical side, this is my best guess of what's happening:

1. We have a file watcher that watches for mutated files that are closed; this is what allows something like a branch switch (even if not by Visual Studio) to be reflected in intellisense.
2. If the file is a large file beyond a few megabytes, the writes result in multiple file change notifications being triggered through the file watcher. I'm guessing this is because we say "size" is a valid trigger for the file watch, not just last modified time, so if the file is being written in chunks we see intermediate sizes.
3. Each file change results in us immediately producing a new Solution snapshot. These don't actually read the file.
4. We process the swarm of WorkspaceChanged events quickly, but requesting a Compilation after this results in us trying to remove the old trees from the old DocumentStates and adding new trees from the new DocumentStates. The swarm of change events means we have a huge list of trees to remove/add.
5. Because the text is actually loaded asynchronously, each removal and add to produce the final Compilation is actually making multiple parsed copies of the syntax tree, which since the file is large is also large.
6. In @samrueby's case, the trees that are large also have a assembly-level attribute, so we don't hold any of the trees weakly. As a result, everything is being held in memory until we ultimately OOM.

I see a few fixes here:

1. Throttle the file change events, so we wait a bit and then process the file change once rather than each time the underlying event is raised. This would be the biggest help.
2. In the CompilationTracker, if we're queueing two TouchDocumentActions back to back, and they're for the same underlying document, we should just merge them so we're only holding onto the original document state and the final document state.
3. Investigate if we still need the special case around recoverable/weak trees; if the compiler can change the access pattern, or we change our heuristic, it might help here as well.

Throttling file change events would be by far the biggest improvement, but 2 and 3 would also simplify the code and address some other potentially problematic patterns that we've seen.

[samrueby]

Hey @jasonmalinowski! Sorry- I was Ignite all last week and have been trying to catch up this week.

Yes! We do have a few large generated files. Yes they have assembly-level attributes: AssemblyTitle, AssemblyProduct, ComVisible, AssemblyVersion, etc. One file I'm looking at that triggers this problem at lot is 11,012 KB.

[jasonmalinowski]

So as a workaround, if you split it into two files: the one that has the assembly attributes and the rest of the huge one, that may workaround the issue or at least reduce it's prevalence. I won't call this bug "fixed" if that works, but it may unblock you.

[samrueby]

Unfortunately these particular projects we don't have control over the generated output 😢

Thank you!

[samrueby]

Not sure if you'd be interested but I have another crash dump w/ the latest VS for this problem: 16.4.0 Prev 6.

[samrueby]

This probably is in line with what you've already discovered but I noticed while watching task manager right before this happens that the number of handles held by devenv.exe goes through the roof. Something like ~6k => 30k.

[jasonmalinowski]

(switching the title to make it clear that my PR #41217 probably won't hurt the scenario @samrueby is hitting but it's not the core fix as was originally implied)