More error list performance improvements for build scenario by mavasani · Pull Request #48804 · dotnet/roslyn

mavasani · 2020-10-21T01:07:54Z

With this PR, error list refreshes almost instantaneously after build finishes on all manual test scenarios I have tried.

Recommend reviewing with whitespace diffs turned off

This change removes one of the primary performance bottleneck in the build/live diagnostics de-dupe code path when user triggers a build: reading and writing diagnostics into persistent storage as part of de-duping.

Current approach for build/live de-dupe: Once we have identified the build diagnostics that are supported in live analysis, we do the following:
1. Load all existing diagnostics for each project being built (and all its documents) from the persistent storage.
2. Merge the build diagnostics that are supported in live with the hidden severity existing diagnostics.
3. Save the merged diagnostics into the persistent storage.
4. Raise diagnostics updated events to update the diagnostics in error list and other diagnostic clients.
New approach: Steps 1 to 3 above are the expensive ones. With this PR, we avoid these steps and instead do the following:
1. Do not load existing diagnostics or attempt to merge existing hidden diagnostics with build diagnostics - computing hidden diagnostics for open file is pretty cheap, and is in fact already done via a separate re-analyze call for active document at end of the build.
2. Store the build diagnostics that are supported in live in the InMemory cache, that takes precendence over persistent storage cache. This is much faster, almost instantaneous, and all the diagnostic clients will hit the InMemory cache and see no effective behavior change.
3. Schedule a separate task on a post-build task queue to write the diagnostics in step 2. from the InMemory cache to persistent storage, also dropping them from in memory cache to reduce memory pressure. This change postpones this I/O step to be done after error list refresh, hence making the error list refresh almost instantaneous.
4. Raise diagnostics updated events to update the diagnostics in error list and other diagnostic clients.

This change removes one of the primary performance bottleneck in the build/live diagnostics de-dupe code path when user triggers a build. **With this PR, error list refreshes almost instantaneously after build finishes on all manual test scenarios.** - Current approach for build/live de-dupe: Once we have identified the build diagnostics that are supported in live analysis, we do the following: 1. Load all existing diagnostics for each project being built (and all its documents) from the persistent storage. 2. Merge the build diagnostics that are supported in live with the hidden severity existing diagnostics. 3. Save the merged diagnostics into the persistent storage. 4. Raise diagnostics updated events to update the diagnostics in error list and other diagnostic clients. - New approach: Steps 1 to 3 above are the expensive ones. With this PR, we avoid these steps and instead do the following: 1. Do not load existing diagnostics or attempt to merge existing hidden diagnostics with build diagnostics - computing hidden diagnostics for open file is pretty cheap, and is in fact already done via a separate re-analyze call for active document at end of the build. 2. Store the build diagnostics that are supported in live in the InMemory cache, that takes precendence over persistent storage cache. This is also much faster, almost instantaneous, and all the diagnostic clients will hit the InMemory cache and see no effective behavior change. 3. Schedule a separate task on a post-build task queue to write the diagnostics in step 2. from the InMemory cache to persistent storage, also dropping them from in memory cache to reduce memory pressure. This change postpones this I/O step to be done after error list refresh, hence making the error list refresh almost instantaneous. 4. Raise diagnostics updated events to update the diagnostics in error list and other diagnostic clients.

mavasani · 2020-10-21T23:23:41Z

@dotnet/roslyn-ide Ping for reviews - this leads to a substantial performance improvement for error list in build scenarios, and the changes are not too complex.

CyrusNajmabadi · 2020-10-21T23:27:18Z

I can get to this tomorrow

...res/Core/Portable/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer_BuildSynchronization.cs

src/VisualStudio/Core/Def/Implementation/TaskList/ExternalErrorDiagnosticUpdateSource.cs

...res/Core/Portable/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer_BuildSynchronization.cs

src/VisualStudio/Core/Def/Implementation/TaskList/ExternalErrorDiagnosticUpdateSource.cs

CyrusNajmabadi

my basic feedback is that i don't understand this stuff :) reigning in complexity would likely go a long way. in particular, having specific components that are responsible for exactly one thing (maybe scheduling), and another component for something else (i.e. which pieces of work to do, or how to merge them) could be really helpful.

mavasani · 2020-10-22T21:12:04Z

Thanks @CyrusNajmabadi - I have added more comments. I agree that there are too many moving pieces and task queues here. Let me try and clean this up in a follow-up PR.

ghost · 2020-10-22T21:20:32Z

Hello @mavasani!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

ghost

Auto-approval

mavasani added the Area-IDE label Oct 21, 2020

mavasani added this to the 16.9.P2 milestone Oct 21, 2020

mavasani marked this pull request as ready for review October 21, 2020 02:58

mavasani requested a review from a team as a code owner October 21, 2020 02:58

mavasani requested review from CyrusNajmabadi and sharwell October 21, 2020 02:58