-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Determine what to do with "async ValueTask" caching #13633
Comments
cc: @benaadams, @mgravell, @mjsabby |
If the choice ends up as
Would there be any disadvantage to keeping the parameter |
Mainly a little more code to execute to read the environment variable. And we'd want to ensure it was clearly "unsupported" to enable us to change the algorithms employed in the future, assuming it was still relevant anyway by the time we shipped. |
I'll have a go at getting the PR working locally. Just to mention another option re making the on/off decision; potentially this could also be done in code via the builder; the current API for picking the builder is awkward, but: (pure imaginary code): [Pooled]
async ValueTask<...> Foo() {...} This does, however, obviously hugely limit where it would apply. For better or for worse. |
I see the PR is merged; presumably that means I can use the nightly for this; sorry if this is a silly question, but is that the 5.0.x? the 3.1.x? neither? both? |
The two options I've explored here are:
And as you say, it hugely limits applicability. If we decide that enabling this for all Regardless, yeah, it's another option. Thanks.
5.0.x. I just checked the latest from https://dotnetcli.blob.core.windows.net/dotnet/Sdk/master/dotnet-sdk-latest-win-x64.exe and it's not there yet, but I expect it should be soon. |
@mgravell, the latest 5.0.x at https://dotnetcli.blob.core.windows.net/dotnet/Sdk/master/dotnet-sdk-latest-win-x64.exe (https://github.com/dotnet/core-sdk/blob/master/README.md#installers-and-binaries) now contains these changes. |
finally had some time to try and look at this, but it looks like the installer is internally inconsistent at the moment - the SDK is installing
reporting that |
Thanks, @mgravell. Did you configure a nuget.config ala the example from https://github.com/dotnet/core-sdk/blob/master/README.md#installers-and-binaries? |
Initial test (code) for HttpClient with http; its quite light on async depth, Tasks aren't the dominant form of allocation and they can't be completely removed (public api); however it does make a difference. On of the things I like about this pooling is the lifetime of async statemachines is very indeterminate compared to a lot of other allocations. They aren't long life in terms of cpu, but are in terms of elapsed time which means they end up moving to higher GC generations perhaps more than the "sync" style of async programming would suggest.
"configProperties": {
"System.GC.Server": true,
"System.GC.HeapHardLimit": 20971520
} |
I wasn't able to get my previous bits updated to .NET 5 (too many moving parts), however: here's an update that I did using RESP using @davidfowl's "Bedrock" pieces (with some minor tweaks to make it run on the .NET 5 version of the Kestrel API - just the Baseline (environment variable
|
Thanks, @mgravell! One of the issues here is that allocations are for the most part a secondary indication of perf, in that the hope is that by reducing allocation, you improve throughput. But here to reduce allocation, we're pooling, which has its own costs. So a decrease in allocations that doesn't improve throughput at all isn't necessarily a win, especially if it brings with it other downsides. It'd be great if you can try out real workloads when you get a chance to see if there's any meaningful gain. |
Does the working set get better as a result? You should be able to run dotnet counters before and after to get a glimpse of what the metics look like. |
(Working set is good to look at it, but working set improvements could also instead suggest tweaks to GC configuration, as such pooling should primarily help working set if the GC has determined it's not worth doing a collection yet. Even so, as David suggests, it's a good data point.) |
Thought I'd add a link to your blogpost here @stephentoub: https://devblogs.microsoft.com/dotnet/async-valuetask-pooling-in-net-5/ |
Thanks, @YairHalberstadt. |
Coming from the blog post, this is really really cool :) I haven't done any measurements but I can see this being beneficial for some places in our code, but I'd be worried to enable it globally. Perhaps it could be enabled/disabled with an attribute, that way you could choose the scope (method, class,assembly) and then down the line if we want it do be the default it could be a default msbuild parameter that works the same as the other assembly info attributes |
Thanks, @aL3891. Technically it could be done in a manner you suggest, and I considered something like that. The main problem I hit is it adds a non-trivial amount of expensive reflection on first use of every async method (whether it uses the attribute or not), and that can contribute non-trivially to things like start-up time costs. If we could figure out how to avoid that, it might be a very reasonable opt-in path to consider. |
I was thinking the attribute could be checked/used at compile time similar to CallerMemberNameAttribute, burning it into the generated code (atleast that's how I think that attribute works, apologies if I'm incorrect) Still, a very cool feature for micro service apps with very chatty APIs Perhaps another option is adding a new PooledValueTask that is essentially just a marker for saying this task uses pooling, that would also sidestep backcompat issues, but that's also another type to keep track of :) |
For .NET 5, the plan is to leave the code for this in but off by default. |
This is experimental feature that adds about 1.7kB in binary footprint per method returning ValueTask. Ifdef it out for native AOT until we figure out what to do with it. See dotnet/runtime#13633.
This is experimental feature that adds about 1.7kB in binary footprint per method returning ValueTask. Ifdef it out for native AOT until we figure out what to do with it. See dotnet/runtime#13633.
PR dotnet/coreclr#26310 added in an experimental feature, guarded by an environment variable feature flag, that uses cached objects behind the scenes to implement
async ValueTask
andasync ValueTask<T>
methods. For .NET 5, we need to decide how to proceed with this feature:We need more data to decide how to proceed, and in particular:
If we decide to keep it, and especially if we decide to keep it as opt-out or always-on, we also need to validate diagnostics, in particular tracing to ensure we're not regressing anything impactful.
Functionally, there's also a behavior change associated with it. While
ValueTask
/ValueTask<T>
are documented to only be used in very specific ways, the fact that instances produced byasync
methods before .NET 5 were backed byTask
meant that you could get away with violating the contract. This change generally ends up requiring that the contract be enforced. We would want to ensure we had good analyzers in place to help catch any misuse: https://github.com/dotnet/corefx/issues/40739.CALL TO ACTION:
Please try out the bits with your app and share your results: throughput, memory load, etc.
To enable the feature, set the
DOTNET_SYSTEM_THREADING_POOLASYNCVALUETASKS
environment variable to either1
ortrue
.This will only impact
async ValueTask
andasync ValueTask<T>
methods, so you may also want to look through your code to switch some internal and privateasync Task
/async Task<T>
methods to instead useValueTask
/ValueTask<T>
... just make sure to only do so when the consumers abide by the rules, which is that an instance must only be consumed once: if callers are doing anything other than directly awaiting the result of the method, be careful.The text was updated successfully, but these errors were encountered: