Use ValueTask instead of custom awaiter for SocketAwaitableEventArgs #2998

davidfowl · 2018-10-11T06:00:54Z

This could improve the performance of async operations (reduce the number of allocations)

TODO: Measure performance, I may combine this with #2974

- This could improve the performance of async operations (reduce the number of allocations)

gfoidl · 2018-10-11T07:57:56Z

src/Kestrel.Transport.Sockets/Internal/SocketAwaitableEventArgs.cs

-        protected override void OnCompleted(SocketAsyncEventArgs _)
+        /// <summary>Initiates a send operation on the associated socket.</summary>
+        /// <returns>This instance.</returns>
+        public ValueTask<int> SendAsync(Socket socket)


The code for ReceiveAsync and SendAsync is quite similar. Can you refactor this to one method, to avoid the code-duplication?

davidfowl · 2018-10-11T17:36:08Z

@dotnet-bot test OSX 10.12 Debug Build

pakrym · 2018-10-11T17:36:35Z

src/Kestrel.Transport.Sockets/Internal/SocketAwaitableEventArgs.cs

            {
-                ThrowSocketException(SocketError);
+                Debug.Fail("Multiple continuations registered!");


It would be really hard to find this bug in release build.

Thats why we run tests in both. I'm not worried about this though, this isn't general purpose. This is specifically used in a single place.

👍 I see no reason to not just throw or even Environment.FailFast in this case.

halter73 · 2018-10-11T18:48:31Z

src/Kestrel.Transport.Sockets/Internal/SocketAwaitableEventArgs.cs

        {
-            OnCompleted(this);
+            Volatile.Write(ref _completion, null);


Good catch. I guess theoretically without this volatile write, if OnCompleted(SocketAsyncEventArgs) was called from another thread before OnCompleted(Action<object> continuation, ..., the first OnCompleted call could observe a stale _completion callback and invoke it a second time.

halter73 · 2018-10-11T18:56:32Z

src/Kestrel.Transport.Sockets/Internal/SocketAwaitableEventArgs.cs

+        {
+            Action<object> completion = _completion;
+
+            if (completion != null || (completion = Interlocked.CompareExchange(ref _completion, _callbackCompleted, null)) != null)


I'm not sure completion != null is safe here. I think you need a memory barrier here for the same reason you need it in Reset(). For one, without a memory barrier, reading UserToken could be reordered to before reading _completion. There's also the issue with not transitioning into the _callbackCompleted state which would make GetStatus wrong momentarily, but I don't think that's very important.

halter73 · 2018-10-11T18:56:56Z

src/Kestrel.Transport.Sockets/Internal/SocketAwaitableEventArgs.cs


        private readonly PipeScheduler _ioScheduler;

-        private Action _callback;
+        private Action<object> _completion;


I think _callback or _continuation is a better name

halter73 · 2018-10-11T19:00:47Z

If this turns out to be valuable, OutputFlowControlAwaitable, ThreadPoolAwaitable and even LibuvAwaitable could probably use this IValueTaskSource treatment. As this gets more complicated, I'm wondering if at least some of this logic could be shared.

halter73 · 2018-10-11T19:02:15Z

src/Kestrel.Transport.Sockets/Internal/SocketAwaitableEventArgs.cs

        }

-        protected override void OnCompleted(SocketAsyncEventArgs _)
+        private ValueTask<int> CompleteSocketOperation()


I prefer GetCompletedValueTask() since this isn't used to complete socket operations in the async case.

halter73 · 2018-10-11T19:04:04Z

src/Kestrel.Transport.Sockets/Internal/SocketAwaitableEventArgs.cs

+            if (ReferenceEquals(awaitableState, _callbackCompleted))
+            {
+#if NETCOREAPP2_1
+                ThreadPool.QueueUserWorkItem(continuation, state, preferLocal: false);


I get this should be rare, but why not UQUWI?

https://github.com/dotnet/corefx/issues/32547

The continuation is Action<object>, so it's not like we're boxing.

Actually, you're right, I could just queue this instead of using the state object directly since the UserToken is a field.

Why use the UserToken field at all in this case? Just use the state object directly. UserToken is only necessary if OnCompleted(SocketAsyncEventArgs is responsible to running the continuation.

halter73 · 2018-10-11T19:07:45Z

src/Kestrel.Transport.Sockets/Internal/SocketAwaitableEventArgs.cs

+                ThreadPool.QueueUserWorkItem(continuation, state, preferLocal: false);
+#else
+                Task.Factory.StartNew(continuation, state);
+#endif


Since you null out UserToken in OnCompleted(SocketAsyncEventArgs it's probably best to also do that here.

I'll null it out after the work item is invoked but before calling the continuation.

You don't have to wait until after the work item invoked to null out UserToken for the same reason you don't need to in OnCompleted(SocketAsyncEventArgs. Just use the local state reference which isn't null. Technically you don't need to even set UserToken in this case, you could just set it in an else block at the end.

halter73 · 2018-10-11T19:11:22Z

src/Kestrel.Transport.Sockets/Internal/SocketAwaitableEventArgs.cs

-            {
-                _ioScheduler.Schedule(state => ((Action)state)(), continuation);
-            }
+            Reset();


Why call Reset() here? _completion must already be null, right? Which now that I think about it means GetStatus is wrong after sync completions. Again, I'm not sure how important that is.

Sync completions don't use this code path. Unless I'm missing something...

??? This is only called when ReceiveAsync/SendAsync return false, right?

I guess it's confusing because of the name. That's why I recommend renaming this to GetCompletedValueTask().

halter73 · 2018-10-11T20:00:00Z

src/Kestrel.Transport.Sockets/Internal/SocketAwaitableEventArgs.cs

+            var callbackState = args.UserToken;
+            args.UserToken = null;
+            var callback = args._callback;
+            callback(callbackState);


I see what was up now. I always seem to forget that you can't convert an Action<object> to a WaitCallback without allocating.

I'm going to change this back, it's a tad bit complicated and not worth saving the allocation.

halter73 · 2018-10-11T20:03:24Z

src/Kestrel.Transport.Sockets/Internal/SocketAwaitableEventArgs.cs


-            if (SocketError != SocketError.Success)
+        public int GetResult(short token)


The active test run was aborted. Reason: Unhandled Exception: System.InvalidOperationException: Multiple continuations registered!

You still need to null out _callback here. It's GetCompletedValueTask() where nulling out _callback is unnecessary because it will always be null there. Assuming you null it out here that is.

halter73 · 2018-10-11T21:37:15Z

src/Kestrel.Transport.Sockets/Internal/SocketAwaitableEventArgs.cs

+            int bytesTransferred = BytesTransferred;
+            SocketError error = SocketError;
+
+            Volatile.Write(ref _callback, null);


_callback should always already be null here.

halter73 · 2018-10-12T02:41:22Z

Both Jenkins Windows failures look really scary. All sockets functional tests and logs like the following:

16:05:36   Log Critical[ApplicationNeverCompleted]: Connection id "0HLHFPHRAPFNN" application never completed 
16:05:36   Log Critical[ApplicationNeverCompleted]: Connection id "0HLHFPHRAPFOC" application never completed

That's the "you unrooted the request processing continuation and we know b/c of our weak reference" log. In one of the runs, this was logged 7 times. 😱

halter73 · 2018-10-12T02:56:28Z

src/Kestrel.Transport.Sockets/Internal/SocketAwaitableEventArgs.cs

-        public bool IsCompleted => ReferenceEquals(_callback, _callbackCompleted);
+        protected override void OnCompleted(SocketAsyncEventArgs _)
+        {
+            var callback = Interlocked.CompareExchange(ref _callback, _callbackCompleted, null);


Why did you change this from Interlocked.Exchange to Interlocked.CompareExchange? You want to move into the _callbackCompleted state no matter what. Here you're only transitioning it if a callback isn't registered yet.

It was always using CompareExchange.

No it wasn't.

KestrelHttpServer/src/Kestrel.Transport.Sockets/Internal/SocketAwaitableEventArgs.cs

Lines 68 to 70 in 081cef0

protected override void OnCompleted(SocketAsyncEventArgs _)

{

var continuation = Interlocked.Exchange(ref _callback, _callbackCompleted);

I think the multiple OnCompleted methods make it easy to mix this kind of stuff up.

davidfowl · 2018-10-12T05:26:48Z

🆙 📅

davidfowl · 2018-10-12T06:09:05Z

Looks slightly worse:

RPS	CPU (%)	Memory (MB)	Avg. Latency (ms)	Startup (ms)	First Request (ms)	Latency (ms)
3,163,229	82	310	1.8	187	42.5	0.7

VS

RPS	CPU (%)	Memory (MB)	Avg. Latency (ms)	Startup (ms)	First Request (ms)	Latency (ms)
3,130,355	73	307	2.2	192	42.1	0.6

halter73

I guess we don't take it if there's no performance improvement and possibly even a regression. It doesn't seem to really simplify things either.

davidfowl · 2018-10-12T18:01:25Z

I guess we don't take it if there's no performance improvement and possibly even a regression. It doesn't seem to really simplify things either.

Yes that's the current conclusion. I need to see if there are debugging benefits here before I close it out (also future improvements that might be coming e.g. https://github.com/dotnet/coreclr/issues/20200).

Use ValueTask instead of custom awaiter for SocketAwaitableEventArgs

6d21ac3

- This could improve the performance of async operations (reduce the number of allocations)

davidfowl requested review from halter73 and pakrym October 11, 2018 06:00

Oops

16ee5c8

gfoidl reviewed Oct 11, 2018

View reviewed changes

Refactor common code

1ed82eb

pakrym reviewed Oct 11, 2018

View reviewed changes

Throw instead of asserting

dc3f461

halter73 reviewed Oct 11, 2018

View reviewed changes

CR feedback

a58ac20

halter73 reviewed Oct 11, 2018

View reviewed changes

Oops

52fb3e7

halter73 reviewed Oct 12, 2018

View reviewed changes

More changes

5f4eee3

halter73 suggested changes Oct 12, 2018

View reviewed changes

davidfowl closed this Oct 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use ValueTask instead of custom awaiter for SocketAwaitableEventArgs #2998

Use ValueTask instead of custom awaiter for SocketAwaitableEventArgs #2998

davidfowl commented Oct 11, 2018

gfoidl Oct 11, 2018

davidfowl commented Oct 11, 2018

pakrym Oct 11, 2018

davidfowl Oct 11, 2018

halter73 Oct 11, 2018

halter73 Oct 11, 2018

halter73 Oct 11, 2018

halter73 Oct 11, 2018

halter73 commented Oct 11, 2018

halter73 Oct 11, 2018

halter73 Oct 11, 2018 •

edited

Loading

davidfowl Oct 11, 2018

halter73 Oct 11, 2018

davidfowl Oct 11, 2018

halter73 Oct 11, 2018 •

edited

Loading

halter73 Oct 11, 2018

davidfowl Oct 11, 2018

halter73 Oct 11, 2018

halter73 Oct 11, 2018

davidfowl Oct 11, 2018

halter73 Oct 11, 2018

halter73 Oct 11, 2018

davidfowl Oct 12, 2018

halter73 Oct 11, 2018

davidfowl Oct 11, 2018

halter73 Oct 11, 2018

halter73 commented Oct 12, 2018 •

edited

Loading

halter73 Oct 12, 2018

davidfowl Oct 12, 2018

halter73 Oct 12, 2018

davidfowl commented Oct 12, 2018

davidfowl commented Oct 12, 2018

halter73 left a comment

davidfowl commented Oct 12, 2018 •

edited

Loading


		if (SocketError != SocketError.Success)
		public int GetResult(short token)

	protected override void OnCompleted(SocketAsyncEventArgs _)
	{
	var continuation = Interlocked.Exchange(ref _callback, _callbackCompleted);

Use ValueTask instead of custom awaiter for SocketAwaitableEventArgs #2998

Use ValueTask instead of custom awaiter for SocketAwaitableEventArgs #2998

Conversation

davidfowl commented Oct 11, 2018

Choose a reason for hiding this comment

davidfowl commented Oct 11, 2018

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

halter73 commented Oct 11, 2018

Choose a reason for hiding this comment

halter73 Oct 11, 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

halter73 Oct 11, 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

halter73 commented Oct 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

davidfowl commented Oct 12, 2018

davidfowl commented Oct 12, 2018

halter73 left a comment

Choose a reason for hiding this comment

davidfowl commented Oct 12, 2018 • edited Loading

halter73 Oct 11, 2018 •

edited

Loading

halter73 Oct 11, 2018 •

edited

Loading

halter73 commented Oct 12, 2018 •

edited

Loading

davidfowl commented Oct 12, 2018 •

edited

Loading