Remove the AnalyzerManager singleton to prevent storing analyzer exec… by mavasani · Pull Request #20109 · dotnet/roslyn

mavasani · 2017-06-08T18:29:28Z

…ution state across compilations

Now we throw away all the analyzer execution state once the CompilationWithAnalyzers goes out of scope.

…ution state across compilations Now we throw away all the analyzer execution state once the CompilationWithAnalyzers goes out of scope.

heejaechang · 2017-06-08T18:35:59Z

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 readonly Dictionary<AnalyzerOptions, Task<HostCompilationStartAnalysisScope>> _compilationScopeMap;


I believe AnalyzerManager is per compilationWithAnalyzer now which is for one compilation meaning AnalyzerOption will be same for all? we might not need this map anymore? #Resolved

Hmm given that each CompilationWithAnalyzers instance will create a new cloned compilation with a new event queue, your claim seems to be correct. Let me cleanup a bit more. #Resolved

heejaechang · 2017-06-08T18:39:52Z

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

        // This cache stores the analyzer execution context per-analyzer (i.e. registered actions, supported descriptors, etc.).
-        private readonly ConditionalWeakTable<DiagnosticAnalyzer, AnalyzerExecutionContext> _analyzerExecutionContextMap =
-            new ConditionalWeakTable<DiagnosticAnalyzer, AnalyzerExecutionContext>();
+        private readonly Dictionary<DiagnosticAnalyzer, AnalyzerExecutionContext> _analyzerExecutionContextMap =


concurrent dictionary and remove the lock? or just create whole map in immutable map at once might better? since this seems get called many times? #Resolved

mavasani · 2017-06-09T21:21:05Z

Test failure: https://ci.dot.net/job/dotnet_roslyn/job/dev15.6/job/windows_debug_unit32_prtest/176/

xunit produced no error output but had exit code 1

mavasani · 2017-06-09T21:21:14Z

@dotnet-bot retest windows_debug_unit32_prtest please

mavasani · 2017-06-09T22:04:18Z

Tagging @dotnet/roslyn-analysis @dotnet/roslyn-compiler for review. This change is a follow up cleanup change to #20096

jcouv · 2017-06-12T17:58:24Z

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

-        /// Gets the default instance of the AnalyzerManager for the lifetime of the analyzer host process.
-        /// </summary>
-        public static readonly AnalyzerManager Instance = new AnalyzerManager();
+        private readonly object _gate = new object();


Is this used? #Resolved

Ah yes, not after the refactoring in the last commit. Thanks, will remove.

jcouv · 2017-06-12T18:04:03Z

public class TestDiagnosticAnalyzerDriver : IDisposable

Should we remove IDisposable, since Dispose does nothing? #Resolved

Refers to: src/EditorFeatures/TestUtilities/Diagnostics/TestDiagnosticAnalyzerDriver.cs:17 in 67ad810. [](commit_id = 67ad810, deletion_comment = False)

jcouv · 2017-06-12T18:06:04Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/CompilationWithAnalyzers.cs

        /// It clears the cached internal state (supported descriptors, registered actions, exception handlers, etc.) for analyzers.
        /// </summary>
        /// <param name="analyzers">Analyzers whose state needs to be cleared.</param>
+        [Obsolete("This API is no longer required to be invoked. Analyzer state is automatically cleaned up when CompilationWithAnalyzers instance is released.")]


FYI @jaredpar @gafter for public API change (marking as obsolete).

jcouv · 2017-06-12T18:11:23Z

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

+            _analyzerExecutionContextMap = CreateAnalyzerExecutionContextMap(SpecializedCollections.SingletonEnumerable(analyzer));
+        }
+
+        private ImmutableDictionary<DiagnosticAnalyzer, AnalyzerExecutionContext> CreateAnalyzerExecutionContextMap(IEnumerable<DiagnosticAnalyzer> analyzers)


ImmutableArray instead of IEnumerable for analyzers?
Can this method be made static? #ByDesign

Hum, the singleton case may fit better with IEnumerable (than ImmutableArray). Ignore that comment.

In reply to: 121486170 [](ancestors = 121486170)

AlekseyTs · 2017-06-12T23:54:53Z

Done with review pass. #Closed

mavasani · 2017-06-13T22:17:28Z

public class TestDiagnosticAnalyzerDriver : IDisposable

Done.

In reply to: 307869813 [](ancestors = 307869813)

Refers to: src/EditorFeatures/TestUtilities/Diagnostics/TestDiagnosticAnalyzerDriver.cs:17 in 67ad810. [](commit_id = 67ad810, deletion_comment = False)

jcouv

LGTM

AlekseyTs · 2017-06-14T04:50:29Z

src/EditorFeatures/TestUtilities/Diagnostics/TestDiagnosticAnalyzerDriver.cs

 namespace Microsoft.CodeAnalysis.UnitTests.Diagnostics
 {
-    public class TestDiagnosticAnalyzerDriver : IDisposable
+    public class TestDiagnosticAnalyzerDriver


public class TestDiagnosticAnalyzerDriver [](start = 4, length = 41)

Isn't removal of the interface a breaking change? The class is public, some clients could implicitly convert it to IDisposable, which is going to be an error now.

This is a test only type though. If we want to preserve API for test types, I can revert it. @jcouv @gafter?

Yes, I don't think test types are part of our public/supported API.

This is a test only type though

Sorry, missed this part.

AlekseyTs · 2017-06-14T04:51:03Z

Done with review pass.

AlekseyTs

LGTM

mavasani · 2017-06-14T14:19:54Z

@dotnet/roslyn-infrastructure I thought the VS integration tests are currently failing, and hence we are allowed to merge without requiring them to pass. However, merging the pull request still seems to be blocked on these tests.

jaredpar · 2017-06-14T14:44:25Z

@mavasani the underlying problem with the VS integration tests was fixed yesterday afternoon. Hence the tests are required again.

jaredpar · 2017-06-14T14:51:39Z

retest windows_debug_vs-integration_prtest please

jaredpar · 2017-06-14T14:51:48Z

retest windows_release_vs-integration_prtest please

mavasani · 2017-06-14T17:01:02Z

Thanks @jaredpar. The Debug integration test failed with following output:

System.Management.Automation.RuntimeException: Command failed to execute: C:\Program Files (x86)\Microsoft Visual Studio\Preview\Enterprise\MSBuild\15.0\Bin\msbuild.exe /warnaserror /nologo /m /nodeReuse:false /consoleloggerparameters:Verbosity=minimal /filelogger /fileloggerparameters:Verbosity=normal /p:BootstrapBuildPath=D:\j\workspace\windows_debug---1522b458\Binaries\Bootstrap BuildAndTest.proj /p:Configuration=Debug /p:Test64=false /p:TestVsi=true /p:TestDesktop=False /p:TestCoreClr=False /p:TestVsiNetCore=false /p:PathMap=D:\j\workspace\windows_debug---1522b458=q:\roslyn /p:Feature=pdb-path-determinism /fileloggerparameters:LogFile=D:\j\workspace\windows_debug---1522b458\Binaries\Build.log;verbosity=diagnostic /p:DeployExtension=false /p:RoslynRuntimeIdentifier=win7-x64 /p:DeployExtensionViaBuild=False /p:TreatWarningsAsErrors=true /p:ProcDumpDir=c:\SysInternals

mavasani · 2017-06-14T17:20:00Z

retest windows_debug_vs-integration_prtest please

jasonmalinowski · 2017-06-14T17:54:02Z

Actually, I only disabled the requirement in master, but didn't do it here. 😦

mavasani · 2017-06-14T18:40:25Z

@jasonmalinowski @jaredpar - The debug integration test job is consistently failing with the same error as above on branches other then master. See https://ci.dot.net/job/dotnet_roslyn/job/dev15.6/job/windows_debug_vs-integration_prtest/260/ for this PR (dev15.6 branch) and also the failure for #20177 (features/ioperation branch)

ivanbasov · 2017-06-14T18:41:25Z

Please ignore integration tests for a while.

mavasani · 2017-06-14T18:42:36Z

@ivanbasov - Merge pull request is still disabled for dev15.6 branch, can we remove this requirement?

ivanbasov · 2017-06-14T18:48:45Z

I think, yes: we should disable integration tests in 15.6 too for a while. @jaredpar, @jasonmalinowski to confirm.

If yes, will it be enough to cherry-pick #20216, once it will be merged to master?

jasonmalinowski · 2017-06-14T20:18:58Z

And to your second question: yes. I'd just have @jimmy9988 do the merge as he'll be doing that flow anyways.

mavasani · 2017-06-15T16:40:22Z

@ivanbasov I am still blocked on VSI failures for this PR. Please advise how to get this through.

ivanbasov · 2017-06-15T17:58:28Z

@jimmy9988 could you please help with the merge? We have integration tests optional in master. We even have them passed in master.

enshuoh · 2017-06-15T18:16:18Z

I already merged the fix #20216 to dev15.6 in #20234.

mavasani · 2017-06-15T18:29:03Z

@dotnet retest windows_debug_vs-integration_prtest please

mavasani · 2017-06-15T18:29:33Z

@dotnet-bot retest windows_release_vs-integration_prtest please

mavasani · 2017-06-15T23:53:49Z

retest this please

mavasani · 2017-06-15T23:54:46Z

@dotnet-bot retest windows_debug_vs-integration_prtest please

mavasani · 2017-06-15T23:55:03Z

@dotnet-bot retest windows_release_vs-integration_prtest please

mavasani · 2017-06-16T17:44:02Z

@dotnet/roslyn-infrastructure Seems like dev15.6 is in broken state:

Type of conditional expression cannot be determined because there is no implicit conversion between 'UserOperationBooster' and '' [D:\j\workspace\windows_build---d2995bb3\src\Workspaces\Remote\ServiceHub\ServiceHub.csproj]

Are we fixing this soon? Is there any commit I can cherry-pick from master to fix it?

enshuoh · 2017-06-16T17:54:08Z

@mavasani Yes. Cyrus changed a type to struct in master which is used in 15.6. This change brought from master breaks 15.6. @heejaechang is working on this.

Remove the AnalyzerManager singleton to prevent storing analyzer exec…

106fe57

…ution state across compilations Now we throw away all the analyzer execution state once the CompilationWithAnalyzers goes out of scope.

mavasani added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Jun 8, 2017

mavasani requested a review from heejaechang June 8, 2017 18:29

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

mavasani requested a review from jinujoseph June 8, 2017 18:30

heejaechang reviewed Jun 8, 2017

View reviewed changes

Address PR feedback

67ad810

mavasani mentioned this pull request Jun 9, 2017

Remove strong reference to AnalyzerOptions in the analyzer manager ex… #20096

Merged

mavasani added Area-Analyzers Area-Compilers and removed PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. labels Jun 9, 2017

jcouv reviewed Jun 12, 2017

View reviewed changes

mavasani added 2 commits June 13, 2017 15:10

Address PR feedback

ca5b0d9

Merge remote-tracking branch 'upstream/dev15.6' into AnalyzerContext

640b407

jcouv approved these changes Jun 13, 2017

View reviewed changes

AlekseyTs reviewed Jun 14, 2017

View reviewed changes

AlekseyTs approved these changes Jun 14, 2017

View reviewed changes

mavasani closed this Jun 15, 2017

mavasani reopened this Jun 15, 2017

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

Merge remote-tracking branch 'upstream/dev15.6' into AnalyzerContext

cb77360

Merge remote-tracking branch 'upstream/dev15.6' into AnalyzerContext

9383b03

mavasani merged commit b4c979c into dotnet:dev15.6 Jun 16, 2017

mavasani deleted the AnalyzerContext branch June 16, 2017 23:41

aolszowka mentioned this pull request Feb 2, 2021

Operations Which Have a Default StringComparer Should Strongly Suggest Explicitly Specifying the StringComparer dotnet/runtime#47775

Closed

Conversation

mavasani commented Jun 8, 2017

Uh oh!

heejaechang Jun 8, 2017 • edited by mavasani Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

heejaechang Jun 8, 2017 • edited by mavasani Loading Uh oh! There was an error while loading. Please reload this page.

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!

mavasani commented Jun 9, 2017

Uh oh!

mavasani commented Jun 9, 2017

Uh oh!

jcouv Jun 12, 2017 • edited by mavasani Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv commented Jun 12, 2017 • edited by mavasani Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jun 12, 2017 • edited by mavasani Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

mavasani commented Jun 13, 2017

Uh oh!

jcouv 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Jun 14, 2017

Uh oh!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

mavasani commented Jun 14, 2017

Uh oh!

jaredpar commented Jun 14, 2017

Uh oh!

jaredpar commented Jun 14, 2017

Uh oh!

jaredpar commented Jun 14, 2017

Uh oh!

mavasani commented Jun 14, 2017

Uh oh!

mavasani commented Jun 14, 2017

Uh oh!

jasonmalinowski commented Jun 14, 2017

Uh oh!

mavasani commented Jun 14, 2017

Uh oh!

ivanbasov commented Jun 14, 2017

heejaechang Jun 8, 2017 •

edited by mavasani

Loading

mavasani Jun 8, 2017 •

edited

Loading

heejaechang Jun 8, 2017 •

edited by mavasani

Loading

jcouv Jun 12, 2017 •

edited by mavasani

Loading

jcouv commented Jun 12, 2017 •

edited by mavasani

Loading

jcouv Jun 12, 2017 •

edited by mavasani

Loading

AlekseyTs commented Jun 12, 2017 •

edited

Loading

mavasani commented Jun 14, 2017 •

edited

Loading