This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
Change some internal async Task methods to be async ValueTask #40527
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
|
Is unhappy? |
stephentoub
force-pushed
the
tasktovaluetask
branch
2 times, most recently
from
7e77d37 to
faf9bca
Compare
Fixed. The System.Net.Security UnitTests build in some product source along with a fake that needed to be tweaked. |
stephentoub
changed the title
stephentoub
added
the
* NO MERGE *
benaadams
approved these changes
Aug 27, 2019
stephentoub
force-pushed
the
tasktovaluetask
branch
from
faf9bca to
96ebf2e
Compare
We have some internal and private `async Task` methods that are only ever `await`ed. Today there's no benefit to making them `async ValueTask` methods, so we've kept them as `async Task`. However, if we end up changing the implementation of `async ValueTask` to pool underlying objects, there becomes a potential benefit to using `async ValueTask` for these instead of `async Task`. This PR changes those in a variety of libraries where we care more about performance and allocations. There are likely a few more methods we'd want to convert based on profiling, but I believe this represents the bulk of them.
stephentoub
force-pushed
the
tasktovaluetask
branch
from
96ebf2e to
feb040e
Compare
stephentoub
removed
the
* NO MERGE *
|
I'd like to go ahead with this PR now. It doesn't make any of the macro scenarios I looked at worse, and having it in will enable better validating the corresponding runtime change's impact. |
picenka21
pushed a commit
to picenka21/runtime
that referenced
this pull request
Feb 18, 2022
…/corefx#40527) We have some internal and private `async Task` methods that are only ever `await`ed. Today there's no benefit to making them `async ValueTask` methods, so we've kept them as `async Task`. However, if we end up changing the implementation of `async ValueTask` to pool underlying objects, there becomes a potential benefit to using `async ValueTask` for these instead of `async Task`. This PR changes those in a variety of libraries where we care more about performance and allocations. There are likely a few more methods we'd want to convert based on profiling, but I believe this represents the bulk of them. Commit migrated from dotnet/corefx@97d9fb9
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Requires dotnet/coreclr#26310.
We have some internal and private
async Taskmethods that are only everawaited (or returned out to callers via public API). Today there's no benefit and a small downside to making themasync ValueTaskmethods, so we've kept them asasync Task. However, if we end up changing the implementation ofasync ValueTaskto pool underlying objects, there becomes a potential benefit to usingasync ValueTaskfor these instead ofasync Task. This PR changes those in a variety of libraries where we care more about performance and allocations. There are likely a few more methods we'd want to convert based on profiling, but I believe this represents the bulk of them.This should only be merged if dotnet/coreclr#26310 is merged and profiling determines that these changes are worthwhile.
cc: @benaadams