Merge SocketAsyncEventArgs into SocketAwaitable and use cached instances #18
Conversation
| private readonly SocketAwaitable socketAwaitable = new SocketAwaitable(); | ||
| private readonly byte[] headerBuffer = new byte[AmsHeaderHelper.AmsTcpHeaderSize]; |
There was a problem hiding this comment.
As the ReceiveLoop handles one receive after another, so in a serialized manner, the awaitable and the buffer can be re-used.
There was a problem hiding this comment.
Yes, I only got to optimizing the sending side of things.
| } | ||
|
|
||
| private bool closed; | ||
| private volatile bool closed; |
There was a problem hiding this comment.
To be on the safe side, this should be volatile as in the receive-loop it's checked (read) and at that point different threads are in play.
For x86-machines it's unlikely to be a real problem, but for good code hygiene and for platforms with a weak memory model it's a good addition. And no harm to x86-machines.
| } | ||
|
|
||
| private async Task ReceiveLoop() | ||
| { | ||
| await Task.Yield(); |
There was a problem hiding this comment.
Instead of the Task.Run in the ctor of this class.
There was a problem hiding this comment.
Task.Run was borrowed from RabbitMQ.Client, but I like this one as well.
| @@ -6,7 +6,7 @@ internal class Assertions | |||
| { | |||
| public static void AssertDataLength(ReadOnlySpan<byte> buffer, int length, int offset) | |||
| { | |||
| if (length + offset != buffer.Length) | |||
| if (length != buffer.Length - offset) | |||
There was a problem hiding this comment.
I'm paranoid about overflow, so prevent that where possible...
| @@ -10,18 +10,18 @@ namespace Viscon.Communication.Ads.Internal; | |||
| /// </remarks> | |||
| internal static class SocketExtensions | |||
| { | |||
| public static SocketAwaitable ReceiveAsync(this Socket socket, SocketAwaitable awaitable) | |||
| public static SocketAwaitable ReceiveAwaitable(this Socket socket, SocketAwaitable awaitable) | |||
There was a problem hiding this comment.
Needed to change the name, otherwise the compiler won't pick that extension method, as an overload on Socket itself would be choosen.
|
Thanks a lot for these changes, I'll take another look at them soon and will merge them then. |
| @@ -28,6 +28,8 @@ internal static class TypeExtensions | |||
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | |||
| public static ref byte GetOffset(this ref byte source, int offset) | |||
There was a problem hiding this comment.
Shouldn't offset be uint here as well to just avoid the cast at line 33 altogether?
There was a problem hiding this comment.
That's possible too, but then there's the cast in L25. If the parameter of that method is changed to uint too, then the cast will likely be somewhere else.
So I think it's just fine to have it here (once), and not scattered around the code-base.
|
Thanks a lot G眉nther! |
For socket operation:
Instead of creating (allocating) a SocketAsyncEventArgs and then a SocketAwaitable both are "merged" into one instance, which can be cached and re-used. Thus avoiding allocations and make it amortized allocation-free.
This could be driven further, by using ValueTask instead of Task. But with the current targets (< .NET 6) it's a bit cumbersome, so I didn't do this now.
@mycroes you mentioned this project in Sally7, so I had a quick look 馃槈.