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.

Sign up for GitHub

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

Jump to bottom

proto: disable XXX_NoUnkeyedLiteral and XXX_sizecache fields #38404

Merged
merged 2 commits into from
Jun 25, 2019
Merged

proto: disable XXX_NoUnkeyedLiteral and XXX_sizecache fields #38404

merged 2 commits into from
Jun 25, 2019

Commits on Jun 25, 2019

  1. protoc: disable XXX_sizecache generation in protos 

    The field claims to come with a small perf win for proto marshalling,
but it looks like it's only used with `proto.Marshal` and not with the
`Marshal` method (which CRDB code and gRPC code always use).

Meanwhile, the cache has two moderate disadvantages:
1. it adds 32 bits to every proto struct. This isn't a big deal in most
   cases, but in code that allocates large slices of little proto structs
   (like the `spanlatch.Manager`, which is what tipped me off to this new
   field), it has a non-negligible cost.
2. it makes value comparison between proto structs dangerously unreliable.
   We compare small protos by value all over the place (see `hlc.Timestamp`)
   and switching to an `Equals` method would come with a cost to code
   complexity and runtime performance.

All in all, the `XXX_sizecache` field doesn't seem to be worth it.

Release note: None
    @nvanbenschoten
    nvanbenschoten committed Jun 25, 2019
    Configuration menu
    Copy the full SHA
    1967c25 View commit details
    Browse the repository at this point in the history

  2. protoc: disable XXX_NoUnkeyedLiteral generation in protos 

    Because the field is now at the end of generated protos, it is now
taking up space even though it is an empty struct:
golang/go#9401

Since this is just a glorified lint which is already enforced by
`go vet`'s “composites” check, it doesn't seem worth the cost.

Release note: None
    @nvanbenschoten
    nvanbenschoten committed Jun 25, 2019
    Configuration menu
    Copy the full SHA
    95878e0 View commit details
    Browse the repository at this point in the history