Skip to content

Add inline scheduler option for Sockets transport #24638

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Aug 18, 2020
Merged

Add inline scheduler option for Sockets transport #24638

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d4a38d9
Add inline scheduler option for Sockets transport
Aug 6, 2020
7c71dec
summary
BrennanConroy Aug 7, 2020
6e9e472
comment
BrennanConroy Aug 13, 2020
1100c2b
whoops
BrennanConroy Aug 13, 2020
0b3599e
wording?
BrennanConroy Aug 17, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/Servers/Kestrel/Transport.Sockets/src/Client/SocketConnectionFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,8 @@ public async ValueTask<ConnectionContext> ConnectAsync(EndPoint endpoint, Cancel
_trace,
_options.MaxReadBufferSize,
_options.MaxWriteBufferSize,
_options.WaitForDataBeforeAllocatingBuffer);
_options.WaitForDataBeforeAllocatingBuffer,
_options.UnsafeInlineScheduling);

socketConnection.Start();
return socketConnection;
Expand Down
19 changes: 14 additions & 5 deletions src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketConnection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,12 @@ internal sealed class SocketConnection : TransportConnection

internal SocketConnection(Socket socket,
MemoryPool<byte> memoryPool,
PipeScheduler scheduler,
PipeScheduler transportScheduler,
ISocketsTrace trace,
long? maxReadBufferSize = null,
long? maxWriteBufferSize = null,
bool waitForData = true)
bool waitForData = true,
bool useInlineSchedulers = false)
{
Debug.Assert(socket != null);
Debug.Assert(memoryPool != null);
Expand All @@ -60,16 +61,24 @@ internal SocketConnection(Socket socket,
// On *nix platforms, Sockets already dispatches to the ThreadPool.
// Yes, the IOQueues are still used for the PipeSchedulers. This is intentional.
// https://github.com/aspnet/KestrelHttpServer/issues/2573
var awaiterScheduler = IsWindows ? scheduler : PipeScheduler.Inline;
var awaiterScheduler = IsWindows ? transportScheduler : PipeScheduler.Inline;

var applicationScheduler = PipeScheduler.ThreadPool;
if (useInlineSchedulers)
{
transportScheduler = PipeScheduler.Inline;
awaiterScheduler = PipeScheduler.Inline;
applicationScheduler = PipeScheduler.Inline;
}

_receiver = new SocketReceiver(_socket, awaiterScheduler);
_sender = new SocketSender(_socket, awaiterScheduler);

maxReadBufferSize ??= 0;
maxWriteBufferSize ??= 0;

var inputOptions = new PipeOptions(MemoryPool, PipeScheduler.ThreadPool, scheduler, maxReadBufferSize.Value, maxReadBufferSize.Value / 2, useSynchronizationContext: false);
var outputOptions = new PipeOptions(MemoryPool, scheduler, PipeScheduler.ThreadPool, maxWriteBufferSize.Value, maxWriteBufferSize.Value / 2, useSynchronizationContext: false);
var inputOptions = new PipeOptions(MemoryPool, applicationScheduler, transportScheduler, maxReadBufferSize.Value, maxReadBufferSize.Value / 2, useSynchronizationContext: false);
var outputOptions = new PipeOptions(MemoryPool, transportScheduler, applicationScheduler, maxWriteBufferSize.Value, maxWriteBufferSize.Value / 2, useSynchronizationContext: false);

var pair = DuplexPipe.CreateConnectionPair(inputOptions, outputOptions);

Expand Down
3 changes: 2 additions & 1 deletion src/Servers/Kestrel/Transport.Sockets/src/SocketConnectionListener.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,8 @@ public async ValueTask<ConnectionContext> AcceptAsync(CancellationToken cancella
}

var connection = new SocketConnection(acceptSocket, _memoryPool, _schedulers[_schedulerIndex], _trace,
_options.MaxReadBufferSize, _options.MaxWriteBufferSize, _options.WaitForDataBeforeAllocatingBuffer);
_options.MaxReadBufferSize, _options.MaxWriteBufferSize, _options.WaitForDataBeforeAllocatingBuffer,
_options.UnsafeInlineScheduling);

connection.Start();

Expand Down
8 changes: 8 additions & 0 deletions src/Servers/Kestrel/Transport.Sockets/src/SocketTransportOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,14 @@ public class SocketTransportOptions

public long? MaxWriteBufferSize { get; set; } = 64 * 1024;

/// <summary>
/// Inline application and transport continuations instead of dispatching to the threadpool.
/// </summary>
/// <remarks>
/// This will run application code on the IO thread which is why this is unsafe.
Copy link
Contributor

Choose a reason for hiding this comment

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

Explain how this can hurt your performance. Also document the DOTNET_SYSTEM_NET_SOCKETS_INLINE_COMPLETIONS environment variable.

/// </remarks>
public bool UnsafeInlineScheduling { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

RunOnTransportThread

Copy link
Member Author

Choose a reason for hiding this comment

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

Doesn't sound unsafe enough!

Copy link
Member

Choose a reason for hiding this comment

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

UnsafeRunOnTransportThread!

Copy link
Contributor

@pranavkm pranavkm Aug 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
public bool UnsafeInlineScheduling { get; set; }
public bool UnsafePreferInlineScheduling { get; set; }

Copy link
Contributor

Choose a reason for hiding this comment

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

  • We add the API and get the API added in dotnet/runtime. Setting our API should set both.
  • We add the API and We use private reflection to set it on the socket itself. Setting our API should set both.
  • We don't expose a public API and we also consume the env variable. (and we have an opt-out for weird cases)

Copy link
Member Author

@BrennanConroy BrennanConroy Aug 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Looks like the env variable in Runtime globally affects some settings and even if you use private reflection to set the per Socket setting you don't affect the other things the env variable does. This results in massive perf regressions, i.e. 100k RPS without the env variable but with private reflection vs. 1.2M RPS with the env variable and no private reflection.

https://github.com/tmds/runtime/blob/6ea1256bf8bd9ce6c1db278603b332cec144ba75/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncEngine.Unix.cs#L26


internal Func<MemoryPool<byte>> MemoryPoolFactory { get; set; } = System.Buffers.SlabMemoryPoolFactory.Create;
}
}