Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
¹This PR only optimizes
WebSocketPayloadTransportService
'sReceiveAsync
method.This PR is one of many that will tackle various issues mentioned in #254.
This one in particular relates to the performance (speed, allocations) concerns raised by the current implementation of receiving a payload (as implemented by the aforementioned class).
Notable changes are:
CommunityToolKit.HighPerformance
WebSocketReceiveResult
to its struct-based alternativeJsonSerializer.Deserialize<T>
instead of its async alternativeConsiderations and justifications
It should be considered that as a critical part of the library, it's important that this code is battle-tested, and not just validated by unit-tests. In theory, the logic of the receive flow has not changed, but there's always the possibility for bugs and edge-cases that may be directly or indirectly found becuase of this PR; any bugs reported in regards to receiving gateway payloads should probably reference this PR for ease of tracking.
²The implementation of
ArrayPoolBufferWriter<T>
is backed byArrayPool<T>.Shared
by defaultAs for why these changes were made:
CommunityToolKit.HighPerformance
is, as it says on the tin, a high-performance set of tools and APIs (such asArrayPoolBufferWriter<T>
) which could have even futher utlization within Remora after some careful consideration and triaging of potential optimizations (such as those commented upon in #254). (SendAsync was not changed as incoming payloads outnumber outgoing by upward of several orders of magnitude for larger bots).The change to
ValueWebSocketReceiveResult
is twofold;ArrayPoolBufferWriter<T>
exposesMemory<T>
andSpan<T>
, the former of which is accepted as an overload toClientWebSocket#ReceiveAsync
, which is returns a struct result instead. This struct only contains two fields, so is much smaller, and never touches the heap. This optimization, if it can be called such, comes for free as a side-effect of other changes.JsonSerializer.DeserializeAsync<T>
was changed to its synchronous alternative because:The latter is especially important to consider, given where the data is coming from.
Becuase the only asynchronous part of the deserialization part is reading from the stream, this is rather bad becuase the stream it's reading from is a
MemoryStream
, which means all the data it's reading is already sitting in memory, not on the wire/on disk. Becuase of this, the overhead of async actually causes deserialization to be slower than if the synchronous version was used.