Cache ShouldAnalyzeTree results for better performance

[sebastien-marichal]

Fixes #8406

Solution using Tuple to store the latest SyntaxTree within the RegisterNodeAction of CompilationStartActionContext.

On oqtane framework, before changes:

After changes:

From RegisterNodeAction point of view:
Before:

After:

[sebastien-marichal]

For now, I use this single bool to cache the result when the SyntaxTree is null as I cannot store null as a key in the ConcurrentDictionary.

[martin-strecker-sonarsource]

Can you open a second PR with this change in SonarCompilationStartAnalysisContext

    public void RegisterNodeAction<TSyntaxKind>(GeneratedCodeRecognizer generatedCodeRecognizer, Action<SonarSyntaxNodeReportingContext> action, params TSyntaxKind[] syntaxKinds)
        where TSyntaxKind : struct
    {
        if (HasMatchingScope(AnalysisContext.SupportedDiagnostics))
        {
            var lastShouldAnalyze = default(Tuple<SyntaxTree, bool>);
            Context.RegisterSyntaxNodeAction(x =>
            {
                var tree = x.Node.SyntaxTree;
                var last = lastShouldAnalyze; // Make a local copy of the reference to avoid concurrency issues between the access of Item1 and Item2
                bool shouldAnalyze;
                if (ReferenceEquals(last?.Item1, tree))
                {
                    shouldAnalyze = last!.Item2;
                }
                else
                {
                    // Inlined from "Execute"
                    shouldAnalyze = ShouldAnalyzeTree(x.Node.SyntaxTree, generatedCodeRecognizer)
                        && SonarAnalysisContext.LegacyIsRegisteredActionEnabled(AnalysisContext.SupportedDiagnostics, x.Node.SyntaxTree)
                        && AnalysisContext.ShouldAnalyzeRazorFile(x.Node.SyntaxTree);
                    lastShouldAnalyze = new Tuple<SyntaxTree, bool>(tree, shouldAnalyze);
                }
                if (shouldAnalyze)
                {
                    action(new(AnalysisContext, x));
                }
            }, syntaxKinds);
        }
    }

It would be interesting to see the savings here, as this doesn't come with the overhead of the concurrent dictionaries.

[sebastien-marichal]

Using this approach is not as good as caching in a dictionary.

But that's only from ShouldAnalyzeTree point of view.

From SonarCompilationStartAnalysisContext.RegisterNodeAction point of view, it is faster:

Using your solution, with a tuple:

My solution, with a ConcurrentDisctionary:

For reference, here it is before any changes (of this PR):

[sebastien-marichal]

Am I right in assuming that this is guaranteed that within the same compilation, the SyntaxTrees will be analyzed sequentially?
Once lastShouldAnalyze will change that means analysis on the last SyntaxTree is done?

[martin-strecker-sonarsource]

Please open two (draft) PRs so we can compare both solutions.

Am I right in assuming that this is guaranteed that within the same compilation, the SyntaxTrees will be analyzed sequentially?

There is no such guarantee to rely on:

There are no ordering constraints for the invocations of any actions, except those introduced by start and end actions (compilation start, compilation end, symbol start, symbol end, operation block start, operation block end, code bock start, and code block end actions). Invocations of actions of one kind may be interleaved with invocations of actions of another kind. A given host environment might appear to provide a predictable order—for example, invoking syntax tree actions before semantic model actions, or symbol actions before code block actions, but depending on such an ordering is not safe.

Source

It was a shot in the blue by me. We can make an educated guess now for the analyzer runner that is in place in the build context. That runner is likely different from the one running in the IDE context. Your solution is more robust but comes at the cost of:

• Locking on write (which is somewhat common)
• Slow on a large number of SyntaxTree per compilation
• Holds references to all SyntaxTrees at the end of the compilation, even though only a small portion is still needed
• Does not work well when one of the additional files changes (namely SonarLint.xml).

We may need to come up with a compromise between only covering "latest" (mine) and "all" (yours). With the following characteristics

• Cache is limited in size (e.g. up CPU cores elements, or up to ~12 elements (linear search for up to 12 elements is considered as fast as a hash-based lookup, see e.g. https://terrainformatica.com/2017/10/15/when-linear-search-is-faster-than-stdmapfind-and-stdunordered_mapfind/))
• First in first out or last accessed semantics
• Lock-free at the cost of imprecision

At the same time, we do not have the time to over-engineer it.

[sebastien-marichal]

I will keep this one PR for the tuple solution.
I opened #8411 with the ConcurrentDirectory solution

[martin-strecker-sonarsource]

I don't like the idea of a Dictionary that holds references to SyntaxTrees on an object, that may outlive the compilation. See also https://github.com/dotnet/roslyn-analyzers/blob/main/src/Microsoft.CodeAnalysis.Analyzers/Microsoft.CodeAnalysis.Analyzers.md#rs1008-avoid-storing-per-compilation-data-into-the-fields-of-a-diagnostic-analyzer for why this isn't a good idea.

Let's keep this PR as a reference, but we should try what I proposed here as well.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

100.0% Coverage
0.0% Duplication

[sebastien-marichal]

Closing this PR in favor of #8414.
The scoped tuple solution tends to have cache misses due to the SyntaxTrees being analyzed concurrently.