Remove strong reference to AnalyzerOptions in the analyzer manager ex… by mavasani · Pull Request #20096 · dotnet/roslyn

mavasani · 2017-06-08T00:07:43Z

…ecution context

Fixes #20065

Customer scenario

Performance: We are leaking solution instances in preview workspace (which is created every time we invoke a lightbulb preview). See #20065 (comment) for details.

Bugs this fixes:

Fixes #20065

Workarounds, if any

N/A

Risk

Low, as we are not changing any functionality here. Instead of having a Dictionary<AnalyzerOptions, ConditionalWeakTable<Compilation, Task<HostCompilationStartAnalysisScope>>> with strong reference to AnalyzerOptions, we now use a ConditionalWeakTable<Compilation, Dictionary<AnalyzerOptions, Task<HostCompilationStartAnalysisScope>>>, which has a weak reference and will let go AnalyzerOptions instance once the compilation is GC'ed.

Performance impact

This should improve performance as we are preventing a leak of solutions in preview workspace.

Is this a regression from a previous update?

Yes, we recently refactored the WorkspaceAnalyzerOptions to strongly hold onto the solution snapshot. This was needed to fix an IDE race. This in turn is causing the map in question here to leak these solution instances.

Root cause analysis:

We don't have tests to catch and prevent leaks. We are going to further improve the design in this area when we tackle #2830 in 15.6, so we prevent such regressions in future.

How was the bug found?

Dogfooding.

…ecution context Fixes dotnet#20065

mavasani · 2017-06-08T16:36:35Z

Test failure in https://ci.dot.net/job/dotnet_roslyn/job/master/job/windows_debug_vs-integration_prtest/5272/ with output: xunit produced no error output but had exit code 1

mavasani · 2017-06-08T16:36:53Z

@dotnet-bot retest windows_debug_vs-integration_prtest please

mavasani · 2017-06-08T16:37:51Z

Tagging @dotnet/roslyn-compiler @dotnet/analyzer-ioperation for review.

mavasani · 2017-06-08T16:39:22Z

@heejaechang Can you please buddy test this change and double check analyzer execution in OOP with FSA turned on? I can share the locally built VSIX if you need.

heejaechang · 2017-06-08T16:55:33Z

👍 sure, I will do the buddy testing.

jasonmalinowski · 2017-06-08T17:33:49Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerManager.AnalyzerExecutionContext.cs

@@ -20,7 +20,7 @@ private sealed class AnalyzerExecutionContext

            // This map stores the tasks to compute HostCompilationStartAnalysisScope for per-compilation analyzer actions, i.e. AnalyzerActions registered by analyzer's CompilationStartActions.
            // Compilation start actions will get executed once per-each AnalyzerAndOptions as user might want to return different set of custom actions for each compilation/analyzer options.


Not sure if the comment needs an update here.

I am going to delete a bunch of code around this in dev15.6 branch soon, we are going to get rid of the singleton.

jasonmalinowski · 2017-06-08T17:36:04Z

Yes, we recently refactored the WorkspaceAnalyzerOptions to strongly hold onto the solution snapshot. This was needed to fix an IDE race.

I'm still not convinced this is what regressed it. Previously we were holding onto the Workspace, which would have still rooted the Solution anyways.

mavasani · 2017-06-08T17:43:11Z

I'm still not convinced this is what regressed it. Previously we were holding onto the Workspace, which would have still rooted the Solution anyways.

Probably something else regressed it before that change, but nevertheless, we shouldn't be leaking anymore after this change.

mavasani · 2017-06-08T19:53:03Z

Tagging @MattGertz for approval

heejaechang · 2017-06-08T20:43:14Z

confirmed that there is no PreviewWorkspace leaked with this change.

mavasani · 2017-06-08T20:44:00Z

@heejaechang - Thank you very much!

gafter

The code change looks good to me.

I would expect this to be testable using a weak reference in the test - does a proper use of these APIs keep the solution alive? Is such a test feasible? Or would it naturally be a flaky test due to nondeterministic behavior of GC?

gafter · 2017-06-08T21:01:21Z

(I'm not signing off yet because I'd like to see if this is testable, and we need another sign-off from the compiler team)

jasonmalinowski · 2017-06-08T22:54:06Z

We do have https://github.com/dotnet/roslyn/blob/6a98a959cbd296991411e3e9d124aeeb4827ae1f/src/Test/Utilities/Portable/ObjectReference.cs as a tool you can use for asserting that objects are no longer held. It holds our current "GC until it goes away" pattern.

heejaechang · 2017-06-08T23:04:43Z

@jasonmalinowski I see you meant this kind of lifetime check test. to explicitly see whether something left alive after certain operation. ya. I guess we could try to add this kind of test.

natidea · 2017-06-09T16:59:34Z

Director Approved to Merge

…ion instances from preview workspace.

mavasani · 2017-06-09T18:25:23Z

@gafter @jasonmalinowski @heejaechang - Added a unit test

mavasani · 2017-06-09T18:33:36Z

@dotnet/roslyn-compiler Can I get another review? FYI that this fixes an Urgency-Now bug #20093 which already has director approval to merge.

sharwell · 2017-06-09T18:45:13Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerManager.AnalyzerExecutionContext.cs


-            public void ClearCompilationScopeMap(AnalyzerOptions analyzerOptions, Compilation compilation)
+            public void ClearCompilationScopeMap(Compilation compilation)
            {


💡 This lock is now technically unnecessary (ConditionalWeakTable provides synchronization), but I think it would be confusing to some readers to remove it.

Yes, leaving unchanged for now. I am going to remove a whole bunch of code in this file in dev15.6 soon.

sharwell · 2017-06-09T18:47:51Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerManager.AnalyzerExecutionContext.cs

            // This map stores the tasks to compute HostCompilationStartAnalysisScope for per-compilation analyzer actions, i.e. AnalyzerActions registered by analyzer's CompilationStartActions.
            // Compilation start actions will get executed once per-each AnalyzerAndOptions as user might want to return different set of custom actions for each compilation/analyzer options.
-            private Dictionary<AnalyzerOptions, ConditionalWeakTable<Compilation, Task<HostCompilationStartAnalysisScope>>> _lazyCompilationScopeMap;
+            private ConditionalWeakTable<Compilation, Dictionary<AnalyzerOptions, Task<HostCompilationStartAnalysisScope>>> _lazyCompilationScopeCache;


❓ Is there any reason why this can't be private static readonly? More specifically, is it possible for two different AnalyzerExecutionContext instances to have the same Compilation instance with different maps associated?

I am going to remove this map in a refactoring in dev15.6 very soon.

Is there a bug tracking this we can reference here?

I have a PR out: #20109

VSadov

LGTM

jasonmalinowski · 2017-06-09T19:48:36Z

src/EditorFeatures/Test/Preview/PreviewWorkspaceTests.cs

+            ExecuteAnalyzers(previewWorkspace, analyzers);
+
+            previewWorkspace.Dispose();
+            solutionObjectReference.AssertReleased();


Consider asserting that the workspace is also disposed, just to make sure that's covered.

I am explicitly disposing the workspace in the previous line. I am not sure what your recommendation is over here?

mavasani · 2017-06-09T20:00:30Z

@gafter Any other feedback?

AlekseyTs · 2017-06-09T21:09:02Z

Was Neal's feedback about tests addressed?

mavasani · 2017-06-09T21:09:55Z

@AlekseyTs - yes, I have added a unit test.

AlekseyTs · 2017-06-09T21:17:23Z

I sign-off on Neal's behalf.

Remove strong reference to AnalyzerOptions in the analyzer manager ex…

84ddab0

…ecution context Fixes dotnet#20065

dnfclas added the cla-already-signed label Jun 8, 2017

mavasani added Area-Analyzers Area-Compilers Area-IDE labels Jun 8, 2017

mavasani requested review from heejaechang and jasonmalinowski June 8, 2017 00:09

heejaechang approved these changes Jun 8, 2017

View reviewed changes

jasonmalinowski approved these changes Jun 8, 2017

View reviewed changes

MattGertz added the Pending director approval label Jun 8, 2017

gafter reviewed Jun 8, 2017

View reviewed changes

gafter self-assigned this Jun 8, 2017

gafter added this to the 15.3 milestone Jun 8, 2017

natidea added Approved to merge and removed Pending director approval labels Jun 9, 2017

sharwell mentioned this pull request Jun 9, 2017

IDE slowly gets less and less responsive and eventually crashes #20093

Closed

Add a unit test to ensure that analyzer execution does not leak solut…

426e587

…ion instances from preview workspace.

sharwell approved these changes Jun 9, 2017

View reviewed changes

VSadov approved these changes Jun 9, 2017

View reviewed changes

jasonmalinowski approved these changes Jun 9, 2017

View reviewed changes

jinujoseph approved these changes Jun 9, 2017

View reviewed changes

mavasani merged commit 13de789 into dotnet:master Jun 9, 2017

mavasani deleted the FixLeak branch June 9, 2017 21:18

This was referenced Jun 9, 2017

Remove the AnalyzerManager singleton to prevent storing analyzer exec… #20109

Merged

Make AnalyzerManager public and remove its static instance for analyzer state management #2830

Closed

		@@ -20,7 +20,7 @@ private sealed class AnalyzerExecutionContext

		// This map stores the tasks to compute HostCompilationStartAnalysisScope for per-compilation analyzer actions, i.e. AnalyzerActions registered by analyzer's CompilationStartActions.
		// Compilation start actions will get executed once per-each AnalyzerAndOptions as user might want to return different set of custom actions for each compilation/analyzer options.

Conversation

mavasani commented Jun 8, 2017

Uh oh!

mavasani commented Jun 8, 2017

Uh oh!

mavasani commented Jun 8, 2017

Uh oh!

mavasani commented Jun 8, 2017

Uh oh!

mavasani commented Jun 8, 2017

Uh oh!

heejaechang commented Jun 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonmalinowski commented Jun 8, 2017

Uh oh!

mavasani commented Jun 8, 2017

Uh oh!

mavasani commented Jun 8, 2017

Uh oh!

heejaechang commented Jun 8, 2017

Uh oh!

mavasani commented Jun 8, 2017

Uh oh!

gafter left a comment

Choose a reason for hiding this comment

Uh oh!

gafter commented Jun 8, 2017

Uh oh!

jasonmalinowski commented Jun 8, 2017

Uh oh!

heejaechang commented Jun 8, 2017

Uh oh!

natidea commented Jun 9, 2017

Uh oh!

mavasani commented Jun 9, 2017

Uh oh!

mavasani commented Jun 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VSadov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mavasani commented Jun 9, 2017

Uh oh!

AlekseyTs commented Jun 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mavasani commented Jun 9, 2017

Uh oh!

AlekseyTs commented Jun 9, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

AlekseyTs commented Jun 9, 2017 •

edited

Loading