Add support for registering an analyzer for concurrent execution of its actions #7184

mavasani · 2015-12-02T18:08:44Z

By default, analyzer driver never makes concurrent callbacks into a single analyzer instance. This change adds a new public API AnalysisContext.RegisterConcurrentExecution to enable thread-safe analyzers to opt into receiving concurrent callbacks, which should theoretically improve performance of such analyzers for concurrent builds.

Fixes #6737

This change replaces synchronous lock statements in the core analyzer executor and analyzer state tracker (for IDE live analysis) with semaphore slim DisposableWaitAsync invocations. This ensures that most of the analyzer driver and analyzer executor code is now async, with only the eventual callback into the analyzers being a synchronous method ([ExecuteAndCatchIfThrows_NoLock](http://source.roslyn.io/Microsoft.CodeAnalysis/R/02d7df8203d3591e.html)) Testing: I verified that the self-build time of Roslyn.sln (which uses a few analyzers), is almost identical after this change. We are hoping that this will improve build performance when there is lot of lock contention, which has shown up from perf traces (see dotnet#6053). Fixes dotnet#6399

…ts actions. By default, analyzer driver never makes concurrent callbacks into a single analyzer instance. This change adds a new public API 'AnalysisContext.RegisterConcurrentExecution' to enable thread-safe analyzers to receive concurrent callbacks, which should theoretically improve performance of such analyzers for concurrent builds. Fixes dotnet#6737

mavasani · 2015-12-02T18:20:06Z

src/Compilers/Core/Portable/PublicAPI.Unshipped.txt

@@ -76,6 +76,7 @@ virtual Microsoft.CodeAnalysis.MetadataReferenceResolver.ResolveMissingAssembly(
 virtual Microsoft.CodeAnalysis.SourceReferenceResolver.ReadText(string resolvedPath) -> Microsoft.CodeAnalysis.Text.SourceText
 Microsoft.CodeAnalysis.SemanticModel.GetOperation(Microsoft.CodeAnalysis.SyntaxNode node, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) -> Microsoft.CodeAnalysis.Semantics.IOperation
 abstract Microsoft.CodeAnalysis.SemanticModel.GetOperationCore(Microsoft.CodeAnalysis.SyntaxNode node, System.Threading.CancellationToken cancellationToken) -> Microsoft.CodeAnalysis.Semantics.IOperation
+abstract Microsoft.CodeAnalysis.Diagnostics.AnalysisContext.RegisterConcurrentExecution() -> void


This is the public API being added.

mavasani · 2015-12-02T18:20:49Z

@JohnHamby @gafter @srivatsn @heejaechang can you please review?

msJohnHamby · 2015-12-02T21:26:08Z

In a quick scan of the code, I didn't see anything preventing concurrent execution of actions that are too closely related. For example, it's probably not sound to simultaneously execute two syntax node actions registered by a code block start action for a given code block (because the two syntax node actions are very likely to be sharing state for the code block). Requiring an analyzer that does this to have no concurrency is too restrictive.

At some point in the past I wrote up a proposed set of rules for when two actions are too closely related to execute in parallel.

heejaechang · 2015-12-02T21:35:16Z

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

+        /// For example, end actions registered on any analysis unit (compilation, code block, operation block, etc.) are by definition semantically dependent on analysis from non-end actions registered on the same analysis unit.
+        /// Hence, end actions are never executed concurrently with non-end actions operating on the same analysis unit.
+        /// </remarks>
+        public abstract void RegisterConcurrentExecution();


is it decide so that people can't flip it more than once?

All of the other methods named "Register***" register an action. Maybe "EnableConcurrency" or "EnableConcurrentActions" or "EnableConcurrentExecution"?

@heejaechang's question is interesting. I don't think we need to support flipping concurrence on and off, but if someone can make a case for it we should discuss that.

I'm +1 to @JohnHamby's point about Register versus Enable. I like EnableConcurrentExecution.

heejaechang · 2015-12-02T21:35:28Z

👍

mavasani · 2015-12-02T21:39:17Z

it's probably not sound to simultaneously execute two syntax node actions registered by a code block start action for a given code block (because the two syntax node actions are very likely to be sharing state for the code block).

I am unsure why we would add such a restriction. What if these actions themselves are thread-safe and can deal with access/update of shared state concurrently? Adding such a restriction will prevent further performance improvements for such an analyzer.

I think we are talking about 3 buckets here:

Completely non-concurrent
Maximally concurrent and thread-safe
Partially concurrent and thread safe

I propose we treat the new API to enforce analyzer needs to be (2) - where only semantically dependent actions are not executed concurrently. End actions by definition depend on non-end actions executing on same analysis unit (compilation, code block, operation block, etc.) and hence cannot be executed before them - driver guarantees that. However, sort of related actions that you mention can be written in thread-safe way and executed concurrently without any functional change in the output.
If and when we get to adding support for 3., we can allow authors to define subset of actions or action kinds that it deems thread safe.

heejaechang · 2015-12-02T21:46:59Z

since we never promised ordering between same level actions (syntax tree, syntax node, code block), I think running those concurrently doesnt break any contracts.

we never promised we will run syntax node actions in in-order tree traversal or pre or post, or tree in the order it shows up in compilation and etc.

so, I think it is fine running them concurrently and let the author deal with managing concurrency.

heejaechang · 2015-12-02T21:50:57Z

by the way, one thing, I think we need to explicitly state that even if one is registered to be executed concurrently, Host can still decide it runs it non-concurrent manner. basically, the call is to let host know characteristic of the analyzer, not to control behavior of the host.

host behavior is completely owned by the host (driver) and analyzer provider can't change it in any way.

in that sense, why is this a method call (runtime behavior) rather than metadata (attribute?) on the analyzer? first one feels like it is controlling behavior of host, when second feels like I am annotating characteristic of the analyzer.

msJohnHamby · 2015-12-02T22:00:41Z

I hunted up the most succinct statement of the "related actions" rules I could find, and pasted it below. The incentive for such restrictions is to make a very high percentage of analyzers achieve maximal safe concurrency without an author having to take locks. Getting locking correct is very difficult, and we shouldn't push the problem entirely onto analyzer authors. The ideal experience is that an author turns on concurrency and an analyzer keeps working.

Here's the last draft of the rules. What we're doing with shared state will necessitate some refinement:

 The Initialize methods of two or more different analyzers can run concurrently. We make no guarantees about state shared between analyzers, or about static state.

 The actions of two or more different analyzers can run concurrently. We make no guarantees about state shared between analyzers.

 Any number of actions that were registered directly by a single analyzer’s Initialize method can run concurrently. We make no guarantees about static state, or about cross-compilation state.

 An action A is an ancestor of action B if B was registered either by A or by another action that has A as an ancestor. Two actions X and Y are related if any of these hold:

a. X is an ancestor of Y.
b. Y is an ancestor of X.
c. X and Y have a common ancestor.
d. X and Y are the same action, and that action has an ancestor.

Any number of actions of a single analyzer can run concurrently so long as no two of those actions are related.

heejaechang · 2015-12-02T22:23:48Z

@JohnHamby I dont think user can understand that rule. so either user will not mark it as concurrent or use lock regardless.

mavasani · 2015-12-02T22:30:46Z

Getting locking correct is very difficult, and we shouldn't push the problem entirely onto analyzer authors. The ideal experience is that an author turns on concurrency and an analyzer keeps working.

I think analyzer author signing up for concurrency have to ensure their actions are thread safe (via locking, concurrent data structures, etc.). They cannot just turn on concurrency and expect their existing analyzer which shares state between actions to keep working.

I agree with your definition of related actions, but this won't be easy for any analyzer author to understand. We shouldn't require them to figure out what actions are related or unrelated and then go and implement thread safety.

Additionally, one big drawback of this restriction would be that all actions registered within a compilation start action would be related actions and hence wouldn't be executed concurrently - I believe majority of analyzers are written as compilation start analyzers just to fetch some well known types from the compilations, but have no related actions or state sharing.

msJohnHamby · 2015-12-03T00:26:25Z

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

+
+        /// <summary>
+        /// Register for concurrent execution of analyzer actions registered by this analyzer.
+        /// An analyzer that registers for concurrent execution should be more performant then a non-concurrent analyzer.


How about "can have better performance than" or "can be more efficient than"? I still haven't accepted "performant" as word--the first definition of it I could find online included "Commonly used by Microsofties but not a real word."

At least, "then" should become "than".

Sure, will change.

…tExecution

mavasani · 2015-12-03T23:37:49Z

@Pilchie @JohnHamby I have addressed your feedback and updated the PR.

Pilchie · 2015-12-03T23:52:20Z

👍

msJohnHamby · 2015-12-04T06:21:25Z

My "related actions" proposal turns out to be too pessimistic for some very important cases. There will be a follow-on discussion about the best way to deal with throttling concurrency in the presence of mutable state.

I sign off.

mavasani · 2015-12-04T06:30:29Z

Thanks John.

Add support for registering an analyzer for concurrent execution of its actions

mavasani · 2015-12-04T14:03:33Z

I have filed #7228 to track John's proposal for enhancing the API.

mavasani added 6 commits November 19, 2015 11:04

Merge remote-tracking branch 'upstream/master'

2ec2d4d

Merge to latest sources.

c9ded1e

Merge remote-tracking branch 'upstream/master'

a75315d

Merge remote-tracking branch 'upstream/master'

8d75815

mavasani added the Area-Analyzers label Dec 2, 2015

dnfclas added the cla-already-signed label Dec 2, 2015

mavasani reviewed Dec 2, 2015
View reviewed changes

heejaechang reviewed Dec 2, 2015
View reviewed changes

msJohnHamby reviewed Dec 3, 2015
View reviewed changes

mavasani added 2 commits December 3, 2015 14:29

Merge remote-tracking branch 'upstream/master' into RegisterConcurren…

7b49d82

…tExecution

Address PR feedback from JohnHamby and KevinPi

2d65833

mavasani added a commit that referenced this pull request Dec 4, 2015

Merge pull request #7184 from mavasani/RegisterConcurrentExecution

069d016

Add support for registering an analyzer for concurrent execution of its actions

mavasani merged commit 069d016 into dotnet:master Dec 4, 2015

mavasani mentioned this pull request Dec 4, 2015

Allow concurrent analyzers to throttle concurrency in the presence of mutable state #7228

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for registering an analyzer for concurrent execution of its actions #7184

Add support for registering an analyzer for concurrent execution of its actions #7184

mavasani commented Dec 2, 2015

mavasani Dec 2, 2015

mavasani commented Dec 2, 2015

msJohnHamby commented Dec 2, 2015

heejaechang Dec 2, 2015

msJohnHamby Dec 3, 2015

Pilchie Dec 3, 2015

mavasani Dec 3, 2015

heejaechang commented Dec 2, 2015

mavasani commented Dec 2, 2015

heejaechang commented Dec 2, 2015

heejaechang commented Dec 2, 2015

msJohnHamby commented Dec 2, 2015

heejaechang commented Dec 2, 2015

mavasani commented Dec 2, 2015

msJohnHamby Dec 3, 2015

mavasani Dec 3, 2015

mavasani commented Dec 3, 2015

Pilchie commented Dec 3, 2015

msJohnHamby commented Dec 4, 2015

mavasani commented Dec 4, 2015

mavasani commented Dec 4, 2015

Add support for registering an analyzer for concurrent execution of its actions #7184

Add support for registering an analyzer for concurrent execution of its actions #7184

Conversation

mavasani commented Dec 2, 2015

mavasani Dec 2, 2015

Choose a reason for hiding this comment

mavasani commented Dec 2, 2015

msJohnHamby commented Dec 2, 2015

heejaechang Dec 2, 2015

Choose a reason for hiding this comment

msJohnHamby Dec 3, 2015

Choose a reason for hiding this comment

Pilchie Dec 3, 2015

Choose a reason for hiding this comment

mavasani Dec 3, 2015

Choose a reason for hiding this comment

heejaechang commented Dec 2, 2015

mavasani commented Dec 2, 2015

heejaechang commented Dec 2, 2015

heejaechang commented Dec 2, 2015

msJohnHamby commented Dec 2, 2015

heejaechang commented Dec 2, 2015

mavasani commented Dec 2, 2015

msJohnHamby Dec 3, 2015

Choose a reason for hiding this comment

mavasani Dec 3, 2015

Choose a reason for hiding this comment

mavasani commented Dec 3, 2015

Pilchie commented Dec 3, 2015

msJohnHamby commented Dec 4, 2015

mavasani commented Dec 4, 2015

mavasani commented Dec 4, 2015