Uh oh!
There was an error while loading. Please reload this page.
Refactor ManagedWebSocket to avoid forcing Task allocations for ReceiveAsync - #56282
Conversation
ghost
commented
Jul 26, 2021
Tagging subscribers to this area: @dotnet/ncl Issue DetailsThe ManagedWebSocket implementation today supports CloseAsyncs being issued concurrently with ReceiveAsyncs, even though CloseAsync needs to issue receives (this allowance was carried over from the .NET Framework implementation). Currently the implementation does that by storing the last ReceiveAsync task and awaiting it in CloseAsync if there is one, but that means multiple parties may try to await the same task multiple times (the original caller of ReceiveAsync and CloseAsync), which means we can't just use a ValueTask. So today asynchronously completing ReceiveAsyncs always use AsTask to create a Task from the returned ValueTask. This isn't actually an additional task allocation today, as the async ValueTask builder will create a Task for the asynchronously completing operation, and then AsTask will just return that (and when it completes synchronously, there's extra code to substitute a singleton). But once we switch to using the new pooling builder, that's no longer the case. This PR uses an async lock as part of the ReceiveAsync implementation, with the existing async method awaiting entering that lock. CloseAsync is then rewritten to be in terms of calling ReceiveAsync in a loop. This also lets us remove the existing Monitor used for synchronously coordinating state between these operations, as the async lock serves that purpose as well. Rather than using a SemaphoreSlim, since we expect zero contention in the common case, we use a simple AsyncMutex that's optimized for the zero contention case, using a single interlocked to acquire and a single interlocked to release the lock. Closes #50921
usingBenchmarkDotNet.Attributes;usingBenchmarkDotNet.Running;usingSystem.Net.WebSockets;[MemoryDiagnoser]publicclassProgram{publicstaticvoidMain(string[]args)=>BenchmarkSwitcher.FromAssembly(typeof(Program).Assembly).Run(args);privateclassConnection{publicreadonlyWebSocketClient,Server;publicreadonlyMemory<byte>ClientBuffer=newbyte[256];publicreadonlyMemory<byte>ServerBuffer=newbyte[256];publicreadonlyCancellationTokenCancellationToken=default;publicConnection(){(StreamStream1,StreamStream2)streams=ConnectedStreams.CreateBidirectional();Client=WebSocket.CreateFromStream(streams.Stream1,isServer:false,subProtocol:null,Timeout.InfiniteTimeSpan);Server=WebSocket.CreateFromStream(streams.Stream2,isServer:true,subProtocol:null,Timeout.InfiniteTimeSpan);}}privateConnection[]_connections=Enumerable.Range(0,256).Select(_ =>newConnection()).ToArray();privateconstintIters=1_000;[Benchmark]publicTaskPingPong()=>Task.WhenAll(fromcin_connectionsselectTask.WhenAll(Task.Run(async()=>{for(inti=0;i<Iters;i++){awaitc.Server.ReceiveAsync(c.ServerBuffer,c.CancellationToken);awaitc.Server.SendAsync(c.ServerBuffer,WebSocketMessageType.Binary,endOfMessage:true,c.CancellationToken);}}),Task.Run(async()=>{for(inti=0;i<Iters;i++){awaitc.Client.SendAsync(c.ClientBuffer,WebSocketMessageType.Binary,endOfMessage:true,c.CancellationToken);awaitc.Client.ReceiveAsync(c.ClientBuffer,c.CancellationToken);}})));}
|
stephentoub
commented
Jul 26, 2021
@davidfowl, can you help validate this with ASP.NET functional tests and against relevant ASP.NET perf tests? That needs to be done before this can be merged. |
Uh oh!
There was an error while loading. Please reload this page.
11562c5 to
d57be3dCompare
karelz
left a comment
There was a problem hiding this comment.
@CarnaViire if you get a chance to take a look as well, that would be good.
d57be3d to
74a34b8Comparestephentoub
commented
Aug 5, 2021
@davidfowl, @adityamandaleeka, any update on validating this change for ASP.NET? |
…veAsync The ManagedWebSocket implementation today supports CloseAsyncs being issued concurrently with ReceiveAsyncs, even though CloseAsync needs to issue receives (this allowance was carried over from the .NET Framework implementation). Currently the implementation does that by storing the last ReceiveAsync task and awaiting it in CloseAsync if there is one, but that means multiple parties may try to await the same task multiple times (the original caller of ReceiveAsync and CloseAsync), which means we can't just use a ValueTask. So today asynchronously completing ReceiveAsyncs always use AsTask to create a Task from the returned ValueTask. This isn't actually an additional task allocation today, as the async ValueTask builder will create a Task for the asynchronously completing operation, and then AsTask will just return that (and when it completes synchronously, there's extra code to substitute a singleton). But once we switch to using the new pooling builder, that's no longer the case. This PR uses an async lock as part of the ReceiveAsync implementation, with the existing async method awaiting entering that lock. CloseAsync is then rewritten to be in terms of calling ReceiveAsync in a loop. This also lets us remove the existing Monitor used for synchronously coordinating state between these operations, as the async lock serves that purpose as well. Rather than using a SemaphoreSlim, since we expect zero contention in the common case, we use a simple AsyncMutex that's optimized for the zero contention case, using a single interlocked to acquire and a single interlocked to release the lock.
74a34b8 to
14990bcComparestephentoub
commented
Aug 10, 2021
Once CI is green, I'll go ahead and merge this. In my own tests, this shows up as neutral to positive both locally in microbenchmarks and on asp-perf-lin and asp-citrine-lin in terms of throughput. We can revert it if it ends up having any negative impact once it makes it to dotnet/aspnetcore. @davidfowl, @adityamandaleeka, I'd still appreciate extra validation here, but at this point if I don't merge it's not going to make the release. The new websockets benchmark doesn't seem to really stress the system with or without this. |
davidfowl
commented
Aug 10, 2021
Merging and propagating a dependency flow PR is the easiest way to get validation. |
The ManagedWebSocket implementation today supports CloseAsyncs being issued concurrently with ReceiveAsyncs, even though CloseAsync needs to issue receives (this allowance was carried over from the .NET Framework implementation). Currently the implementation does that by storing the last ReceiveAsync task and awaiting it in CloseAsync if there is one, but that means multiple parties may try to await the same task multiple times (the original caller of ReceiveAsync and CloseAsync), which means we can't just use a ValueTask. So today asynchronously completing ReceiveAsyncs always use AsTask to create a Task from the returned ValueTask. This isn't actually an additional task allocation today, as the async ValueTask builder will create a Task for the asynchronously completing operation, and then AsTask will just return that (and when it completes synchronously, there's extra code to substitute a singleton). But once we switch to using the new pooling builder, that's no longer the case.
This PR uses an async lock as part of the ReceiveAsync implementation, with the existing async method awaiting entering that lock. CloseAsync is then rewritten to be in terms of calling ReceiveAsync in a loop. This also lets us remove the existing Monitor used for synchronously coordinating state between these operations, as the async lock serves that purpose as well. Rather than using a SemaphoreSlim, since we expect zero contention in the common case, we use a simple AsyncMutex that's optimized for the zero contention case, using a single interlocked to acquire and a single interlocked to release the lock.
Closes#50921