dotnet / orleans

Cloud Native application framework for .NET
https://docs.microsoft.com/dotnet/orleans
MIT License
10.07k stars 2.03k forks source link

Does/will Orleans support the new ValueTask<T> awaitable? #2480

Closed TsengSR closed 6 years ago

TsengSR commented 7 years ago

I was just again going through the Orleans tutorials and when I looked at this one I just remembered that during the .NET Core development a new ValueTask<T> was introduced

ValueTask<T>' has the goal to reduce/avoid allocating of a Task class when returning an value which do not involve calling an async method.

The example where I immediately thought this may be useful is this one (reduced to get to the point):

public class Employee : Grain, IEmployee
{
    public Task<int> GetLevel()
    {
        // allocates a Task
        return Task.FromResult(_level);
    }

    private int _level;
}

The line here return Task.FromResult(_level); will cause a task to be allocated even if there is no async operation involved. While in theory we could create a task from it and cache it we would have to that every time it changes.

The interesting thing is, ValueTask<T> is a struct and stored on a stack rather than on the heap and when we frequently call code which only returns a value w/o involving async operation, it will lower the preasure on GC.

The usage with it would be

public class Employee : Grain, IEmployee
{
    public ValueTask<int> GetLevel()
    {
        return new ValueTask(_level);
    }

    private int _level;
}

We can even use it, when we have a caching layer and there is a chance async operation involved to reduce the allocation of Task.

public class Employee : Grain, IEmployee
{
    public ValueTask<int> GetLevel()
    {
        int level;

        // if cached return synchronously, otherwise get it from asynchronously 
        if(!cache.TryGetValue("level", out level) 
        {
            // Task will only be allocated when the value is not in cache
            Task task = remoteGrain.GetLevel(...)
                .ContinueWith(_ => cache.AddValue("level", level));

            return new ValueTask(task);
        }

       return new ValueTask(level);
    }
}

There is also an excellent post by Bar Arnon on this topic here.

ValueTask<T> (source here) was added as part of System.Threading.Tasks.Channels and later moved to .NET Core corefx, so its available out of the box.

The good news is, System.Threading.Tasks.Extensions is available as separate package and it runs on .NET Framework 4.5 as well as .NET Core and mobile platforms, so it can be implemented (if not already supported out of the box) to Orleans w/o having to delay it until full .NET Core support is there.

ValueTask<T> is also awaitable and can be used like Task<T>.

galvesribeiro commented 7 years ago

Yeah, ValueTask<T> is really a must-have in Orleans and that could reduce a lot GC pressure and memory consumptions... Many grain method calls are just returning values from its state just like you pointed on your sample and like you said, it will allocate a new Task for it.

I wasn't aware that they created the extensions package already for backward support on non-.Net Core applications. That is really good.

@ReubenBond would be hard to add this to 1.4.0? I believe there would be changes to codegen to support it, right?

ReubenBond commented 7 years ago

@galvesribeiro we would need changes to codegen, but they ought to be easy enough to implement if we're not trying to be too smart.

The simplest change would be breaking because in order to make the most out of ValueTask<T>, the IGrainMethodInvoker.Invoke method would be made to return ValueTask<object>. This would have an impact on interceptors, too.

I think it's a decent change to make anyhow, but we should weigh up the API breakages for inclusion in v2.0.

EDIT: if we wanted to get really crazy, we would avoid the boxing conversion (from int to object, etc) and form the response directly in the invoke.

EDIT 2: even if we end up taking a conservative approach and don't optimize for allocations, we should add support for ValueTask<T>.

galvesribeiro commented 7 years ago

I see... An easy but breaking change...

So, @TsengSR, even having the extensions package that would make us support to current userbase, we would still break some of them that are using interceptors like Reuben said.

So, I'm afraid it would probably get pushed to 2.0 which in all cases come along with .Net Core support anyway...

But I still agree that we should add it...

jdom commented 7 years ago

Well, the issue is the single interface that both clients and grain implementation use. From the client perspective, using ValueTask is always Les efficient, since it always incurs in a network call. Ideally, it'd be best if the grain interface (or IDL) is defined (either using sync or async methods) and a client interface is codegen'ed to always use tasks. No ValueTasks involved there. That's easier in functional via a unified interface such a s in Orleankka, because there is not even a need to generate a client interface at compile time

ReubenBond commented 7 years ago

To be clear, we can support this without breaking anything - so that's a good first step. The question is whether or not we want to add further optimizations to not just support this but take advantage of the potentially immediate nature of ValueTask<T> and reduce allocations for non-Task results.

TsengSR commented 7 years ago

@ReubenBond: I wonder if that first step wouldn't reduce the allocations at all or just not as much as changing the IGrainMethodInvoker.Invoke?

I tried to look at the source and see where the result of an grain is awaited but I'm not familiar with the internals of Orleans. When I created the issue, I thought there'd be somewhere an await for the grain method call before it's being serialized and sent over the wire (i.e. to the client calling the grain), so that it would reduce it within the silos.

ReubenBond commented 7 years ago

@TsengSR if we converted our generated invoker into an async Task<object>, then we could use ValueTask<T> grain methods with 1 less allocation than Task<T> methods in the synchronous case.

Converting the generated invoker to async ValueTask<object> may reduce allocations by one more in the above case and by 0 + additional ValueTask struct overhead in the typical case.

kutensky commented 6 years ago

I'll try to implement this one

ReubenBond commented 6 years ago

Good luck, @kutensky, please feel free to message me (Twitter, Gitter) so we can chat about the approach

kutensky commented 6 years ago

4562

Ok. So I've done a first step. With my code change our code generator can work with ValueTask. As for now it only allow users to use ValueTask as a grain method return type, so if they want to use it, they can. Inside of our core logic we still use Task, converting it to ValueTask there and back. But as a second step I'll try to change our core logic to use ValueTask all the way. It should increase performance and decrease memory allocation even when user doesn't use ValueTask.

ReubenBond commented 6 years ago

Fixed by @kutensky in #4562