tafia / quick-protobuf

A rust implementation of protobuf parser
MIT License
452 stars 87 forks source link

Custom Cow type support #62

Open koivunej opened 7 years ago

koivunej commented 7 years ago

I've been successfully using quick-protobuf with tokio. However there are a few problems with tokio-io codecs which only allow the use of 'static data. This means I need to convert all parsed structs into owned, which much manual labour or with my new derive-into-owned.

Optimal way to parse protobuf messages with tokio would be to allow reading straight from BytesMut and to store any values as some kind of wrappers against BytesMut (or even downgraded readonly version, does not really matter) or even as BytesMut or Vec length on-stack versions (both are something like 3 usizes, which would be 12 bytes for 32-bit or even more for 64-bit architectures).

This would however require side-stepping the plain std::borrow::Cow a lot. Since there might be even more interesting user specific needs (for example, using some mmap resolved slices) in addition to tokio/bytes usage, there should probably be an option to use user provided Cow-type.

Quickly looking at the generated code, the "custom Cow type" would have to be something like:

These could probably all be just duck-typed by specifying the "CowType" as a command line option but BytesReader would have to be enhanced to split_to or at least allow access to the underlying container. It might already have a get_ref or something like that.

Filing this issue here to discuss this more. Would changes supporting this kind of option be welcome?

tafia commented 7 years ago

Optimal way to parse protobuf messages with tokio would be to allow reading straight from BytesMut

I didn't know about bytes crate existence. This looks really useful!

I really welcome such changes even if I am not sure I am the best person to handle it. It looks much lower level than what I'm used to!

nerdrew commented 6 years ago

I've just run into this issue as well. I've been playing with both warp and tower-web and both use the bytes crate.

I've been using the rental crate so I can have a struct that holds both a buffer and the parsed proto structs, but if I need to translate from one proto to another, I need to "promote" (not sure what the right rust terminology is for to_owned) the Cow::Borrowed fields to Cow::Owned.

I think a build option to use bytes and string for those fields instead of Cow would be a nice API. It would complicate the internals a bit though. If you are still ok with such changes, I might take a look at adding a config option (maybe behind a build feature flag).

I haven't used the bytes crate, but I asked (some possibly unclear) questions about what the implementation would need here.

nerdrew commented 5 years ago

Would you be open to a PR that adds an optional crate feature that enables Bytes instead of Cow<&'a [u8]> and the same for strings?

tafia commented 5 years ago

Definitely!

nerdrew commented 5 years ago

I see that support for String and Vec was merged already. I didn't open a PR for bytes because I thought the performance hit was too large.

tafia commented 5 years ago

I think the best is to let people decide if they want performance or convenience, the default being performance.

nerdrew commented 5 years ago

Makes sense to me. I'll see if I can rebase and cleanup my bytes commit as some point.