Skip to content

[Runtime Async] Reuse ValueTaskSourceNotifier instance#126622

Open
VSadov wants to merge 3 commits intodotnet:mainfrom
VSadov:cacheNotifier
Open

[Runtime Async] Reuse ValueTaskSourceNotifier instance#126622
VSadov wants to merge 3 commits intodotnet:mainfrom
VSadov:cacheNotifier

Conversation

@VSadov
Copy link
Copy Markdown
Member

@VSadov VSadov commented Apr 7, 2026

This avoids allocation on every time we need to suspend on a ValueTaskSource.

Copilot AI review requested due to automatic review settings April 7, 2026 23:40
@dotnet-policy-service
Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @agocke
See info in area-owners.md if you want to be subscribed.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Reduces per-await allocations in runtime-async by reusing a per-thread ValueTaskSource notifier instead of allocating a new wrapper object each time a ValueTask suspends on an IValueTaskSource.

Changes:

  • Replace per-call ValueTaskSourceNotifier allocations with a [ThreadStatic] cached ValueTaskSourceNotifier instance.
  • Introduce a reusable notifier that stores the source, token, and an OnCompleted dispatcher delegate.
  • Update CoreCLR runtime-async suspension state to store the concrete ValueTaskSourceNotifier type (instead of a removed interface).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/ValueTask.cs Adds reusable ValueTaskSourceNotifier and updates AsTaskOrNotifier to return a cached notifier for IValueTaskSource-backed ValueTasks.
src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/AsyncHelpers.CoreCLR.cs Updates runtime-async suspension bookkeeping to reference/cast the new ValueTaskSourceNotifier type.

Copilot AI review requested due to automatic review settings April 8, 2026 13:02
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

GetTaskForValueTaskSource(Unsafe.As<IValueTaskSource>(obj));
}

// A class to not have a static initializer directly on ValueTask
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Roslyn will take care of the caching and other details for you if you just use lambda.

Are we doing the manual caching to improve the perf characteristics in some way? What is the perf characteristics that the manual caching is improving?

Copy link
Copy Markdown
Member Author

@VSadov VSadov Apr 8, 2026

Choose a reason for hiding this comment

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

I think this is where I'd need some advice. I'd like to not pay too much for this cache.

I have two concerns:

  • ValueTask<T> is a very common type and will be instantiated over all kind of T. But the number of ValueTask sources in the process could be very few. VTS is typically used to interact with things that actually can source asynchrony (like sockets, pipes, ManualResetEventSlim ...).
    I'd like to be sure that we do not initialize this lambda as soon as we see another instantiation of ValueTask<T>.
    I assume placing the lambda instance directly on ValueTask<T> might not be a good idea.

  • I'd like the static readonly to be a JIT-time constant, if possible. I am not sure what Roslyn produces would fit the requirements. Assuming that what I want is possible at all.. - I am not sure if reference-typed readonly statics can actually be JIT-constants in a generic class. The requirements for that were changing over time.

I just wanted to be more explicit of how this is initialized in case it needs to be changed or moved around to fit the criteria.

{
internal static readonly Action<object, Action<object?>, object?, short, ValueTaskSourceOnCompletedFlags> s_action =
static (object obj, Action<object?> continuation, object? state, short token, ValueTaskSourceOnCompletedFlags flags) =>
Unsafe.As<IValueTaskSource>(obj).OnCompleted(continuation, state, token, flags);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Given that we depend on Unsafe.As, would it be even better to use a function pointer instead of a delegate?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I did not realize that we actually could use a function pointer here. Can we get a pointer to an interfce method?

Copy link
Copy Markdown
Member Author

@VSadov VSadov Apr 8, 2026

Choose a reason for hiding this comment

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

The annoying part is that all this deal with the lambda is because IValueTaskSource<T> does not derive from IValueTaskSource. It could have inherited OnCompleted, since OnCompleted does not need to know T.
Then the notifier could just be a tuple of {source, token}.
The lambda/indirection is just a way to capture T without making the notifier a generic type.

I wonder if with a pointer we could avoid specializing the notifier to T somehow?
OnCompleted is likely in the same slot in both interfaces (and all instantiations), although not sure if it would be safe to rely on that.

Copy link
Copy Markdown
Member

@jakobbotsch jakobbotsch Apr 8, 2026

Choose a reason for hiding this comment

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

I have a branch where I started on #119842. It should allow solving this without requiring the extra ValueTaskSourceNotifier or IValueTaskSourceNotifier, via function pointers to instantiations that get the ValueTask from the continuation. You can see the idea here for normal awaiters, but the same approach should work for ValueTask:

https://github.com/jakobbotsch/runtime/blob/3e90970b894fa401894d63271b60baa6b7d50774/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/AsyncHelpers.cs#L249-L263

https://github.com/jakobbotsch/runtime/blob/3e90970b894fa401894d63271b60baa6b7d50774/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/AsyncHelpers.cs#L297-L303

There is some cost to create instantiations even here... Ideally with the ability to create lightweight OnCompleted-funclets (a la #121013) we wouldn't even need that.

Copy link
Copy Markdown
Member

@jkotas jkotas Apr 8, 2026

Choose a reason for hiding this comment

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

Can we get a pointer to an interfce method?

That's complicated. I would get a pointer to a (specialized) static method that calls the interface method you need.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I have a branch where I started on #119842. It should allow solving this without requiring the extra ValueTaskSourceNotifier or IValueTaskSourceNotifier, via function pointers to instantiations that get the ValueTask from the continuation. You can see the idea here for normal awaiters, but the same approach should work for ValueTask:

https://github.com/jakobbotsch/runtime/blob/3e90970b894fa401894d63271b60baa6b7d50774/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/AsyncHelpers.cs#L249-L263

https://github.com/jakobbotsch/runtime/blob/3e90970b894fa401894d63271b60baa6b7d50774/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/AsyncHelpers.cs#L297-L303

There is some cost to create instantiations even here... Ideally with the ability to create lightweight OnCompleted-funclets (a la #121013) we wouldn't even need that.

I think ValueTaskSource will still require some special casing. By default UnsafeOnCompleted on ValueTask/Awaiter would pass ValueTaskSourceOnCompletedFlags.UseSchedulingContext to the source. In our case that could be incorrect as the context at the time of IsCompleted (the actual scheduling context) could be different from the context at the time we call UnsafeOnCompleted - after popping the stack. We may need to compute this flag from the head continuation, similarly to how we do that right now, and pass the computed flag to the OnCompleted on the source.

That does not prevent the proposed mechanism from working, just that VTS will require some special handling.
The head continuation indeed contains all the information that we store in ValueTaskSourceNotifier, we just do not have a good way to extract it.

Once the suggested mechanism works, ValueTaskSourceNotifier would become dead code and could be deleted.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Can we get a pointer to an interfce method?

That's complicated. I would get a pointer to a (specialized) static method that calls the interface method you need.

Thanks! That does look much simpler.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants