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.

Sign up for GitHub

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

Jump to bottom

Make UtilityAnalyzerBase stateless by removing properties #8221

Merged
merged 4 commits into from
Oct 20, 2023
Merged

Make UtilityAnalyzerBase stateless by removing properties #8221

merged 4 commits into from
Oct 20, 2023

Conversation

martin-strecker-sonarsource
Copy link
Contributor

Fixes #7288
Re-open of #6902
Replaces #8218

@martin-strecker-sonarsource martin-strecker-sonarsource mentioned this pull request Oct 18, 2023
Closed
@martin-strecker-sonarsource martin-strecker-sonarsource marked this pull request as ready for review October 18, 2023 14:19
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@martin-strecker-sonarsource
Copy link
Contributor Author

martin-strecker-sonarsource commented Oct 19, 2023 •
edited
Loading

Performance measurement with the analyzer runner, the CSVHelper project, and 4 runs of master(org) and PR (chg) each can be found here (internal only)
https://docs.google.com/spreadsheets/d/1MgHsEJIyG6n_ZsuYWYGCZ0_CstZAD34izhuPtHolsyQ/edit?usp=sharing

Summary

Analyzer Min (org) Min (chg) Diff   Avg (org) Avg (chg) Diff   Max (org) Max (chg) Diff
Overall 55267 55417 -0.27%   55484.5 55976.8 0.89%   55609 56582 1.75%
AnalysisWarningAnalyzer 11 11 0.00%   11.0 11.0 0.00%   11 11 0.00%
CopyPasteTokenAnalyzer 1082 1100 1.66%   1100.0 1124.8 2.25%   1128 1163 3.10%
FileMetadataAnalyzer 161 162 0.62%   163.5 166.0 1.53%   170 173 1.76%
LogAnalyzer 8 8 0.00%   8.5 8.0 -5.88%   9 8 -11.11%
MetricsAnalyzer 3497 3553 1.60%   3534.5 3611.5 2.18%   3575 3688 3.16%
SymbolReferenceAnalyzer 47042 47145 0.22%   47272.8 47607.0 0.71%   47379 48151 1.63%
TokenTypeAnalyzer 1476 1498 1.49%   1489.8 1512.8 1.54%   1504 1529 1.66%

Note: The absolute best run was run 5 of the changed solution, but wasn't included in the results:

Found 0 diagnostics in 54970ms
AnalysisWarningAnalyzer:      11
CopyPasteTokenAnalyzer:     1093
FileMetadataAnalyzer:        163
LogAnalyzer:                   8
MetricsAnalyzer:            3495
SymbolReferenceAnalyzer:   46823
TokenTypeAnalyzer:          1467

costin-zaharia-sonarsource
costin-zaharia-sonarsource approved these changes Oct 20, 2023
View reviewed changes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

analyzers/src/SonarAnalyzer.Common/Rules/Utilities/AnalysisWarningAnalyzerBase.cs
@@ -35,11 +35,11 @@ public abstract class AnalysisWarningAnalyzerBase : UtilityAnalyzerBase
protected sealed override void Initialize(SonarAnalysisContext context) =>
context.RegisterCompilationAction(c =>
{
ReadParameters(c);
if (IsAnalyzerEnabled && !RoslynHelper.IsRoslynCfgSupported(MinimalSupportedRoslynVersion)) // MsBuild 15 is bound with Roslyn 2.x, where Roslyn CFG is not available.
var parameter = ReadParameters(c);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to use the plural here.

analyzers/src/SonarAnalyzer.Common/Rules/Utilities/UtilityAnalyzerBase.cs
return new UtilityAnalyzerParameters(
IsAnalyzerEnabled: true,
IgnoreHeaderComments: sonarLintXml.IgnoreHeaderComments(language),
AnalyzeGeneratedCode: sonarLintXml.AnalyzeGeneratedCode(language),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just an idea, it's not required to do the change.

In order to avoid re-creating the parameters we can also provide a default value for analyzeGeneratedCode and read it only when needed.

protected virtual UtilityAnalyzerParameters ReadParameters(SonarCompilationReportingContext context, bool? analyzeGeneratedCode = null)
...
AnalyzeGeneratedCode: analyzeGeneratedCode ?? sonarLintXml.AnalyzeGeneratedCode(language),

If we do this we can move the functionality in another class responsible only for creating these parameters.

In the long run, we might want to unify these analyzers in a single analyzer to be able to reduce some operations like subscribing and reading the parameters multiple times (reducing thread contention since multiple analyzers are waiting for the read to finish) or writing to multiple protobuf files.

@costin-zaharia-sonarsource costin-zaharia-sonarsource merged commit 6ce67a6 into master Oct 20, 2023
25 checks passed
@costin-zaharia-sonarsource costin-zaharia-sonarsource deleted the Martin/UtilityAnalyzerStateless branch October 20, 2023 12:49
@martin-strecker-sonarsource
Copy link
Contributor Author

Peach validation: Code colorization and references are working as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@costin-zaharia-sonarsource costin-zaharia-sonarsource costin-zaharia-sonarsource approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

UtilityAnalyzer: Make UtilityAnalyzerBase stateless by removing properties
2 participants
@martin-strecker-sonarsource @costin-zaharia-sonarsource