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

Improve S5443 performance: Reuse compiled Regex #8183

Merged
merged 9 commits into from
Nov 23, 2023
Merged

Improve S5443 performance: Reuse compiled Regex #8183

merged 9 commits into from
Nov 23, 2023

Conversation

martin-strecker-sonarsource
Copy link
Contributor

see also #8181
Strips of another 4,5 seconds (or 0,08%) by caching the JITed regex in IsSensitiveDirectoryUsage:

JIT

Before:
image

After:
image

Overall

Before
image

After
image

@martin-strecker-sonarsource martin-strecker-sonarsource added Area: VB.NET VB.NET rules related issues. Area: C# C# rules related issues. Type: Performance It takes too long. labels Oct 11, 2023
@martin-strecker-sonarsource martin-strecker-sonarsource mentioned this pull request Oct 11, 2023
Merged
@sonarcloud
Copy link

sonarcloud bot commented Oct 11, 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 Oct 11, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot E 4 Security Hotspots
Code Smell A 1 Code Smell

100.0% 100.0% Coverage
0.0% 0.0% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Nov 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 SonarCloud
Copy link

sonarcloud bot commented Nov 17, 2023 •
edited
Loading

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

Regarding the code smells/security hotspots:

pavel-mikula-sonarsource
pavel-mikula-sonarsource reviewed Nov 21, 2023
View reviewed changes
analyzers/src/SonarAnalyzer.Common/Rules/Hotspots/PubliclyWritableDirectoriesBase.cs
var insecureEnvironmentVariables = new[] { "tmp", "temp", "tmpdir" };
InsecureEnvironmentVariables = insecureEnvironmentVariables;
UserProfile = new("""^%USERPROFILE%[\\\/]AppData[\\\/]Local[\\\/]Temp""", WindowsAndUnixOptions);
LinuxDirectories = new($@"^({LinuxDirs().JoinStr("|", Regex.Escape)})(\/|$)", RegexOptions.Compiled);
Copy link
Contributor

Choose a reason for hiding this comment

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

I would question the use of RegexOptions.Compile in general in this rule. The cost of creating in memory assembly with emitting instructions at runtime to evaluate regex is extremely high, often not worth the benefit of the speed bump.

These regexes are generally straight-forward with not so much backtracking.

Did you try to measure the impact of the Compiled thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not what the PR is about. The change shows significant improvements. Whether "Compiled" is bringing gains needs to be decided for all Regexes and a wide variety of projects. Create an issue, if you want us to look into it. We also have to see this in the light of the internal Regex cache: https://learn.microsoft.com/en-us/dotnet/api/system.text.regularexpressions.regex.cachesize?view=net-7.0#remarks

@antonioaversa antonioaversa changed the title PubliclyWritableDirectories: Reuse compiled Regex Improve S5443 performance: Reuse compiled Regex Nov 23, 2023
@antonioaversa antonioaversa modified the milestone: 9.14 Nov 23, 2023
sebastien-marichal
sebastien-marichal approved these changes Nov 23, 2023
View reviewed changes
Copy link
Contributor

@sebastien-marichal sebastien-marichal left a comment

Choose a reason for hiding this comment

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

LGTM!

@martin-strecker-sonarsource martin-strecker-sonarsource merged commit a5a2023 into master Nov 23, 2023
39 checks passed
@martin-strecker-sonarsource martin-strecker-sonarsource deleted the Martin/PubliclyWritableDirectoriesBase_Static branch November 23, 2023 16:40
@antonioaversa antonioaversa added this to the 9.15 milestone Nov 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pavel-mikula-sonarsource pavel-mikula-sonarsource pavel-mikula-sonarsource left review comments

@sebastien-marichal sebastien-marichal sebastien-marichal approved these changes

Assignees

@martin-strecker-sonarsource martin-strecker-sonarsource

Labels
Area: C# C# rules related issues. Area: VB.NET VB.NET rules related issues. Type: Performance It takes too long.
Projects
None yet
Milestone
9.15
Development

Successfully merging this pull request may close these issues.
4 participants
@martin-strecker-sonarsource @pavel-mikula-sonarsource @sebastien-marichal @antonioaversa