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

Allocations: Unroll AdditionalFile extension method #7440

Closed
sharwell opened this issue Jun 14, 2023 · 4 comments · Fixed by #7652
Closed

Allocations: Unroll AdditionalFile extension method #7440

sharwell opened this issue Jun 14, 2023 · 4 comments · Fixed by #7652
Assignees
Labels
Type: Performance It takes too long.
Milestone

Comments

@sharwell
Copy link

Accounts for 5037MB allocations in https://developercommunity.visualstudio.com/t/Visual-Studio-2022:-High-CPU-usage-by-Se/10378039

options.AdditionalFiles.FirstOrDefault(x => x.Path is not null && Path.GetFileName(x.Path).Equals(fileName, StringComparison.OrdinalIgnoreCase));

@pavel-mikula-sonarsource
Copy link
Contributor

This is a great catch! Thank you for reporting it.

Do you know from user logs how many additional files user had?
We could add case insensitive .EndsWith pre-check, but if he has 1 or 2 files, it will not fix the problem.
We could not use the API and do our own string-only based check too.

I have a feeling that we call that method way too often and fine-tuning the implementation could have no impact. What do you think of caching the value and using AnalyzerOptions as a key? In practice, the cache would never get purged (unless we purge it above a certain size). At the same time, the naive idea is that there shouldn't be too many different sets of AdditionalFiles during the memory lifetime of the analyzer. Or is there? As we would prevent GC from collecting the unique instances.

@sharwell
Copy link
Author

It should be possible to slice the string after the final \ or / and compare the resulting ReadOnlySpan<char>, instead of allocating a substring.

@martin-strecker-sonarsource
Copy link
Contributor

martin-strecker-sonarsource commented Jul 14, 2023

The measurement of a dotnet build of akka with dotnet sonarscanner begin of a SQ EE instance and "Sonar Way" profile revealed that Path.GetFileName (allocates strings) and AdditionalFiles.FirstOrDefault (allocates a delegate and a capture class) are the main source of allocations by far:
image

These two allocations contribute 1GB for the string + 700MB for the delegate + 260 MB for the capture class out of 11,3 GB total. We are also responsible for SymbolDisplay allocations shown above. The driver is here TypeHelper.IsMatch in Sonar-Security:
https://github.com/SonarSource/sonar-security/blob/b33150486bec9c1d61fd30b01a03c388a845b0c7/frontend/csharp/src/SonarAnalyzer.Security/Helpers/TypeHelper.cs#L31-L32

Note: I stopped capturing in the middle of the build after 30 minutes.

@martin-strecker-sonarsource
Copy link
Contributor

martin-strecker-sonarsource commented Jul 14, 2023

We could add case insensitive .EndsWith pre-check, but if he has 1 or 2 files, it will not fix the problem.

There is an overload that takes a StringComparison so we should be fine. There are just two things to do:

  • Unroll FirstOrDefault to a foreach to avoid the delegate and capture class allocations (1GB saving).
  • Use EndsWith(StringComparison.OrdinalIgnoreCase) instead of Path.GetFileName. Another 1GB saving.

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