Skip to content

Commit

Permalink
Merge #38404
Browse files Browse the repository at this point in the history
38404: proto: disable XXX_NoUnkeyedLiteral and XXX_sizecache fields r=nvanbenschoten a=nvanbenschoten

Fixes #37706.

The PR removes the `XXX_NoUnkeyedLiteral` and `XXX_sizecache` fields from generated protobufs.

`XXX_NoUnkeyedLiteral` claims to come with a small perf win for proto marshaling, 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, it 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.

After removing `XXX_NoUnkeyedLiteral`, `XXX_sizecache` 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.

### Benchmarks

```
name                    old reads/s   new reads/s   delta
kv95/enc=false/nodes=3    35.5k ± 1%    35.8k ± 1%  +0.86%  (p=0.214 n=5+5)

name                    old writes/s  new writes/s  delta
kv95/enc=false/nodes=3    1.87k ± 2%    1.89k ± 2%  +0.94%  (p=0.214 n=5+5)
```

Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
  • Loading branch information
craig[bot] and nvanbenschoten committed Jun 25, 2019
2 parents 6ccc447 + 95878e0 commit d7511f1
Show file tree
Hide file tree
Showing 72 changed files with 5,741 additions and 6,941 deletions.
79 changes: 37 additions & 42 deletions pkg/acceptance/cluster/testconfig.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

55 changes: 27 additions & 28 deletions pkg/build/info.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading

0 comments on commit d7511f1

Please sign in to comment.