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

Ideas for serialization rework #15

Closed
cjpatton opened this issue Jun 23, 2022 · 4 comments
Closed

Ideas for serialization rework #15

cjpatton opened this issue Jun 23, 2022 · 4 comments

Comments

@cjpatton
Copy link
Contributor

We currently use libprio's codec::Decode trait for decoding all messages in the DAP protocol. Currently all fields are owned by the struct, i.e., no fields reference data somewhere else. This means that, when decoding a message from the wire, it is necessary to copy bytes into the struct fields. This is fine for short messages, but some messages are quite long. In particular, we would like to be able to decode aggregate requests and responses and have the corresponding structs hold a reference to the underlying data.

@cjpatton
Copy link
Contributor Author

cjpatton commented Sep 16, 2022

While working on #100 I noticed a couple other nit-picky things with Decode and Encode that I would like to fix:

  1. We have run into message length limits, which suggests that it would be useful if encoding could fail gracefully. The parallel nature of the aggregation flow gives one the impression that an arbitrary number of reports can be processed at once. In fact, this would breach size limits quite quickly in DAP-01.
  2. The Cursor API is a bit awkward to work with, as you have to typecast usize to u64 in a few places.
  3. It might be simpler for decoding to return Option<_> instead of Error<_, CodecError>. (We pretty much never interpret CodecError.)
  4. Length-prefixed encoding is a bit awkward, since we have to allocate space for the prefix, then go back and write the prefix. I reallly like how Go solved this problem with the cryptobyte API -- I wonder if we could do the same thing here, withhout compromising the explicitness of what we have today.

@cjpatton cjpatton changed the title Daphne: Zero-copy decoding for longer messages Ideas for serialization rework Sep 16, 2022
@tgeoghegan
Copy link

Enabling zero-copy decoding is a good direction to take. I did the simplest thing that would work in the current iteration of the codec module, following the unofficial Rust programmer guidance of avoiding references and tricky lifetime problems until you have a real enough performance problem to motivate it. I'm also happy to move away from Cursor. Possibly that trait could be generic over std::io::Read (which Cursor<Vec<u8>> does implement), but I'm guessing I punted on the complex generic signature that would entail for the same reason I chickened out of dealing with references.

It might be simpler for decoding to return Option<_> instead of Error<_, CodecError>. (We pretty much never interpret CodecError.)

Applications that don't want the error can use std::result::Result::ok to convert to an Option. I don't think prio should force all its clients to discard a potentially interesting error string.

@branlwyd
Copy link
Contributor

branlwyd commented Sep 19, 2022

I would really like using std::io::Read rather than a Cursor. The one downside that comes to mind is that reading from a Cursor can't fail due to I/O errors, while a general Read implementation can encounter arbitrary I/O errors, so we'd need to deal with the complexity this induces[1]. OTOH, I think using Read would allow us to wire our decoding directly with reading bytes off the wire, which might sidestep the need to implement the (quite complex, I think) changes required to allow message types to either own or reference their data.

IMO, if we decide we want to avoid unnecessarily reading all of the data before parsing and/or making unnecessary copies, using Read would be a relatively simple way to do so. (I'm pretty sure we won't end up with true zero-copy with this change, since we'll in practice be buffering I/O -- that's OK by me, the buffer will be of fixed size, and I am unconvinced true zero-copy is justified just yet.)

[1] The codec.rs implementations would likely just pass these to the caller, perhaps after appropriately wrapping them in CodecError; I think most of the complexity would fall on users of codec.rs which would need to care about these error conditions.

@branlwyd
Copy link
Contributor

One additional idea that fell out of a discussion this morning: codec could provide derive-based macros providing derived implementations of Encode & Decode. While Rust protects against the truly nasty memory-safety issues that tend to arise with binary-parsing code, these implementations are still somewhat tricky and error-prone. And the implementations tend to be mostly boilerplate.

Instead, we could simply provide the necessary support such that relevant types could #[derive(Encode, Decode)] & get the expected implementations as generated code.

We'll need some way to indicate the "size" (u16, u24, u32, etc) of variable-length vectors.

This functionality would ideally be integrated into Serde, but I am not sure Serde provides enough support to let us indicate the size of variable-length vectors. Failing Serde, we could implement our own derivation implementations.

@cjpatton cjpatton closed this as not planned Won't fix, can't repro, duplicate, stale Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants