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

Make UtilityAnalyzerBase stateless by removing properties #6902

Conversation

martin-strecker-sonarsource
Copy link
Contributor

Pre-condition for #6674

Analyzer should be stateless. This PR removes the properties from UtilityAnalyzerBase and captures them in RegisterCompilationStartAction per compilation.

After this change, EnableConcurrentExecution can be set to true (and also var treeMessages = new List<TMessage>(); needs to be changed to var treeMessages = new ConcurrentStack<TMessage>(); of course).

@martin-strecker-sonarsource martin-strecker-sonarsource added Type: Improvement Area: VB.NET VB.NET rules related issues. Area: C# C# rules related issues. Type: Performance It takes too long. labels Mar 10, 2023
@github-actions github-actions bot added this to In progress in Best Kanban Mar 10, 2023
@martin-strecker-sonarsource martin-strecker-sonarsource marked this pull request as ready for review March 10, 2023 13:20
@martin-strecker-sonarsource martin-strecker-sonarsource removed the Type: Performance It takes too long. label Mar 10, 2023
Comment on lines -33 to 37
protected bool IsAnalyzerEnabled { get; set; }
protected bool IgnoreHeaderComments { get; set; }
protected virtual bool AnalyzeGeneratedCode { get; set; }
protected virtual bool AnalyzeTestProjects => true;
protected string OutPath { get; set; }
protected bool IsTestProject { get; set; }
protected override bool EnableConcurrentExecution => false;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Like with the OutPath property, the mutable state is the root cause that prevents ConcurrentExecution (to be fair: ConcurrentExecution would also work without this change, but it is tough to see why that is the case. With this change, it is evident that it works, and it is no longer possible to break it).

The PR moves the state into its own type, which is now captured in RegisterCompilationStartAction for the compilation. The new Parameters struct is readonly now and passed to all methods that need it to make decisions.

@martin-strecker-sonarsource martin-strecker-sonarsource force-pushed the Martin/UtilityAnalyzer_Stateless branch 2 times, most recently from f6f146e to 9cd9199 Compare March 14, 2023 10:27
@pavel-mikula-sonarsource pavel-mikula-sonarsource moved this from In progress to To do in Best Kanban Mar 29, 2023
@andrei-epure-sonarsource
Copy link
Contributor

This is slowly getting out of sync.

I suggest to close this and link it to the issue so the ideas can be reused once it will be actively worked on.

@gregory-paidis-sonarsource
Copy link
Contributor

There is also ConcurrentQueue and ConcurrentBag, and the implementation yields different performance based on the scenario you are targeting. For example, one of these collections might be faster in the context of many-threads-writing and one-thread-reading, but another must be faster if you have a lot of thread-writing/reading contextion. So I think we should:

  • Understand what kind of concurrency this analyzer needs
  • Benchmark the collections in that setting (e.g. make a testbed were the threads are writing rapid-fire, if that's what we expect to happen here)
  • Pick the best one

Concurrent collections: click here

@mary-georgiou-sonarsource mary-georgiou-sonarsource moved this from To do to In progress in Best Kanban May 15, 2023
@mary-georgiou-sonarsource mary-georgiou-sonarsource force-pushed the Martin/UtilityAnalyzer_Stateless branch 2 times, most recently from 350813c to 1283458 Compare May 17, 2023 13:18
@github-actions github-actions bot moved this from In progress to Review in progress in Best Kanban May 17, 2023
@mary-georgiou-sonarsource mary-georgiou-sonarsource changed the base branch from master to feature/utility-analyzers May 17, 2023 15:20
@mary-georgiou-sonarsource
Copy link
Contributor

We need to open a cleanup issue for this PR because the CreateMessage method in the base UtilityAnalyzer class is getting the parameters as input but they are only used by one derived class.

@sonarcloud
Copy link

sonarcloud bot commented May 17, 2023

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

@sonarcloud
Copy link

sonarcloud bot commented May 17, 2023

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

Copy link
Contributor

@csaba-sagi-sonarsource csaba-sagi-sonarsource left a comment

Choose a reason for hiding this comment

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

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: C# C# rules related issues. Area: VB.NET VB.NET rules related issues.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants