Skip to content
This repository was archived by the owner on Dec 18, 2018. It is now read-only.

Reduce Sockets mainloop Send/Receive statemachine size#2376

Merged
davidfowl merged 3 commits intoaspnet:devfrom
benaadams:sockets
Mar 14, 2018
Merged

Reduce Sockets mainloop Send/Receive statemachine size#2376
davidfowl merged 3 commits intoaspnet:devfrom
benaadams:sockets

Conversation

@benaadams
Copy link
Contributor

@benaadams benaadams commented Mar 12, 2018

Removed from sending statemachine struct 16+16+16+16+8+8 = 80 bytes (5 object refs)
Added to sending statemachine struct 16+8 = 24 bytes (1 object ref)

So 56 byte reduction overall and 4 object refs (slower copies) removed.

Similar to #2313

Before

DoSend.MoveNext is 4 .trys deep in il

image

After

ProcessSends.MoveNext is 1 .trys deep in il

Removed (larger due to padding/alignment):

  • ReadResult { ReadOnlySequence { 2 x SequencePosition { object, int } }, bool }
  • ReadOnlySequence { 2 x SequencePosition { object, int } }
  • Exception

Added

  • SequencePosition { object, int }
  • bool

image


private volatile bool _aborted;

private SequencePosition SendEnd;
Copy link
Member

Choose a reason for hiding this comment

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

This is pretty gross, can this be left as a local (that will get hoisted).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah :-/

But it means the main loop doesn't need error handling; so there is only the try catch from the statemachine itself.

Putting it back in the loop means the finally has to go back to ensure Output.AdvanceTo(End); is always called; even when its crashing out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh... maybe its not needed? dotnet/corefx#27596

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

@benaadams
Copy link
Contributor Author

Is now hoisting SequencePosition rather than ReadOnlySequence which is much smaller

image

@benaadams
Copy link
Contributor Author

Don't think it likes second commit; Complete throws...

Log Critical[0]: Unexpected exception in HttpConnection.ProcessRequestsAsync. System.ObjectDisposedException: Safe handle has been closed
402   at System.Runtime.InteropServices.SafeHandle.DangerousAddRef(Boolean& success)
403   at System.StubHelpers.StubHelpers.SafeHandleAddRef(SafeHandle pHandle, Boolean& success)
404   at Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv.Internal.Networking.LibuvFunctions.NativeMethods.uv_async_send(UvAsyncHandle handle)
405   at Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv.Internal.Networking.LibuvFunctions.async_send(UvAsyncHandle handle) in C:\projects\kestrelhttpserver\src\Kestrel.Transport.Libuv\Internal\Networking\LibuvFunctions.cs:line 179
406   at Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv.Internal.LibuvThread.Post[T](Action`1 callback, T state) in C:\projects\kestrelhttpserver\src\Kestrel.Transport.Libuv\Internal\LibuvThread.cs:line 192
407   at Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv.Internal.LibuvThread.Schedule[T](Action`1 action, T state) in C:\projects\kestrelhttpserver\src\Kestrel.Transport.Libuv\Internal\LibuvThread.cs:line 401
408   at System.IO.Pipelines.Pipe.TrySchedule[TState](PipeScheduler scheduler, Action`1 action, TState state)
409   at System.IO.Pipelines.Pipe.CompleteWriter(Exception exception)
410   at System.IO.Pipelines.Pipe.DefaultPipeWriter.Complete(Exception exception)
411   at Microsoft.AspNetCore.Server.Kestrel.Core.Adapter.Internal.AdaptedPipeline.<WriteOutputAsync>d__15.MoveNext() in C:\projects\kestrelhttpserver\src\Kestrel.Core\Adapter\Internal\AdaptedPipeline.cs:line 115
412--- End of stack trace from previous location where exception was thrown ---
413   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
414   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
415   at Microsoft.AspNetCore.Server.Kestrel.Core.Adapter.Internal.AdaptedPipeline.<RunAsync>d__14.MoveNext() in C:\projects\kestrelhttpserver\src\Kestrel.Core\Adapter\Internal\AdaptedPipeline.cs:line 44
416--- End of stack trace from previous location where exception was thrown ---
417   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
418   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
419   at System.Runtime.CompilerServices.TaskAwaiter.ValidateEnd(Task task)
420   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.HttpConnection.<ProcessRequestsAsync>d__47`1.MoveNext() in C:\projects\kestrelhttpserver\src\Kestrel.Core\Internal\HttpConnection.cs:line 179

@benaadams
Copy link
Contributor Author

Hmm... that's libuv, which is uneffected

@benaadams
Copy link
Contributor Author

Also test pass/fail results seem to be ignoring it..?

@benaadams
Copy link
Contributor Author

benaadams commented Mar 12, 2018

Removed from sending statemachine struct 16+16+16+16+8+8 = 80 bytes (5 object refs)
Added to sending statemachine struct 16+8 = 24 bytes (1 object ref)

So 56 byte reduction overall and 4 object refs (slower copies) removed.

@benaadams
Copy link
Contributor Author

i.e.

Removed (larger due to padding/alignment):

  • ReadResult { ReadOnlySequence { 2 x SequencePosition { object, int } }, bool }
  • ReadOnlySequence { 2 x SequencePosition { object, int } }
  • Exception

Added

  • SequencePosition { object, int }
  • bool

image

@halter73
Copy link
Member

Any idea how this 56 byte async state machine reduction affects the techempower benchmarks?

@benaadams
Copy link
Contributor Author

It also trims the Send MoveNext from 576 il to 380 il and drops 3 .try blocks, 4 catch, 1 filter and 2 finally

Any idea how this 56 byte async state machine reduction affects the techempower benchmarks?

Alas I am between locations atm, is there a magic incantation you can run? (for sockets)

@davidfowl
Copy link
Member

I’ll run it for you

@halter73
Copy link
Member

Thanks @davidfowl. I'm not trying to make it harder for you to do free work for us @benaadams. 😄 I was just wondering if you already did your own RPS measurements.

@davidfowl
Copy link
Member

Looks like it's within noise range but it does clean up a code a bit (looks nicer).

@davidfowl davidfowl merged commit e65e58d into aspnet:dev Mar 14, 2018
@benaadams benaadams deleted the sockets branch March 14, 2018 06:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants