Reduce closure allocations associated with AsyncLazy usage #72449
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was noticed while looking at speedometer profiles. This class is used in several high traffic areas, and the current AsyncLazy design doesn't allow usage in a way that prevents closure allocations. This PR adds the "arg" usage pattern to AsyncLazy such that funcs passed to AsyncLazy can take advantage of the arg infrastructure to avoid closure allocations. This is done by changing AsyncLazy<T> to be an abstract class with two derivations: 1) AsyncLazyWithoutArg 2) AsyncLazyWithArg The former is the same behavior as the prior implementation of AsyncLazy, holding onto the same compute functions as before. The latter holds onto an extra piece of data and compute functions which take that additional argument. I did talk with Cyrus about an alternate approach where the base class held on to data and there was a derived type which masked that for users that didn't wish to use that mechanism. That approach had the advantage of not having the virtual calls that the approach outlined in this PR has, however, the type declarations were a bit clunky in that they needed to specify the data type. Anothed disadvantage of that approach was that all users of AsyncLazy would pay the cost of having some additional data, even if they weren't using that feature. The first commit in this PR is the guts of the change, pretty much solely contained within the AsyncLazy`1.cs file. The second commit will contain the changes for the usages I found that were allocating closures.
…osure allocations
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
|
Added @sharwell and @CyrusNajmabadi and started in draft mode |
|
/azp run |
|
Azure Pipelines successfully started running 4 pipeline(s). |
sharwell
reviewed
Mar 8, 2024
src/EditorFeatures/Core/RenameTracking/RenameTrackingTaggerProvider.RenameTrackingCommitter.cs
Outdated
Show resolved
Hide resolved
sharwell
reviewed
Mar 8, 2024
src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Utilities/AsyncLazy`1.cs
Outdated
Show resolved
Hide resolved
src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Utilities/AsyncLazy`1.cs
Outdated
Show resolved
Hide resolved
src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Utilities/AsyncLazy`1.cs
Outdated
Show resolved
Hide resolved
src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Utilities/AsyncLazy`1.cs
Outdated
Show resolved
Hide resolved
src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Utilities/AsyncLazy`1.cs
Outdated
Show resolved
Hide resolved
src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Utilities/AsyncLazy`1.cs
Outdated
Show resolved
Hide resolved
src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Utilities/AsyncLazy`1.cs
Outdated
Show resolved
Hide resolved
src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Utilities/AsyncLazy`1.cs
Outdated
Show resolved
Hide resolved
2) Fix test failure 3) Fix formatting request 4) Change AsyncLazy ctors to protected 5) Rename ResetComputeFunctions to ClearComputeFunctions
ToddGrun
commented
Mar 8, 2024
src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Utilities/AsyncLazyWithArg`2.cs
Outdated
Show resolved
Hide resolved
sharwell
reviewed
Mar 11, 2024
| var compilationStateCapture = compilationState; | ||
| Interlocked.CompareExchange(ref _lazyDependentVersion, AsyncLazy.Create( | ||
| c => ComputeDependentVersionAsync(compilationStateCapture, c)), null); | ||
| Interlocked.CompareExchange( |
There was a problem hiding this comment.
💡 InterlockedOperations.Initialize
…ontain the data, and the version that doesn't need data stored built on that.
sharwell
reviewed
Mar 11, 2024
src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Utilities/AsyncLazy.cs
Outdated
Show resolved
Hide resolved
src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Utilities/AsyncLazy.cs
Outdated
Show resolved
Hide resolved
src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Utilities/AsyncLazy.cs
Outdated
Show resolved
Hide resolved
…d out) Get rid of AsyncLAzyWithoutArgs
src/Interactive/Host/Microsoft.CodeAnalysis.InteractiveHost.csproj
Outdated
Show resolved
Hide resolved
src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Utilities/AsyncLazy`1.cs
Outdated
Show resolved
Hide resolved
src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Utilities/AsyncLazy`1.cs
Show resolved
Hide resolved
src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Utilities/AsyncLazy`1.cs
Outdated
Show resolved
Hide resolved
CyrusNajmabadi
approved these changes
Mar 11, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reduce closure allocations associated with AsyncLazy usage
This was noticed while looking at speedometer profiles. This class is used in several high traffic areas, and the current AsyncLazy design doesn't allow usage in a way that prevents closure allocations. This PR adds the "arg" usage pattern to AsyncLazy such that funcs passed to AsyncLazy can take advantage of the arg infrastructure to avoid closure allocations.
This is done by changing AsyncLazy to be an abstract class with two derivations:
The former is the same behavior as the prior implementation of AsyncLazy, holding onto the same compute functions as before. The latter holds onto an extra piece of data and compute functions which take that additional argument.
I did talk with Cyrus about an alternate approach where the base class held on to data and there was a derived type which masked that for users that didn't wish to use that mechanism. That approach had the advantage of not having the virtual calls that the approach outlined in this PR has, however, the type declarations were a bit clunky in that they needed to specify the data type. Anothed disadvantage of that approach was that all users of AsyncLazy would pay the cost of having some additional data, even if they weren't using that feature.
The first commit in this PR is the guts of the change, pretty much solely contained within the AsyncLazy`1.cs file. The second commit will contain the changes for the usages I found that were allocating closures.