search

dropbox / pb-jelly

A protobuf code generation framework for the Rust language developed at Dropbox.
Apache License 2.0
611 stars 25 forks source link

Rework how zerocopy works, again #127

Closed goffrie closed 2 years ago

goffrie commented 2 years ago

This PR attempts to simplify the Lazy system again by removing more indirection. PbBuffer is now a more self-contained trait, without reference to PbBufferReader/Writer, and merely acts as a container. Instead, type-dispatch logic is pushed solely into PbBufferReader/PbBufferWriter.

goffrie commented 2 years ago

Deserializing from a slice of a type that contains a Lazy field succeeds, where it used to fail, but incurs a copy.

Yeah, I think this is generally the right behaviour. That being said, I think it should be up to the PbBufferReader/PbBufferWriter impl to decide. I'm going to change this a bit more to give more flexibility.

goffrie commented 2 years ago

I've changed it so that the PbBufferReader is responsible for constructing a PbBuffer via read_buffer, or falling back to copying if it wants to. impl PbBufferReader for Cursor<Bytes> does this fallback, but BlobReaderImpl probably won't.

nipunn1313 commented 2 years ago

update the CHANGELOG.md!

Particularly because this contains some function name changes. Give a summary on how to get from old code -> new code using the zerocopy feature.

Given that it's backward incompatible code-wise - also maybe we should do a non-patch version number bump to 0.1.0? Given that we're not 1.0.0, all rules are off, but it might be kind to do something like that.

nipunn1313 commented 2 years ago

Looks great BTW!

goffrie commented 2 years ago

Given that it's backward incompatible code-wise - also maybe we should do a non-patch version number bump to 0.1.0? Given that we're not 1.0.0, all rules are off, but it might be kind to do something like that.

Cargo uses the "first nonzero component" to decide version compatibility, so bumping 0.0.9->0.0.10 is already incompatible.

If you think it's a good time to "declare 0.1" though I wouldn't object.

ParkMyCar commented 2 years ago

I think we should move to 0.1 soon, but personally I'd like to wait for some open PRs to land. e.g. #121 . Also before we declare 0.1 we should make sure we can use pb-jelly as-is internally