-
Notifications
You must be signed in to change notification settings - Fork 4.1k
proto: disable XXX_NoUnkeyedLiteral and XXX_sizecache fields #37706
Description
For the reasons listed in the last two commits of the following branch, we would like to disable the autogenerated XXX_NoUnkeyedLiteral field and the XXX_sizecache field in Go protobufs:
https://github.com/nvanbenschoten/cockroach/commits/nvanbenschoten/sizeCache
However, this seems to cause a strange performance regression. To reproduce this, apply the following patch:
diff --git a/pkg/cmd/protoc-gen-gogoroach/main.go b/pkg/cmd/protoc-gen-gogoroach/main.go
index b47507f..88f6ec8 100644
--- a/pkg/cmd/protoc-gen-gogoroach/main.go
+++ b/pkg/cmd/protoc-gen-gogoroach/main.go
@@ -80,7 +80,14 @@ func main() {
// Something something extensions; we don't use 'em currently.
// vanity.TurnOffGoExtensionsMapAll,
- vanity.TurnOffGoUnrecognizedAll,
+ // Disable generation of the following fields, which aren't worth
+ // their associated runtime cost:
+ // - XXX_unrecognized
+ // - XXX_NoUnkeyedLiteral
+ // - XXX_sizecache
+ vanity.TurnOffGoUnrecognizedAll,
+ vanity.TurnOffGoUnkeyedAll,
+ vanity.TurnOffGoSizecacheAll,
// Adds unnecessary dependency on golang/protobuf.
// vanity.TurnOffGogoImport,
Build cockroach using ./build/builder.sh mkrelease linux-gnu. Finally, run roachtest run 'kv95/enc=false/nodes=3$'. Without the patch, throughput should hover around 32k qps. With the patch, throughput drops to around 13k qps.
The goal here is to figure out why. There must be an obvious explanation for this, but I haven't been able to find it. Debugging may require a combination of debugging the performance of running clusters and exploring micro-benchmarks.