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

protobuf: update all generated code to google.golang.org/protobuf #3932

Merged
merged 6 commits into from
Oct 21, 2020
Merged

protobuf: update all generated code to google.golang.org/protobuf #3932

merged 6 commits into from
Oct 21, 2020

Conversation

dfawley
Copy link
Member

@dfawley dfawley commented Oct 6, 2020

We should run some performance tests before merging this.

@dfawley dfawley requested a review from menghanl October 6, 2020 21:16
menghanl
menghanl reviewed Oct 6, 2020
View reviewed changes
vet.sh Outdated Show resolved Hide resolved
vet.sh Outdated Show resolved Hide resolved
menghanl
menghanl previously approved these changes Oct 6, 2020
View reviewed changes
vet.sh Outdated Show resolved Hide resolved
vet.sh Outdated Show resolved Hide resolved
@menghanl
Copy link
Contributor

menghanl commented Oct 6, 2020

route_guide test is failing:

client log missing output: location:<latitude:416851321 longitude:-742674555 >

New message's String() might not match what the script is greping for.

@dfawley
Copy link
Member Author

dfawley commented Oct 6, 2020

New message's String() might not match what the script is greping for.

Exactly that. I don't usually run the examples locally and just wait for travis. Fixed.

@menghanl menghanl mentioned this pull request Oct 7, 2020
Merged
@menghanl menghanl assigned easwars and dfawley and unassigned menghanl and easwars Oct 8, 2020
easwars added a commit to easwars/grpc-go that referenced this pull request Oct 8, 2020
@easwars
Update vet.sh from grpc#3932.
eb8dd91
@dfawley
Copy link
Member Author

dfawley commented Oct 8, 2020

I keep running benchmarks for longer and longer time periods, but keep seeing a consistent performance loss, with some anomalies in the data:

unary-networkMode_Local-bufConn_false-keepalive_false-benchTime_4m0s-trace_false-latency_0s-kbps_0-MTU_0-maxConcurrentCalls_2-reqSize_1B-respSize_1048576B-compressor_off-channelz_false-preloader_false
               Title       Before        After Percentage
            TotalOps       274056       270689    -1.23%
            Bytes/op   4609721.00   4612058.62     0.05%

streaming-networkMode_Local-bufConn_false-keepalive_false-benchTime_4m0s-trace_false-latency_0s-kbps_0-MTU_0-maxConcurrentCalls_2-reqSize_1048576B-respSize_1B-compressor_off-channelz_false-preloader_false
               Title       Before        After Percentage
            TotalOps       279797       275116    -1.67%
            Bytes/op   4667533.73   4665968.09    -0.03%

streaming-networkMode_Local-bufConn_false-keepalive_false-benchTime_4m0s-trace_false-latency_0s-kbps_0-MTU_0-maxConcurrentCalls_2-reqSize_1048576B-respSize_1048576B-compressor_off-channelz_false-preloader_false
               Title       Before        After Percentage
            TotalOps       156119       153513    -1.67%
            Bytes/op   8857367.74   8944954.30     0.99%

unconstrained-networkMode_Local-bufConn_false-keepalive_false-benchTime_4m0s-trace_false-latency_0s-kbps_0-MTU_0-maxConcurrentCalls_2-reqSize_1B-respSize_1048576B-compressor_off-channelz_false-preloader_false
               Title       Before        After Percentage
             SendOps    172920821    152882603   -11.59%
             RecvOps       172337       285348    65.58%
            Bytes/op      8970.36     15864.00    76.84%

unary-networkMode_Local-bufConn_false-keepalive_false-benchTime_4m0s-trace_false-latency_0s-kbps_0-MTU_0-maxConcurrentCalls_2-reqSize_1048576B-respSize_1B-compressor_off-channelz_false-preloader_false
               Title       Before        After Percentage
            TotalOps       261823       259976    -0.71%
            Bytes/op   4672066.54   4669787.24    -0.05%

unconstrained-networkMode_Local-bufConn_false-keepalive_false-benchTime_4m0s-trace_false-latency_0s-kbps_0-MTU_0-maxConcurrentCalls_2-reqSize_1048576B-respSize_1B-compressor_off-channelz_false-preloader_false
               Title       Before        After Percentage
            TotalOps            0            0      NaN%
             SendOps        15241        11337   -25.62%
             RecvOps    247065351    243122587    -1.60%
            Bytes/op      1265.19      1188.37    -6.09%

unary-networkMode_Local-bufConn_false-keepalive_false-benchTime_4m0s-trace_false-latency_0s-kbps_0-MTU_0-maxConcurrentCalls_2-reqSize_1048576B-respSize_1048576B-compressor_off-channelz_false-preloader_false
               Title       Before        After Percentage
            TotalOps       151819       149497    -1.53%
            Bytes/op   8842244.11   8923408.51     0.92%

unary-networkMode_Local-bufConn_false-keepalive_false-benchTime_4m0s-trace_false-latency_0s-kbps_0-MTU_0-maxConcurrentCalls_2-reqSize_1B-respSize_1B-compressor_off-channelz_false-preloader_false
               Title       Before        After Percentage
            TotalOps      3120422      3019656    -3.23%
            Bytes/op      8947.30      9110.93     1.82%

streaming-networkMode_Local-bufConn_false-keepalive_false-benchTime_4m0s-trace_false-latency_0s-kbps_0-MTU_0-maxConcurrentCalls_2-reqSize_1B-respSize_1B-compressor_off-channelz_false-preloader_false
               Title       Before        After Percentage
            TotalOps      4841823      4669642    -3.56%
            Bytes/op      1345.89      1420.53     5.57%

unconstrained-networkMode_Local-bufConn_false-keepalive_false-benchTime_4m0s-trace_false-latency_0s-kbps_0-MTU_0-maxConcurrentCalls_2-reqSize_1B-respSize_1B-compressor_off-channelz_false-preloader_false
               Title       Before        After Percentage
             SendOps    178788735    169663423    -5.10%
             RecvOps    178933175    179988331     0.59%
            Bytes/op       779.55       816.11     4.75%

streaming-networkMode_Local-bufConn_false-keepalive_false-benchTime_4m0s-trace_false-latency_0s-kbps_0-MTU_0-maxConcurrentCalls_2-reqSize_1B-respSize_1048576B-compressor_off-channelz_false-preloader_false
               Title       Before        After Percentage
            TotalOps       277134       271986    -1.86%
            Bytes/op   4639842.09   4627962.11    -0.26%

unconstrained-networkMode_Local-bufConn_false-keepalive_false-benchTime_4m0s-trace_false-latency_0s-kbps_0-MTU_0-maxConcurrentCalls_2-reqSize_1048576B-respSize_1048576B-compressor_off-channelz_false-preloader_false
               Title       Before        After Percentage
             SendOps        70254        63720    -9.30%
             RecvOps       354236       352059    -0.61%
            Bytes/op   6587804.58   6629572.79     0.63%

Looking again, it seems that the biggest anomalies are bidi-streaming, where the two sides just get different priorities between runs, so the results can vary more wildly. For unary, most cases show a ~1-2% performance loss in total RPCs over 4 minutes.

@dsnet @neild - are these kinds of results expected? Do you run benchmarks for the new generated code vs. the old generated code using the old library to serialize/deserialize? In case it makes a difference, we use the proto.Buffer implementation (ref), since that had the best performance (at the time, anyway).

@dsnet
Copy link
Contributor

dsnet commented Oct 8, 2020

The logic in grpc-go/encoding/proto/proto.go doesn't seem particularly well suited for the new module. I seem to recall that Damien looked at that in the past and had ideas for how to optimize it.

easwars added a commit to easwars/grpc-go that referenced this pull request Oct 8, 2020
@easwars
Update vet.sh from grpc#3932.
80c427e
@dfawley
Copy link
Member Author

dfawley commented Oct 8, 2020

The logic in grpc-go/encoding/proto/proto.go doesn't seem particularly well suited for the new module. I seem to recall that Damien looked at that in the past and had ideas for how to optimize it.

This was #3400. We can try to use proto.Marshal and Unmarshal as suggested, but we did run benchmarks when we started using proto.Buffer this way that showed a reasonable performance improvement due to reuse of the Buffer struct itself, even though we were not reusing the byte buffers themselves.

@dfawley dfawley mentioned this pull request Oct 14, 2020
Merged
@dfawley
Copy link
Member Author

dfawley commented Oct 16, 2020

After updating the proto codec implementation, this is showing a net win in performance of 2-4%:

unary-networkMode_Local-bufConn_false-keepalive_false-benchTime_1m0s-trace_false-latency_0s-kbps_0-MTU_0-maxConcurrentCalls_2-reqSize_1B-respSize_1048576B-compressor_off-channelz_false-preloader_false
               Title       Before        After Percentage
            TotalOps        65011        66782     2.72%
            Bytes/op   4278419.00   4278623.84     0.00%

unary-networkMode_Local-bufConn_false-keepalive_false-benchTime_1m0s-trace_false-latency_0s-kbps_0-MTU_0-maxConcurrentCalls_2-reqSize_1048576B-respSize_1B-compressor_off-channelz_false-preloader_false
               Title       Before        After Percentage
            TotalOps        62302        63795     2.40%
            Bytes/op   4278363.72   4275618.79    -0.06%

unary-networkMode_Local-bufConn_false-keepalive_false-benchTime_1m0s-trace_false-latency_0s-kbps_0-MTU_0-maxConcurrentCalls_2-reqSize_1048576B-respSize_1048576B-compressor_off-channelz_false-preloader_false
               Title       Before        After Percentage
            TotalOps        33689        34481     2.35%
            Bytes/op   8689701.23   8676134.94    -0.16%

unary-networkMode_Local-bufConn_false-keepalive_false-benchTime_1m0s-trace_false-latency_0s-kbps_0-MTU_0-maxConcurrentCalls_2-reqSize_1B-respSize_1B-compressor_off-channelz_false-preloader_false
               Title       Before        After Percentage
            TotalOps       742632       773159     4.11%
            Bytes/op      9226.18      9137.61    -0.96%

unary-networkMode_Local-bufConn_false-keepalive_false-benchTime_1m0s-trace_false-latency_0s-kbps_0-MTU_0-maxConcurrentCalls_2-reqSize_1B-respSize_1048576B-compressor_off-channelz_false-preloader_false
               Title       Before        After Percentage
            TotalOps        65261        67470     3.38%
            Bytes/op   4279335.87   4277865.44    -0.03%

unary-networkMode_Local-bufConn_false-keepalive_false-benchTime_1m0s-trace_false-latency_0s-kbps_0-MTU_0-maxConcurrentCalls_2-reqSize_1048576B-respSize_1B-compressor_off-channelz_false-preloader_false
               Title       Before        After Percentage
            TotalOps        62268        64100     2.94%
            Bytes/op   4279279.51   4277282.59    -0.05%

unary-networkMode_Local-bufConn_false-keepalive_false-benchTime_1m0s-trace_false-latency_0s-kbps_0-MTU_0-maxConcurrentCalls_2-reqSize_1048576B-respSize_1048576B-compressor_off-channelz_false-preloader_false
               Title       Before        After Percentage
            TotalOps        33508        34923     4.22%
            Bytes/op   8694425.09   8676746.56    -0.20%

unary-networkMode_Local-bufConn_false-keepalive_false-benchTime_1m0s-trace_false-latency_0s-kbps_0-MTU_0-maxConcurrentCalls_2-reqSize_1B-respSize_1B-compressor_off-channelz_false-preloader_false
               Title       Before        After Percentage
            TotalOps       746697       777178     4.08%
            Bytes/op      9226.57      9139.70    -0.94%

@dfawley dfawley assigned menghanl and unassigned dfawley Oct 21, 2020
@dfawley
Copy link
Member Author

dfawley commented Oct 21, 2020

@menghanl PTAL; I think this is now a good change to make.

FYI @chalin looks like we will be doing this after all! Once it's merged we can update the quick start instructions to go get the new protoc-gen-go version. Sorry to keep changing the plan.

menghanl
menghanl reviewed Oct 21, 2020
View reviewed changes
regenerate.sh
echo "go install github.com/golang/protobuf/protoc-gen-go"
(cd test/tools && go install github.com/golang/protobuf/protoc-gen-go)
echo "go install google.golang.org/protobuf/cmd/protoc-gen-go"
(cd test/tools && go install google.golang.org/protobuf/cmd/protoc-gen-go)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does the generated code still depend on "github.com/golang/protobuf/proto" with the codegen from google.golang.org/protobuf?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See golang/protobuf#1077 for why it originally existed. The dependency has since been removed, but won't be observable until we release v1.26.0.

@dfawley dfawley merged commit 4e8458e into grpc:master Oct 21, 2020
@dfawley dfawley deleted the protocver branch October 21, 2020 23:05
@easwars easwars mentioned this pull request Oct 21, 2020
Closed
lopezator pushed a commit to lopezator/grpc-go that referenced this pull request Oct 22, 2020
@dfawley @lopezator
protobuf: update all generated code to google.golang.org/protobuf (gr…
70beb7c 
…pc#3932)
lopezator pushed a commit to lopezator/grpc-go that referenced this pull request Oct 22, 2020
@dfawley @lopezator
protobuf: update all generated code to google.golang.org/protobuf (gr…
7b55947 
…pc#3932)
chalin added a commit to grpc/grpc.io that referenced this pull request Oct 22, 2020
@chalin
Go quick start: use protoc-gen-go from new pkg
a88c483 
Contributes to #437

Related: grpc/grpc-go#3932 (comment)
@chalin chalin mentioned this pull request Oct 22, 2020
Merged
@chalin
Copy link
Contributor

chalin commented Oct 22, 2020

@chalin looks like we will be doing this after all! Once it's merged we can update the quick start instructions...

Done, see grpc/grpc.io#475

chalin added a commit to grpc/grpc.io that referenced this pull request Oct 22, 2020
@chalin
Go quick start: use protoc-gen-go from new pkg (#475)
0b14ae1 
Contributes to #437. I've done the first part of the Quick start to confirm that it works.

Context: grpc/grpc-go#3932 (comment)
@tigrulya-exe tigrulya-exe mentioned this pull request Oct 28, 2020
Closed
GarrettGutierrez1 pushed a commit that referenced this pull request Nov 6, 2020
@dfawley @GarrettGutierrez1
protobuf: update all generated code to google.golang.org/protobuf (#3932
c1989b5 
)
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@dsnet dsnet dsnet left review comments

@menghanl menghanl menghanl approved these changes

Assignees

@menghanl menghanl

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@dfawley @menghanl @dsnet @chalin @easwars @GarrettGutierrez1