Skip to content
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

WIP: More efficient ProtobufNet serializer #6456

Conversation

icanhasjonas
Copy link
Contributor

@icanhasjonas icanhasjonas commented Apr 2, 2020

  • Reduced the number of buffer copies
  • Removed the length prefix/guard (possible breaking change)

These are improvements easily applied to the BinaryFormatter serializer too

Co-Authored-By: Kevin Cathcart <kevincathcart@gmail.com>
@jsteinich
Copy link
Contributor

The length prefix change is breaking since it changes what the 1st 4 bytes mean.
It should be pretty simple (and low perf impact) to make reading of old data still work, but I'm not sure there's a way to go the other direction.

Personally, I think the ability to read old data is critical, but having the old code be able to read new data doesn't seem very important.

return;
}

context.StreamWriter.Write(-1); // -1 should trigger an error in old deserializer
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... maybe I'll just use a MemoryStream and do TryGetBuffer() to get the supporting ArraySegment<byte> and then pass that over to the IBinaryTokenStreamWriter.. that way we should get minimal buffer copies while still being able to accurately write the length prefix

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.. and replace the WriteAdapter

@KevinCathcart
Copy link
Contributor

KevinCathcart commented Apr 3, 2020

Yeah, I agree back-compat is a concern, here.

Arguably a bigger concern is that protobuf does not have any size-of-message or end of message indication, so it will consume until the end of the stream. I think that could be a problem, since objects serialized with this could be embedded inside an object serialized with the default Orleans serializer, so it needs to stop before the end of the stream.

But protobufnet actually has built-in support for inputting and outputting exactly the format with the 32 bit length prefix, so we can use it to fix both concerns. It only cleanly exposed on the Serializer class when providing a type argument, but you can do it yourself with:

//Deserialize (This DOES handles nulls represented as 0 length.)
return RuntimeTypeModel.Default.DeserializeWithLengthPrefix(stream, value: null, type, style, fieldNumber: 0);

//serialize (unlike Deserialize, this does NOT handle nulls. Write 0 yourself for them.)
RuntimeTypeModel.Default.SerializeWithLengthPrefix(stream, value, type, PrefixStyle.Fixed32, fieldNumber: 0);

Now, you make think that on serialization this is just using a buffer inside protobuf, and well, you would be right. However this is a buffer it already uses, but we are still saving one round of buffer copying like this, although the returns would be diminished by that internal buffer growing more than it normally would. Still one fewer copies is still an improvement.

Deserialization does not have this problem, since it just tracks position, and will not read more than the number of bytes in the prefix. However, in this case, deserialization method will make use of ReadByte() for reading the length prefix, so you should probably implement that on the Adapter stream, to avoid the crazy inefficient base class implementation.

Yes, this is a supported protobufnet API. the serializer class is merely a user friendly front-end over the RuntimeTypeModel, but using it directly is both supported, and potentially necessary if you want to use a custom type model to decide which properties to serialize, and what number they get.

Base automatically changed from master to main March 15, 2021 14:46
@ReubenBond
Copy link
Member

ReubenBond commented Mar 24, 2022

An updated protobuf serializer implementation could be offered either as a separate package, or perhaps the efficiency improvements can be put behind a configuration switch to retain compatibility (unsure how viable that is). Regardless, I'll close this for now, but feel free to open a new one if this is still of interest. Also note that Orleans now includes a fast, version-tolerant serializer out of the box (in 4.0.0-preview1). It also now supports configurable serializers on a per-storage-provider basis (#7416), so hopefully external serializers are easier to implement in future.

@ReubenBond ReubenBond closed this Mar 24, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Dec 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants