Reloading a project doesn't reuse IWorkspaceProjectContext

[jasonmalinowski]

This a tracking bug since this issue has often came up in traces/bugs and we don't really have a single place to track it.

Currently, if the project system reloads a project or a configuration changes (i.e. debug to release), the project system disposes the IWorkspaceProjectContext and creates a new one from scratch. This is not great for a few reasons:

1. This really hurts performance in some cases. A debug to release switch recreating projects means we will end up getting rid of all our parsed syntax trees and re-parsing them from scratch. If we simply were given an update to the options for the existing project, it would mean we would be able to only reparse impacted trees which would be a significant improvement.
2. It sometimes causes other things to break. For example, https://developercommunity2.visualstudio.com/t/VS-1630-NET-Core-30-Blazor-Server-Ap/746166 happened because once the context switch happens, the project connected to the contained language was removed.

Fixing this would (to my understanding) require some rework of the project system and how it abstracts accesses, and might require Roslyn to add a new API or two, but this would address some longstanding issues.

Things Roslyn would need to do to enable this:

• [ ] Allow the hierarchy of an IWorkspaceProjectContext to be updated.
• [ ] For the context switch case, don't reparse trees (but update the ParseOptions) for trees that don't contain directives. This may require a compiler API.

[davkean]

Reloading a project will never reuse, and would require a significant change and rewrite on how this works. This would be better fit into Dev18 where the communication layer will likely change.

For config-change, this will require an IActiveConfiguredProjectSubscriptionService-like implementation that works across all implicitly active configurations (not just the main active one) and makes it easier to handle the transition so it just looks like a normal change down the pipe (think files that are included in debug but not release, should appear as a delete when switching from debug -> release).

[drewnoakes]

Reloading a project will never reuse, and would require a significant change and rewrite on how this works

The project configurations could register workspaces with a global scope component that tracks such objects across reloads, and prevents leaks. The pattern could be pulled out and reused to prevent other kinds of external-resource recycling during reloads.

[davkean]

These are currently tied to a IVsHierarchy instance that cannot be changed and Roslyn will need to be updated to handle this transient state. A project also doesn't have context it's in a reload state, so that would also need to be plumbed through.

It's a seperate issue and probably better to track it such.

[jasonmalinowski]

These are currently tied to a IVsHierarchy instance that cannot be changed and Roslyn will need to be updated to handle this transient state.

That wouldn't be difficult to do.

[jasonmalinowski]

And added a checklist in the original item to track what Roslyn would need to update.

[davkean]

I would separate the two states; configuration-switch and project-reload. We don't need a bunch of plumbing for the former, we do for the latter. If we block on this plumbing, it would delay the former.

[jasonmalinowski]

So looking at some performance numbers here: if I have Roslyn open with some files open and let things stabilize, and then switch from Debug -> Release, we spend about 20 seconds parsing C# files again and 10 seconds parsing VB again in the same process (and presumably the same amount again out of process.) Now not all of those we can avoid a reparse of. Of the 27,070 trees we'd potentially reparse, 11,584 of them have some directive, but that'll include things that are #nullable; if we filter to just things that are #if and not something else, we're down to 1690 trees in the 27,070 having some #if directive. So back-of-the-napkin math implies we can avoid 94% of our parsing time, or 28ish seconds of CPU. This also only measured the in-proc numbers, I'd expect out of proc to mean the total processor time is 2x that.

[CyrusNajmabadi]

@jasonmalinowski would be good to know what percentage of those dcs have a pp-directive that actually affects tree-parsing. so things like #region and and #pragma wouldn't count.

[jasonmalinowski]

So @CyrusNajmabadi's question made me find the code I had ran and hadn't deleted yet. I discovered two things:

1. Past me was smart enough that "we're down to ... trees in the 27,070 having some #if directive" did indeed look at C# files containing just #if.
2. Past me was an idiot in that the VB code path didn't do .ContainsDirectives, but just counted all VB trees. Like, all of them.

The corrected number is trees with #if specifically is 1690. I've edited the prior comment too.

[CyrusNajmabadi]

Being able to share information in these scenarios can be significant. For example, in Roslyn itself, adding this optimization for our syntactic indices (used by NavTo, FindRefs, etc.) saves 60s of CPU time recomputing indices on config switch: https://github.com/dotnet/roslyn/pull/60693

I expect that such an optimization will have even more impact if we reuse documents/trees as we will save a ton of time reparsing as well as producing a ton less garbage.

[drewnoakes]

The work here is mostly done, but given the scale of this change and the proximity to 17.3 release we will hold this back for 17.4 to give it more time for testing internally.

There is also a CPS change for 17.4 that will allow us to more easily obtain ordered command line arguments and further simplify the implementation.

[drewnoakes]

https://github.com/dotnet/project-system/issues/7541 implements the configuration switch optimisation.

[drewnoakes]

With the configuration switch work done, I'm wondering whether it's realistic that we'd ever address the other half of this issue: the reuse of IWorkspaceProjectContext across reloads.

I doubt this is something we would ever do, and so I will close this out. If we have a clear signal that there'd be significant customer value to supporting this across reloads, we can revisit. Note that reloads usually happen for a single project at a time, while config switch is solution wide. This means the impact of preserving the context across reloads is more limited.