Reduce allocations in SymbolDeclaredCompilationEvent #74250
Conversation
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.
dotnet-issue-labeler
bot
added
Area-Analyzers
untriaged
| { | ||
| if (!_lazyCachedDeclaringReferences.HasValue) | ||
| { | ||
| _lazyCachedDeclaringReferences = Symbol.DeclaringSyntaxReferences; |
There was a problem hiding this comment.
There was a problem hiding this comment.
@ToddGrun Specifically, you're going to want to use InterlockedOperations.Initialize here with a static lambda callback.
There was a problem hiding this comment.
I believe the full form would be:
return InterlockedOperations.Initialize(
ref _lazyCachedDeclaringReferences,
static symbol => symbol.DeclaringSyntaxReferences,
Symbol);There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Went over the sheared write issue in more detail offline.
There was a problem hiding this comment.
Fred stopped by and explained what I was missing. Not the first time a torn-write has slipped past me.
|
Done with review pass (commit 1) |
| { | ||
| if (!_lazyCachedDeclaringReferences.HasValue) | ||
| { | ||
| _lazyCachedDeclaringReferences = Symbol.DeclaringSyntaxReferences; |
There was a problem hiding this comment.
@ToddGrun Specifically, you're going to want to use InterlockedOperations.Initialize here with a static lambda callback.
| { | ||
| if (!_lazyCachedDeclaringReferences.HasValue) | ||
| { | ||
| _lazyCachedDeclaringReferences = Symbol.DeclaringSyntaxReferences; |
There was a problem hiding this comment.
I believe the full form would be:
return InterlockedOperations.Initialize(
ref _lazyCachedDeclaringReferences,
static symbol => symbol.DeclaringSyntaxReferences,
Symbol);…asn't been initialized
|
@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) |
…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 ...
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.