Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UtilityAnalyzer: Reduce lock contention in ShouldGenerateMetrics #7411

Closed
csaba-sagi-sonarsource opened this issue Jun 13, 2023 · 5 comments · Fixed by #8397
Closed

UtilityAnalyzer: Reduce lock contention in ShouldGenerateMetrics #7411

csaba-sagi-sonarsource opened this issue Jun 13, 2023 · 5 comments · Fixed by #8397
Assignees
Labels
Type: Performance It takes too long.
Milestone

Comments

@csaba-sagi-sonarsource
Copy link
Contributor

#6576 introduced a performance regression that was reverted in #7262. Our investigation showed the cause of the regression was that using the RegisterSemanticModelAction changed the behaviour of the analyzer to a multithreaded approach. ShouldGenerateMetrics queries the UnchangedFilesCache and calls GeneratedCodeRecognizer.IsGenerated. As both are based on synchronized collections it creates a thread contention which caused the performance regression.
The idea is to precalculate the generated and unchanged files and in the SemanticModelAction only use the values. In theory, this should remove the thread contention, enabling us to introduce one again the changes made in #6576.

@martin-strecker-sonarsource
Copy link
Contributor

The UnchangedFilesCache uses the ConditionalWeakTable<TKey,TValue>.GetValue Method which calls SonarAnalysisContextBase<TContext>.CreateUnchangedFilesHashSet, which hits the disk. From the remark section of GetValue we learn:

If multiple threads try to create the same key, createValueCallback may be invoked multiple times with the same key.

This might result in CreateUnchangedFilesHashSet executing File.ReadAllLines(unchangedFilesPath) multiple times for the same file, if ShouldGenerateMetrics is called by multiple threads at the same time.
It might be enough, to lock the value creation in case the key can not be found:

if (UnchangedFilesCache.TryGetValue(Compilation, out var unchangedFiles))
{
    return unchangedFiles;
}
else
{
    lock (UnchangedFilesLock)
    {
        // Might be populated by now
        if (UnchangedFilesCache.TryGetValue(Compilation, out var unchangedFiles))
        {
            return unchangedFiles;
        }
        else
        {
            var unchangedFiles = CreateUnchangedFilesHashSet();
            UnchangedFilesCache.AddOrUpdate(Compilation, unchangedFiles);
        }
    }
}

This still locks, but the file access only occurs once. I would do this, if the precalculation of the cache does not work for some reason.

@martin-strecker-sonarsource
Copy link
Contributor

Unrelated minor perf improvement:

        private static bool HasGeneratedFileName(SyntaxTree tree)
        {
            if (string.IsNullOrEmpty(tree.FilePath))
            {
                return false;
            }

            var fileName = Path.GetFileName(tree.FilePath);
            return Array.Exists(GeneratedFileParts, part => fileName.IndexOf(part, StringComparison.OrdinalIgnoreCase) >= 0);
        }

Same can be done for AutoGeneratedCommentParts in HasAutoGeneratedComment.

@martin-strecker-sonarsource
Copy link
Contributor

Other things to look at:

  • HasGeneratedCodeAttribute: Is there a reason why we call DescendantNodesAndSelf (walking the entire tree) instead of ChildNodes?
  • Why attributeName.EndsWith? GetAttributeName should give the unqualified name.
  • Why is IsGenerated not called via SyntaxTreeExtensions.IsGenerated (which caches) by UtilityAnalyzers? Should we make the method uncallable or inline the caching to make it impossible to call the uncached version? It is e.g. also called by the ClassAndMethodName analyzer.

We may need to split the issue before we work on it.

@martin-strecker-sonarsource
Copy link
Contributor

Another root cause might be the queuing of the tasks for the SyntaxTree registration. It might also help to enable concurrent analyzes. This requires the Analyzers to be stateless. See #7288 and #6674.

@martin-strecker-sonarsource
Copy link
Contributor

martin-strecker-sonarsource commented Nov 24, 2023

The contention on IsGenerated can be seen in the profiling here:
image

TryGetValue of the CWT runs under a lock in all implementations (.Net and .Net Framework):
https://github.com/microsoft/referencesource/blob/51cf7850defa8a17d815b4700b67116e3fa283c2/mscorlib/system/runtime/compilerservices/ConditionalWeakTable.cs#L105-L117

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Performance It takes too long.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants