Skip to content

Conversation

ChristopherHaas
Copy link

Currently, the ISerializer and IAsyncSerializer interfaces force an implementation to return a byte array with exact size. This approach effectively makes pooling impossible and often requires expensive memory copies and additional allocations in serializer implementations. See ProtobufSerializer as an example.

This PR changes the memory management for serialization. Instead of relying on serializer implementations to return a byte array, they are given an IBufferWriter to write to.

void Serialize(T data, SerializationContext context, IBufferWriter<byte> bufferWriter);

The producer implementation will then supply a buffer writer implementation that internally uses ArrayPool to allocate the memory. Users can overwrite this implementation by setting a custom ISerializationBufferProvider.

@mhowlett
Copy link
Contributor

Nice work @ChristopherHaas - apologies for the delayed response.

One important thing to note is we can't break the API - the old ISerializer<T> is going to need to stay - we'll need to support both types of serializer.

The other thing worth noting is due to the size, there could be a delay in getting this properly reviewed / merged (as you have already no doubt noticed). The time taken will depend on how much I agree with you. If there is too much disagreeing, it'll likely stall. Based on a quick look, it is seeming good to me so far.

So I'd encourage you to make changes to make this non-breaking, and hopefully I can get to a proper review before too long.

@cla-assistant
Copy link

cla-assistant bot commented Aug 7, 2023

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants