Ideas for serialization rework
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.
While working on #100 I noticed a couple other nit-picky things with Decode and Encode that I would like to fix:
- 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.
- The
CursorAPI is a bit awkward to work with, as you have to typecast usize to u64 in a few places. - It might be simpler for decoding to return
Option<_>instead ofError<_, CodecError>. (We pretty much never interpretCodecError.) - 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.
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 ofError<_, 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.
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.
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.