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

maksimkim · 2018-03-12T19:19:08Z

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).

nayato · 2018-03-13T16:34:47Z

examples/Telnet.Server/TelnetServerHandler.cs

@@ -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);


nayato · 2018-03-13T16:38:14Z

src/DotNetty.Codecs/MessageToByteEncoder.cs

 }
 catch (Exception ex)
 {
- return TaskEx.FromException(new EncoderException(ex));
+ throw new EncoderException(ex);


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

Any reason it was diverged from original netty in a first place:
https://github.com/netty/netty/blob/0692bf1b6a6764dee22132630bef7ee2f850771e/codec/src/main/java/io/netty/handler/codec/MessageToByteEncoder.java#L125

and what happens to that exception?

Presumable it baubles up all the way to a caller

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.

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?

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.

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

maksimkim · 2018-03-15T00:58:18Z

src/DotNetty.Common/Utilities/TaskEx.cs

+ {
+ try
+ {
+ await future;


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?

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

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

maksimkim · 2018-04-12T01:04:33Z

src/DotNetty.Handlers/Streams/ChunkedWriteHandler.cs

+ CloseInput((IChunkedInput<T>)promise.Message);
+ promise.TrySetException(ex);
+ }
+ }


@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?

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

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

@maksimkim also note that success also calls progress
https://github.com/netty/netty/blob/4f982be91e6d61c7c7a9d8d63115e0ebe65b2774/handler/src/main/java/io/netty/handler/stream/ChunkedWriteHandler.java#L350

nayato · 2018-04-12T18:55:24Z

build.cake

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


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

maksimkim · 2018-04-12T22:04:54Z

src/DotNetty.Transport/Channels/IChannelHandlerContext.cs

@@ -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);


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.

maksimkim · 2018-10-10T07:55:21Z

src/DotNetty.Codecs.Http/HttpServerUpgradeHandler.cs

+ Task writeTask = ctx.WriteAndFlushAsync(upgradeResponse);
+
+ // Perform the upgrade to the new protocol.
+ this.sourceCodec.UpgradeFrom(ctx);


@StormHub, check this out. I change it to align with netty. So upgrade doesn't wait write to complete.
https://github.com/netty/netty/blob/00afb19d7a37de21b35ce4f6cb3fa7f74809f2ab/codec-http/src/main/java/io/netty/handler/codec/http/HttpServerUpgradeHandler.java#L324

maksimkim · 2018-10-10T07:59:12Z

src/DotNetty.Codecs.Http/WebSockets/Extensions/WebSocketServerExtensionHandler.cs

 }
+
+ Task task = base.WriteAsync(ctx, msg).AsTask();
+ task.ContinueWith(this.upgradeCompletedContinuation, ctx, TaskContinuationOptions.ExecuteSynchronously);


@StormHub, check this out. Aligned with netty
https://github.com/netty/netty/blob/00afb19d7a37de21b35ce4f6cb3fa7f74809f2ab/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/extensions/WebSocketServerExtensionHandler.java#L118

nayato requested changes Mar 13, 2018

View reviewed changes

maksimkim force-pushed the writevaluetask branch from df42e17 to 9e5d322 Compare March 14, 2018 21:43

maksimkim commented Mar 15, 2018

View reviewed changes

maksimkim force-pushed the writevaluetask branch from d0d827a to e4a836d Compare March 22, 2018 07:29

maksimkim force-pushed the writevaluetask branch 2 times, most recently from d3240b7 to c468763 Compare April 12, 2018 00:59

maksimkim commented Apr 12, 2018

View reviewed changes

nayato reviewed Apr 12, 2018

View reviewed changes

maksimkim commented Apr 12, 2018

View reviewed changes

maksimkim force-pushed the writevaluetask branch from ad712da to 7f473c1 Compare June 5, 2018 18:05

maksimkim force-pushed the writevaluetask branch from 7f473c1 to 1236174 Compare October 10, 2018 07:52

maksimkim commented Oct 10, 2018

View reviewed changes

maksimkim added 17 commits October 10, 2018 14:52

custom alloc free custom awaitable

0be4706

ChannelFuture -> ValueTask

ff944b2

tls fixes

2b6f37d

single callback per future + callback execution on executor

47ff644

uv changes

4f8f30e

continuations and recycle are inlined

c70594a

test fixes

eef526d

revert libuv changes

8abd878

cleanup

cd0b11e

update packages

1cef415

multi continuation support returned back

5bed79f

using nuget.config on pkg restore

d0836b0

add nuget.org source

ed79cce

rename

208e69c

execution and sync context preservation

c9d0c8a

context propagation fixes

67b306b

post rebase fixes

619bfb8

maksimkim added 12 commits October 10, 2018 14:55

multicontinuation fix for timeout handler

baa5e3a

switch to nuget.org for tasks.extensions package

3428e2f

default WriteAndFlushAsync returns Task

eb5ab1b

post rebase fixes

4dcb8bb

remove nuget config

78ec006

upgrade packages to stable version

fc447d7

ws fixes

03e7b1d

bump up package versions

5fab149

http test fixes

6c99f3a

fix keep alive implementation

05e4aa6

disable configureawait

7f08e1d

package version fixes

c1c9865

maksimkim force-pushed the writevaluetask branch from 2b21e67 to c1c9865 Compare October 10, 2018 22:27

nayato self-requested a review March 26, 2019 18:19

maksimkim mentioned this pull request May 22, 2021

Why use IPromise and not ValueTask for Write/Close/etc ? cuteant/SpanNetty#48

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

maksimkim commented Mar 12, 2018 •

edited

Loading

nayato Mar 13, 2018

nayato Mar 13, 2018

maksimkim Mar 13, 2018

nayato Mar 13, 2018

maksimkim Mar 14, 2018

maksimkim Mar 14, 2018

nayato Mar 14, 2018

maksimkim Mar 14, 2018

StormHub Mar 15, 2018

maksimkim Mar 15, 2018

nayato Mar 15, 2018

maksimkim Mar 15, 2018

maksimkim Apr 12, 2018

StormHub Apr 12, 2018

StormHub Apr 13, 2018

StormHub Apr 13, 2018

nayato Apr 12, 2018

maksimkim Apr 12, 2018

maksimkim Apr 12, 2018 •

edited

Loading

maksimkim Oct 10, 2018

maksimkim Oct 10, 2018

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

Are you sure you want to change the base?

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

Conversation

maksimkim commented Mar 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maksimkim Apr 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maksimkim commented Mar 12, 2018 •

edited

Loading

maksimkim Apr 12, 2018 •

edited

Loading