Open MichalBrylka opened 2 years ago
as per data.ToArray();
this is copied from documentation:
Copies the contents of this read-only span into a new array. This heap allocates, so should generally be avoided, however it is sometimes necessary to bridge the gap with APIs written in terms of arrays.
Hello Team would you like me to provide a pull request with a solution ?
hi, would you like to accept a PR with improved serializer and deserializer ?
Hi, I took the liberty of creating an efficient deserializer for both sync and async version: EfficientProtobufDeserializer
Would you like me to create a pull request for your codebase with such deserializer?
thanks. yes, we'd like to but need to prioritize reviewing etc. at some point in the future, I expect we'll review many aspects of the serdes for performance, and consider this then.
@mhowlett let me know if/when I should prepare a PR. Code itself is ready
Description
I'd like to address issue in
DeserializeAsync
metod ofProtobufDeserializer
class: https://github.com/confluentinc/confluent-kafka-dotnet/blob/master/src/Confluent.SchemaRegistry.Serdes.Protobuf/ProtobufDeserializer.cs#L97I like the idea that API for
IAsyncDeserializer
usesReadOnlyMemory
(orReadOnlySpan
for sync version). In deserialization code however there is at lot of unnecessary allocations:I understand that they make code more smooth-looking but please have a look at this benchmark: https://github.com/nemesissoft/KafkaProtobufSyncOverAsyncPerf/blob/main/DeserializerBenchmarks.cs I've attached my results: https://github.com/nemesissoft/KafkaProtobufSyncOverAsyncPerf/blob/main/README.md
Could we improve this class by removing these allocations ? I've already proposed a quick solution: https://github.com/nemesissoft/KafkaProtobufSyncOverAsyncPerf/blob/main/Deserializers/NonAllocProtobufDeserializer.cs
Sadly there is no SpanStream class that would make coding easier (though I plan to provide one in future) but solution I've proposed (span + position to mock stream read-advance behaviors) should be enough for now
I personally do not see benefit of having async deserializer just to call
.ConfigureAwait(continueOnCapturedContext: false).GetAwaiter().GetResult();
but this overhead seems negligible. So even if we leave Deserialize method as async (or potentially add sync version as well), could we remove allocations ?How to reproduce
Run provided benchmark or look at results: https://github.com/nemesissoft/KafkaProtobufSyncOverAsyncPerf/blob/main/README.md
Checklist
Please provide the following information: