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.
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
Protobuf deserializer is allocating unnecessary memory #1701
Comments
as per |
Hello Team |
hi, would you like to accept a PR with improved serializer and deserializer ? |
Hi, 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. |
@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#L97
I 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:
The text was updated successfully, but these errors were encountered: