Improve state management in analyzers

[sharwell]

• Access settings once per Compilation instead of once per syntax tree
• Properly store state in analyzer instances
• Eliminate use of ConditionalWeakTable<TKey, TValue>

[codecov-io]

Current coverage is 79.17%

Merging #1671 into master will increase coverage by +0.01% as of be25c16

@@            master   #1671   diff @@
======================================
  Files          539     539       
  Stmts        31799   31872    +73
  Branches      8928    8935     +7
  Methods                          
======================================
+ Hit          25175   25234    +59
- Partial       5178    5186     +8
- Missed        1446    1452     +6

Review entire Coverage Diff as of be25c16

Powered by Codecov. Updated on successful CI builds.

[vweijsters]

👍 Looks good to me. There is a risk that the new cache is not effective if the any analyzer is called concurrently for multiple compilations.

[sharwell]

@mavasani @tmeschter Are the compilation start actions for multiple Compilation instances invoked in parallel? For example, suppose I have compilations C1 and C2 representing two projects in the same solution, and I have two analyzers A1 and A2 installed in each of these projects. A1 and A2 each implement DiagnosticAnalyzer.Initialize by registering a single compilation start action. The compilation start action accesses the cache. Therefore the following ordering is optimal (other optimal orderings exist):

1. A1.CompilationStart(C1)
2. A2.CompilationStart(C1)
3. A1.CompilationStart(C2)
4. A2.CompilationStart(C2)

However, the following ordering would eliminate all benefits of the cache:

1. A1.CompilationStart(C1)
2. A1.CompilationStart(C2)
3. A2.CompilationStart(C1)
4. A2.CompilationStart(C2)

[mavasani]

Are the compilation start actions for multiple Compilation instances invoked in parallel? - The answer is yes and no :)

For background analysis which populates the error list. we currently execute analyzers sequentially and never move back to an older compilation - so the callbacks should be as per your first ordering.

For user facing operations where we have the UI thread and can execute analysis concurrently (such as FixAll occurrences across project or solution scope), we execute the analyzers concurrently across documents, each of which forks a different CompilationWithAnalyzers, so the second ordering is very likely.

Note that both of these are implementation details that can change anytime based on our perf investigations - I wouldn't rely on it for your caching logic. Having said that, I totally agree with your current PR of getting rid of the static conditional weak table - they are purely evil for IDE analyzer perf.

I personally feel its an overkill to require analyzer authors to perform state and lifetime management for shared state across analyzer instances - I am going to open a work item on our GitHub repo, provide a proposal and loop you into the design discussions.