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

rust-protobuf version 3 #518

Closed
11 of 12 tasks
stepancheg opened this issue Aug 12, 2020 · 4 comments
Closed
11 of 12 tasks

rust-protobuf version 3 #518

stepancheg opened this issue Aug 12, 2020 · 4 comments
Labels
rust-protobuf-v3 Issued to be fixed to release version 3

Comments

@stepancheg
Copy link
Owner

stepancheg commented Aug 12, 2020

Issues to be fixed to release version 3

Version 3 alpha versions are published. docs.rs

Open questions:

  • Is current proto file placing OK? I. e. all files become first-level modules regardless of input directory structure and package declaration. I. e. foo/bar.proto becomes mod bar
  • Do we need to rename .proto member to rust idiomatic names? I. e. do we need to rename enum variant FOO to Foo?
  • Should we use raw identifiers for reserved names? Currently proto field type becomes field_type in generated code. Perhaps it should be r#type.
  • What about MessageField type? It is convenient, but isn't it too much mental overhead to work with?
  • Should we get rid of hack around figuring out how place messages in oneofs? They are boxed when there's a loop, and unboxed otherwise. I'm in favor of unconditionally making them boxed. Maybe later provide codegen option to store then unboxed.
  • Is codegen customize callback API convenient?
  • Shouldn't we pull JSON/text format parsing and serialization out of protobuf crate? For JSON we would be able to use third-party JSON parser more easily
  • should we restrict generated code only to current version of rust-protobuf? Or guarantee forward compatibility with generated code as rust-protobuf v2 did? Not guaranteeing compatibility would make future changes around reflection much easier.
@stepancheg stepancheg pinned this issue Aug 12, 2020
@stepancheg stepancheg added the rust-protobuf-v3 Issued to be fixed to release version 3 label Aug 13, 2020
@saethlin
Copy link
Contributor

I'm looking at a ~22% perf regression from 2.18.1 to the current master due to the changes in the semantics of Clear. Previously, I could use that trait to reuse all of a message's internal Vecs, but not anymore. It looks like this is caused by the change from SingularField to Option, which seems pretty deliberate but the docs for MessageField::clear still document the old behavior, which now contradicts the implementation:

/// Clear this object, but do not call destructor of underlying data.

Is re-establishing the old Clear semantics a TODO item? Or is this some consequence of proto3 😕

@stepancheg
Copy link
Owner Author

@saethlin thank you for the feedback!

We can more or less easily restore old Clear semantics of MessageField (because it is already a new type).

But the issue is with RepeatedField. Current version of rust-protobuf 3 unconditionally uses Vec for repeated fields, and respectively does not preserve allocated objects. It was done because for users it is not always convenient to use RepeatedField and convert it to and from Vec.

So trade-off here is between:

  • significantly faster performance when clear is used, and it is used rarely
  • mental and syntax overhead working with RepeatedField

I honestly don't know what would be the right solution.

@saethlin
Copy link
Contributor

Would it be too much mental overhead to have Clear declare that it only retains the first level of indirection? Personally I don't mind the mental model being too odd because this is entirely an optimization. But maybe the opinion of someone who actually uses proto3 would matter more.

@stepancheg
Copy link
Owner Author

Would it be too much mental overhead to have Clear declare that it only retains the first level of indirection?

You meant, only singular nested messages preserved, but not repeated nested messages? It can be done, but it will be only partial benefit, and still some complication: MessageField Option<Box<T>> field is public now. If it meant to preserve allocation, field should be private, somewhat less convenient.

BTW is protobuf really meant to be super-fast? I suspect for really good performance, probably flatbuffers or capnproto should be used (where "deserialization" is no-op).

But maybe the opinion of someone who actually uses proto3 would matter more.

The issue is the same in proto2 and proto3. Singular and repeated message fields work the same in proto2 and proto3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rust-protobuf-v3 Issued to be fixed to release version 3
Projects
None yet
Development

No branches or pull requests

2 participants