Skip to content
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

After ValueTask with IValueTaskSource, there r a lot more we want! #26896

Closed
juepiezhongren opened this issue Jul 22, 2018 · 20 comments
Closed
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Threading.Tasks
Milestone

Comments

@juepiezhongren
Copy link

https://github.com/dotnet/corefx/issues/new is such a perfect solution! But optimization is just starting.

  1. ValueTask.Delay() seems to be very much needed;
  2. Task.WhenAny and Task.WhenAll are required to be optimised for ValueTask;
  3. Object Pooling 'heap 'with manua-gc api is required;
  4. considering async's 5 main senarios(IO; Net; Timer; long Computing; UI;)where vTask with IValueTaskSource will definitely bring a lot of benefits;
  5. TaskCompletionSource is required to be reusable and pool-cached
@marksmeltzer
Copy link

As I asked over here (https://github.com/dotnet/corefx/issues/27445#issuecomment-406839279), what specific benefit would you be looking to get out of ValueTask.Delay()? I cannot think of any real-world benefit, and I discussed why in my other comment.

@juepiezhongren
Copy link
Author

@marksmeltzer for accuracy, Task.Delay(1) is always the same as Task.Delay(10)

@marksmeltzer
Copy link

marksmeltzer commented Jul 22, 2018

@juepiezhongren, that's because the delay mechanism uses the system clock (Environment.TickCount), which currently only has a reliability of 10-16 milliseconds. See here: https://msdn.microsoft.com/en-us/library/system.environment.tickcount(v=vs.110).aspx

As such, Task.Delay(1) through Task.Delay(30) will typically completely in roughly the same time.

@juepiezhongren
Copy link
Author

juepiezhongren commented Jul 22, 2018

besides, i use delay() a lot, to make it allocate-free does serve me well
@marksmeltzer thx for ur information

@marksmeltzer
Copy link

marksmeltzer commented Jul 22, 2018

How could it possibly be allocation free? The delay will need to call back to your completion handler delegate, which means it will need to allocate the completion handler delegate.

@juepiezhongren
Copy link
Author

completion handler is something that has to be done, but at least Task instance for delay will not be created.

@juepiezhongren
Copy link
Author

pooling will make handler least allocated, i think

@juepiezhongren
Copy link
Author

Task vs ValueTask with IVTS is just like Binding vs x:Bind

@marksmeltzer
Copy link

IValueTaskSource bypasses the need to allocate a completion handler delegate because the IValueTaskSource implementation handles the completion directly, which removes the need to allocate anything other than the IValueTaskSource implementation. Where would the IValueTaskSource implementation exist in ValueTask.Delay? It seems an allocation is required there.

As I mentioned in my comment (here https://github.com/dotnet/corefx/issues/27445#issuecomment-406839279), the only way I can think of to avoid the allocation would be to have the ValueTask.Delay() use an alternative enum specifically for delays and not use IValueTaskSource at all. It would have to allocate the completion handler delegate, and it could cheat and use ValueTask's state value to hold the delay time. The internal task APIs would have to be updated to handle this additional enum value for delay tasks.

That's a fair about of work, and it will not provide any benefits: all delays take a long time (15 milliseconds is a LONG time) and at that time-scale temporary allocations do NOT affect performance. Removing the Task allocation will not provide any performance benefit at that time-scale.

If it were possible to have an async ValueTask.DelayTicks( 1000 ) or ValueTask.DelayMicroseconds( 1 ) APIs that worked reliably, then we would get some benefit from removing the allocations. But currently the real-time clock APIs are neither precise enough nor fast enough to make that happen. Measuring time is actually quite expensive.

For more information about how hard it is to measure time in small intervals, review this article:
https://aakinshin.net/blog/post/datetime/

@benaadams
Copy link
Member

If you want the shortest delay possible, you might want to use Yield

await Task.Yield();

@marksmeltzer
Copy link

marksmeltzer commented Jul 22, 2018 via email

@jnm2
Copy link
Contributor

jnm2 commented Jul 22, 2018

Task.Yield returns a YieldAwaitable. What would ValueTask.Yield return?

@benaadams
Copy link
Member

Task.Yield returns a YieldAwaitable. What would ValueTask.Yield return?

Doesn't need to return anything different or exist; its already a struct awaitable. Just pointing out it is likely better than Task.Delay(1) for forcing async; but also executing asap

@jnm2
Copy link
Contributor

jnm2 commented Jul 22, 2018

@benadams That was in reply to @marksmeltzer's comment.

@stephentoub
Copy link
Member

stephentoub commented Jul 22, 2018

A ValueTask.Yield() might actually make some sense because the call can complete synchronously without any delay.

I don't see how this could be made any more efficient than Task.Yield.

Task.WhenAny and Task.WhenAll are required to be optimised for ValueTask

ValueTask.WhenAny is not viable; in fact most combinators are not, as there's no way to unregister a continuation. Only WhenAll is really feasible, and I'm not convinced even that is a good idea. ValueTask, and more generally anything reusable like this, is really meant for the case where you await the result of the invocation directly; anything else, and you greatly risk using an instance after you're supposed to, after it's already being used for something else. That's why the guidance for ValueTask is to use AsTask to get a Task if you're not awaiting it immediately. And that goes against the general point of WhenAll.

TaskCompletionSource is required to be reusable and pool-cached

The plan is to expose the ManualResetValueTaskSource{Logic} types, or some derivation thereof, once we've gotten a better feel for them and what APIs are actually needed.

I've opened https://github.com/dotnet/corefx/issues/31258 for that. I think this issue can be closed.

@marksmeltzer
Copy link

marksmeltzer commented Jul 22, 2018

@jnm2 I wasn't suggesting to actually add an API for ValueTask.Yield(). I personally haven't ever used Task.Yield() and I didn't know how the current API was implemented. I incorrectly assumed it returned a Task or something similar since @juepiezhongren had requested a ValueTask.Yield() API in the first place.

My goal was to describe the rationale to @juepiezhongren behind why ValueTask exists at all, rather than to justify any specific new APIs. (I generally agree with @stephentoub's reasoning against the WhenAll and WhenAny APIs for ValueTask.)

I am curious to hear more about this bit though:

The plan is to expose the ManualResetValueTaskSource{Logic} types, or some derivation thereof, once we've gotten a better feel for them and what APIs are actually needed.

That's an interesting concept. I have a set of fully managed "slim" event handle classes that have async support for all of the handes' operations (AutoReset, ManualReset, Semaphore, as well as a few other new patterns), and I could update those implementations to have them return ValueTasks instead to optimize the synchronous result case.

However, for the case when the operation cannot complete synchronously, I would still need to allocate a continuation to put into the handle's awaiters queue. I currently use TaskCompletionSources for that purpose. I don't see any way around the need for an allocation for the awaiters queue -- even if it was my own class implementing IValueTaskSource it would still be an allocation. So, TaskCompletionSources seem just fine for the case where the operation is asynchronous.

I am guessing that for the ManualResetValueTaskSource{Logic} types the team might have something in mind more like this good old post of @stephentoub's:
https://blogs.msdn.microsoft.com/pfxteam/2011/12/15/awaiting-socket-operations/

@stephentoub
Copy link
Member

stephentoub commented Jul 22, 2018

However, for the case when operation cannot complete synchronously, I would still need to allocate a continuation to put into the handle's awaiters queue.

I'm not following your use case, but each async method that completes asynchronously has a single delegate allocated and handed off to each awaiter in the method: if you await a thousand awaitables in that async method, there's still only one delegate allocated. And in .NET Core 2.1, in many cases you don't even have that one delegate, with special code paths being used to use the async method state machine object instead.

@marksmeltzer
Copy link

marksmeltzer commented Jul 22, 2018

@stephentoub I would have to see some code to fully understand the implementation(s) you're referring to to see if any could apply to my scenario. My current scenario looks something like this:

        public ValueTask<bool> WaitOneAsync( int timeoutMilliseconds )
        {
            if ( IsSet( ) )
            {
                return new ValueTask<bool>( true );
            }

            lock ( _awaiters )
            {
                if ( IsSet( ) )
                {
                    return new ValueTask<bool>( true );
                }

                var tcs = new TaskCompletionSource<bool>( );
                _awaiters.Enqueue( (timeoutMilliseconds, tcs ) ); //not real code -- the timeout would need to be handled properly
                return tcs.Task;
            }
        }

Konrad Kokosa has an interesting article up on how pooling can be combined with IValueTaskSource to eliminate allocations, and I think that his strategy could be adapted to generally eliminate the need for allocating a Task in the above code in a generalizable away that addresses all wait handle scenarios (not just manual reset).
http://tooslowexception.com/implementing-custom-ivaluetasksource-async-without-allocations/

Aside from something like pooling, I don't see another way to capture the operational context for the waiter since there isn't anything in the handle itself that corresponds to an operational context (unlike the AwaitableSocketAsyncEventArgs case which does have its own operational context). For example, if I implemented IValueTaskSource directly on the handle, it would be possible to implement IValueTaskSource.OnCompleted (this is where I would enqueue the awaiter's continuation delegate and state) but the IValueTaskSource.GetResult and IValueTaskSource.GetStatus methods wouldn't have a way of relating to the actual operation for all handle types.

Disclaimer: I'm not well versed in the underlying mechanisms and guarantees surrounding IValueTaskSource since I haven't tried implementing it yet. If the guarantees are sufficiently robust, then I suppose I might be able to get away with implementing IValueTaskSource.GetResult on the base handle class and always return true (or more technically it should return !_isDisposed) as long as the state machine would only call that method after the continuation delegate is invoked. That could work because the handle would only ever invoke the continuation when the handle is disposed or when the handle is set.

Okay, so that takes care of IValueTaskSource.OnCompleted and IValueTaskSource.GetResult... However, what about IValueTaskSource.GetStatus? I don't see a way to implement that directly on the base handle class in an coherent way that would have a meaningful correspondence to a specific operation. I can see how I could implement IValueTaskSource.GetStatus for the manual reset event (and maybe the semaphore event): it could return pending when not set, succeeded when set, and faulted when disposed. But I don't think I could implement IValueTaskSource.GetStatus for the auto reset event or some of the more exotic event handles that I have created.

The pooling strategy seems like it has legs though and I will try to implement that in an A/B performance comparison against the current strategy that allocates a task for each operation. If the net cost of the pooling mechanisms are less than the costs of the task allocations, I can readily see how that would provide an improvement.

Outside of pooling and implicit operational contexts, I would love to know about other innovative tricks the team or community has come up with for implementing IValueTaskSource.

If IValueTaskSource.GetStatus is only ever called after the continuation is invoked, when I suppose I could have it use this logic return !_isDisposed? ValueTaskSourceStatus.Succeeded : ValueTaskSourceStatus.Faulted;, but I'm not sure when the IValueTaskSource.GetStatus might be called.

@juepiezhongren
Copy link
Author

juepiezhongren commented Jul 31, 2018

@stephentoub , ValueTask.WhenAll(VTask0, VTask1, VTask2....) i means this kind needs api support and optimization, while VT mixed with T whenAll there should be some new api format

@stephentoub
Copy link
Member

i means this kind needs api support and optimization

I don't currently see enough benefit to introducing that. If and when there's a proven need, it could be examined, but in the meantime, it's functionally supported via Task.WhenAll(vt1.AsTask(), vt2.AsTask(), ...).

This issue was opened with a laundry list of items, most of which as has been discussed aren't feasible or relevant. If there are specific, viable, concrete proposals for new APIs, please open individual issues for them so that they may be addressed individually. In the meantime, I'm going to close this one.

Thanks.

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 3.0 milestone Jan 31, 2020
@dotnet dotnet locked as resolved and limited conversation to collaborators Dec 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Threading.Tasks
Projects
None yet
Development

No branches or pull requests

6 participants