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

Reduce allocations in SymbolDeclaredCompilationEvent #74250

Conversation

ToddGrun
Copy link
Contributor

@ToddGrun ToddGrun commented Jul 3, 2024

The SymbolDeclaredCompilationEvent ctor is showing up as 0.5% of allocations in vbcscompiler.exe in a customer profile I'm looking at. All these costs are attributable to the use of Lazy in this class. Instead, just use a nullable field, and accept the fact that during extremely high contention, it is possible for an ImmutableArray to be created multiple times.

image

The SymbolDeclaredCompilationEvent ctor is showing up as 0.5% of allocations in vbcscompiler.exe in a customer profile I'm looking at. All these costs are attributable to the use of Lazy in this class. Instead, just use a nullable field, and accept the fact that during extremely high contention, it is possible for an ImmutableArray to be created multiple times.
@ToddGrun ToddGrun requested a review from a team as a code owner July 3, 2024 03:01
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Analyzers untriaged Issues and PRs which have not yet been triaged by a lead labels Jul 3, 2024
jaredpar
jaredpar previously approved these changes Jul 3, 2024
{
if (!_lazyCachedDeclaringReferences.HasValue)
{
_lazyCachedDeclaringReferences = Symbol.DeclaringSyntaxReferences;
Copy link
Contributor

@AlekseyTs AlekseyTs Jul 3, 2024

Choose a reason for hiding this comment

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

_lazyCachedDeclaringReferences = Symbol.DeclaringSyntaxReferences;

This assignment is not atomic. Why we are using a nullable type at all? Is using default(ImmutableArray<SyntaxReference>) as an initial value and using interlocked initialize going to work? #Closed

Copy link
Member

Choose a reason for hiding this comment

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

@ToddGrun Specifically, you're going to want to use InterlockedOperations.Initialize here with a static lambda callback.

Copy link
Member

Choose a reason for hiding this comment

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

I believe the full form would be:

return InterlockedOperations.Initialize(
  ref _lazyCachedDeclaringReferences,
  static symbol => symbol.DeclaringSyntaxReferences,
  Symbol);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I've gone ahead with your and Sam's suggestion. I did consider this originally, but wasn't sure whether the Symbol.DeclaringSyntaxReferences could return a default immutable array. But, if that's not a concern, then this is definitely better.

Copy link
Member

Choose a reason for hiding this comment

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

Even if it could, we would still need to use some kind of sentinel array to compare with. Nullable<T> cannot be used in this scenario, period; you could have two threads race and observe a sheared Nullable<ImmutableArray> value in thread 2, such that the write to Nullable.hasValue is true, but Nullable.value has not been written yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I wrote in the original description, I didn't consider multiple assignments during periods of high contention as being a deal breaker. However, I'm more than glad to have made the change such that the value won't be calculated twice as that seems to be the desired consensus.

Copy link
Member

Choose a reason for hiding this comment

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

Went over the sheared write issue in more detail offline.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fred stopped by and explained what I was missing. Not the first time a torn-write has slipped past me.

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 1)

@jaredpar jaredpar dismissed their stale review July 3, 2024 15:28

Feedback

{
if (!_lazyCachedDeclaringReferences.HasValue)
{
_lazyCachedDeclaringReferences = Symbol.DeclaringSyntaxReferences;
Copy link
Member

Choose a reason for hiding this comment

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

@ToddGrun Specifically, you're going to want to use InterlockedOperations.Initialize here with a static lambda callback.

{
if (!_lazyCachedDeclaringReferences.HasValue)
{
_lazyCachedDeclaringReferences = Symbol.DeclaringSyntaxReferences;
Copy link
Member

Choose a reason for hiding this comment

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

I believe the full form would be:

return InterlockedOperations.Initialize(
  ref _lazyCachedDeclaringReferences,
  static symbol => symbol.DeclaringSyntaxReferences,
  Symbol);

@ToddGrun
Copy link
Contributor Author

ToddGrun commented Jul 3, 2024

@sharwell -- Mostly matched your suggested change (I didn't pass in the Symbol directly, as there's no need to call into that property in the already initialized case)

Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (commit 2)

@ToddGrun ToddGrun merged commit 6e83620 into dotnet:main Jul 5, 2024
24 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Jul 5, 2024
333fred added a commit to 333fred/roslyn that referenced this pull request Jul 9, 2024
…solution-priority

* upstream/main: (184 commits)
  Disable BuildWithNetFrameworkHostedCompiler (dotnet#74299)
  Avoid using constants for large string literals (dotnet#74305)
  Adjust lowering of a string interpolation in an expression lambda to not use expanded non-array `params` collection in Format/Create calls. (dotnet#74274)
  Consolidate test Span sources (dotnet#74281)
  Allow Document.FilePath to be set to null (dotnet#74290)
  Update Directory.Build.rsp
  Remove fallback options from IdeAnalyzerOptions (dotnet#74235)
  Fix msbuild issue
  Improve parser recovery around nullable types in patterns (dotnet#72805)
  Syntax formatting options (dotnet#74223)
  Localized file check-in by OneLocBuild Task: Build definition ID 327: Build ID 2490585 (dotnet#74287)
  fix (dotnet#74276)
  Remove more
  fix (dotnet#74237)
  Fix scenario where lightbulbs weren't being displayed
  Reduce closures allocated during invocation of CapturedSymbolReplacement.Replacement (dotnet#74258)
  Reduce allocations in SymbolDeclaredCompilationEvent (dotnet#74250)
  remove type that now serves no purpose
  Remove uncalled method
  Remove more unused code
  ...
@RikkiGibson RikkiGibson modified the milestones: Next, 17.12 P1 Jul 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Analyzers untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants