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

roachpb: Protobuf encoding does not omit empty messages #85312

Closed
erikgrinaker opened this issue Jul 29, 2022 · 2 comments
Closed

roachpb: Protobuf encoding does not omit empty messages #85312

erikgrinaker opened this issue Jul 29, 2022 · 2 comments
Labels
A-kv Anything in KV that doesn't belong in a more specific category. C-performance Perf of queries or internals. Solution not expected to change functional behavior. T-kv KV Team

Comments

@erikgrinaker
Copy link
Contributor

erikgrinaker commented Jul 29, 2022

As seen over in #85138 (comment), it appears that gogoproto is encoding embedded messages even though they are empty. In other words, consider this message:

message Foo {
  Timestamp timestamp = 1; 
}

message Timestamp {
  int64 wall_time = 1;
  int32 logical = 2;
  bool synthetic = 3;
}

Even though empty Protobuf fields can be omitted, gogoproto will encode an empty Foo into 2 bytes: 0x0a 0x00, which essentially says field 1 of wire type delimited with length 0 (see encoding spec).

The encoding should be able to omit this field entirely, which might net us some minor savings throughout the codebase (e.g. for every unset timestamp field).

Jira issue: CRDB-18178

@erikgrinaker erikgrinaker added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. A-kv Anything in KV that doesn't belong in a more specific category. T-kv KV Team labels Jul 29, 2022
@erikgrinaker erikgrinaker added C-performance Perf of queries or internals. Solution not expected to change functional behavior. and removed C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. labels Jul 29, 2022
@erikgrinaker
Copy link
Contributor Author

@nvanbenschoten points out that this may only happen for non-nullable fields, and refers to ResponseHeader.MarshalToSizedBuffer and it's handling of the Txn field.

@erikgrinaker
Copy link
Contributor Author

Yeah, this is a gogoproto-ism.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-kv Anything in KV that doesn't belong in a more specific category. C-performance Perf of queries or internals. Solution not expected to change functional behavior. T-kv KV Team
Projects
None yet
Development

No branches or pull requests

1 participant