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

Allocation-free Write(AndFlush)Async using ValueTask #375

Open
wants to merge 29 commits into
base: dev
Choose a base branch
from

Conversation

maksimkim
Copy link
Member

@maksimkim maksimkim commented Mar 12, 2018

Motivation:

corefx 2.1 will introduce ability to implement allocation free async awaitables using existing ValueTask struct and new IValueTaskSource interface (https://github.com/dotnet/corefx/issues/27445). PR adopts this new feature for write apis to avoid unnecessary Task allocation on every WriteAsync call.

ValueTask backed by IValueTaskSource by design has lots of limitations (see corefx issue for details).

In this PR IValueTaskSource is mostly implemented by recyclable objects (ChannelOutboundBuffer.Entry, PendingWriteQueue.PendingWrite, AbstractChannelHandlerContext.WriteTask, etc). It means than once completed it is immediately recycled which has certain implication on how it can be used inside and outside DotNetty code:

  • Once ValueTask is returned by WriteAsync call it should be either immediately awaited or immediately converted to Task or never touched again (e.g. storing it as a class field is not allowed).
    Not following this restriction can't be verified by compiler and can cause runtime error (if it was recycled after completion) or unpredictable result (if it was reactivated from pool for different async operation).

  • WriteAndFlushAsync method doesn't follow guideline above (it calls WriteAsync then Flush and then returns ValueTask to a caller) that is why new overload of WriteAndFlushAsync is introduced.
    New notifyComplete bool parameter defines if caller is interested in completion notification (for the penalty of Task allocation) or not. In later case completed ValueTask is returned from WriteAndFlushAsync.

Modifications:

  • Corresponding projects reference new System.Threading.Tasks.Extensions package version from myget dotnet feed. Assumption is that by the time PR is reviewed new version is available on nuget.org.
  • New promise hierarchy is introduced (IPromise, AbstractPromise, AbstractRecyclablePromise) to provide similar capabilities as TaskCompletionSource but for ValueTask/IValueTaskSource pattern.
  • WriteAsync/WriteAndFlushAsync return ValueTask
  • WriteAndFlushAsync has new overload with notifyComplete bool parameter to opt out of Task allocation

Result:

  • If calling code is not interested in awaiting write completion no TaskCompletionSource/Task allocation happen (DotNetty.Transport.Tests.Performance.Sockets.TcpChannelPerfSpecs+TcpChannel_Duplex_Throughput_10_messages_per_flush has twice less Gen0 collections).

@@ -35,10 +36,10 @@ protected override void ChannelRead0(IChannelHandlerContext contex, string msg)
response = "Did you say '" + msg + "'?\r\n";
}

Task wait_close = contex.WriteAndFlushAsync(response);
ValueTask wait_close = contex.WriteAndFlushAsync(response);
Copy link
Member

Choose a reason for hiding this comment

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

waitClose?

}
catch (Exception ex)
{
return TaskEx.FromException(new EncoderException(ex));
throw new EncoderException(ex);
Copy link
Member

Choose a reason for hiding this comment

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

throwing synchronously in async method is a bad practice. Can we return failure in ValueTask?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

and what happens to that exception?

Copy link
Member Author

Choose a reason for hiding this comment

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

Presumable it baubles up all the way to a caller

Copy link
Member Author

Choose a reason for hiding this comment

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

However ChannelHandlerContext always converts it to failed future:
https://github.com/netty/netty/blob/74f24a5c19f8f351e9c6a7a84bdd9fbcc7a07ada/transport/src/main/java/io/netty/channel/AbstractChannelHandlerContext.java#L740

So probably context is assumed to be higher level exception handler here.

Copy link
Member

Choose a reason for hiding this comment

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

so, pro: we're collecting stack trace which might be useful debugging real issue
cons: we're collecting stack trace which is costly (prohibitively so in some scenarios).
not sure what is worse here. I'd guess if we wanted to return a failed future here it would be a valuetask wrapping failed Task, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

main pro I see: we're close to original source code so it won't cause unexpected behavior when porting code based on this encoder/decoder infra in future.

For failed future yes, Exception -> Task -> ValueTask. Stephen Toub explicitly said that exceptions are already expensive enough and making ValueTask reference exception instance directly doesn't make sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

@nayato @maksimkim If we assume Exceptions are rare, it would ok to accept the perf hit.

{
try
{
await future;
Copy link
Member Author

Choose a reason for hiding this comment

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

AbstractPromise now can flow both execution and synchronization context. What is our context management story?

  • Can we use await in dotnetty
  • If we can, should it always be ConfigureAwait false?
  • Should we suppress execution context?

Copy link
Member

Choose a reason for hiding this comment

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

if you do .ConfigureAwait(false), you would erase notion of custom task scheduler that dispatches continuation to this same event loop.

Copy link
Member Author

Choose a reason for hiding this comment

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

So in case of ValueTask it's IValueTaskSource implementation responsibility to execute continuation. That implementation presumably not aware of any task scheduler.

CloseInput((IChunkedInput<T>)promise.Message);
promise.TrySetException(ex);
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

@StormHub , I've aligned behavior with netty here: progress() never completes promise
https://github.com/netty/netty/blob/8d78893a76a8c14c18b304ffe1fce2dc43ed4206/handler/src/main/java/io/netty/handler/stream/ChunkedWriteHandler.java#L278

Any reason it was ported in different way?

Copy link
Contributor

Choose a reason for hiding this comment

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

@maksimkim I don't think there is a particular reason. Let me double check.

Copy link
Contributor

Choose a reason for hiding this comment

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

@maksimkim I think I did it wrong. Netty progress never completes the promise.

Copy link
Contributor

Choose a reason for hiding this comment

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

build.cake Outdated
@@ -50,7 +50,7 @@ Task("Restore-NuGet-Packages")
.Description("Restores dependencies")
.Does(() =>
{
DotNetCoreRestore();
DotNetCoreRestore(new DotNetCoreRestoreSettings { ConfigFile = ".nuget\\nuget.config" });
Copy link
Member

Choose a reason for hiding this comment

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

do we still need this? and change to nuget.config above?

Copy link
Member Author

Choose a reason for hiding this comment

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

we don't

@@ -67,11 +67,13 @@ public interface IChannelHandlerContext : IAttributeMap

IChannelHandlerContext Read();

Task WriteAsync(object message); // todo: optimize: add flag saying if handler is interested in task, do not produce task if it isn't needed
ValueTask WriteAsync(object message); // todo: optimize: add flag saying if handler is interested in task, do not produce task if it isn't needed

IChannelHandlerContext Flush();

Task WriteAndFlushAsync(object message);
Copy link
Member Author

@maksimkim maksimkim Apr 12, 2018

Choose a reason for hiding this comment

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

Since WriteAndFlushAsync is consequent WriteAsync and Flush, and later can cause former to complete synchronously, ValueTask returned from WriteAsync backed by recyclable future is always materialized in full blown Task. So I kept Task as return type. Assumption is that those who want to do write+flush in non-allocating fire-and-forget mode call overload and pass false in notifyComplete. In that case overload always returns completed ValueTask.

Task writeTask = ctx.WriteAndFlushAsync(upgradeResponse);

// Perform the upgrade to the new protocol.
this.sourceCodec.UpgradeFrom(ctx);
Copy link
Member Author

Choose a reason for hiding this comment

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

}

Task task = base.WriteAsync(ctx, msg).AsTask();
task.ContinueWith(this.upgradeCompletedContinuation, ctx, TaskContinuationOptions.ExecuteSynchronously);
Copy link
Member Author

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants