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

encoding/proto: simplify & optimize proto codec #3958

Merged
merged 1 commit into from Oct 14, 2020
Merged

Conversation

@dfawley
Copy link
Contributor

@dfawley dfawley commented Oct 14, 2020

Adopt recommendation in #3400 to simplify the proto codec; it seems the buffer pool is no longer buying us any performance wins.

Performance #s:

First run:

unary-networkMode_Local-bufConn_false-keepalive_false-benchTime_1m0s-trace_false-latency_0s-kbps_0-MTU_0-maxConcurrentCalls_4-reqSize_1B-respSize_1B-compressor_off-channelz_false-preloader_false
               Title       Before        After Percentage
            TotalOps      1258048      1246113    -0.95%
            Bytes/op      9041.12      9030.22    -0.12%

unary-networkMode_Local-bufConn_false-keepalive_false-benchTime_1m0s-trace_false-latency_0s-kbps_0-MTU_0-maxConcurrentCalls_4-reqSize_1B-respSize_1048576B-compressor_off-channelz_false-preloader_false
               Title       Before        After Percentage
            TotalOps        79675        78736    -1.18%
            Bytes/op   4544492.46   4272822.58    -5.98%

unary-networkMode_Local-bufConn_false-keepalive_false-benchTime_1m0s-trace_false-latency_0s-kbps_0-MTU_0-maxConcurrentCalls_4-reqSize_1048576B-respSize_1B-compressor_off-channelz_false-preloader_false
               Title       Before        After Percentage
            TotalOps        75874        78201     3.07%
            Bytes/op   4547662.25   4271000.57    -6.08%

unary-networkMode_Local-bufConn_false-keepalive_false-benchTime_1m0s-trace_false-latency_0s-kbps_0-MTU_0-maxConcurrentCalls_4-reqSize_1048576B-respSize_1048576B-compressor_off-channelz_false-preloader_false
               Title       Before        After Percentage
            TotalOps        47519        47441    -0.16%
            Bytes/op   8773989.78   8576394.34    -2.25%

Second run:

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       743806       748614     0.65%
            Bytes/op      9233.55      9221.13    -0.13%

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        62939        66276     5.30%
            Bytes/op   4523063.20   4278421.65    -5.41%

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        60894        62828     3.18%
            Bytes/op   4553250.74   4277739.19    -6.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        33206        33723     1.56%
            Bytes/op   8903907.39   8693996.61    -2.36%

This appears to provide slightly better performance on average, and is considerably simpler, and would provide a lower memory footprint due to the lack of a buffer pool.

Related to #3932

@dfawley dfawley added this to the 1.34 Release milestone Oct 14, 2020
Copy link
Contributor

@menghanl menghanl left a comment

🎉

@dfawley dfawley merged commit 7b167fd into grpc:master Oct 14, 2020
3 checks passed
@dfawley dfawley deleted the protomarshal branch Oct 14, 2020
@alexshtin
Copy link

@alexshtin alexshtin commented Dec 10, 2020

I like this simplification but it causes error when trying to marshal nil: #4094

func (codec) Marshal(v interface{}) ([]byte, error) {
if pm, ok := v.(proto.Marshaler); ok {
Copy link

@morningsend morningsend Feb 5, 2021

This is breaking existing code generate with gogoproto.customtype = "MyType", where a user can write custom marshaling/unmarshaling code to have more control. IMHO should not be removed.

https://github.com/gogo/protobuf/blob/master/custom_types.md#custom-type-method-signatures

Copy link
Contributor Author

@dfawley dfawley Feb 5, 2021

gogoproto is not officially supported by gRPC-Go.

If you want a codec that suits your needs better, it's easy to install your own. Just copy/paste this code into another package, update the parts that are important, and import it in your main package after any grpc imports. (If you don't control main, you can give it a new name and use the Codec Call-/Dial-/Server- Options.)

Ok thanks, I will give it a try. I have also created an issue about this. Please feel free to close it #4192 I will also comment the solution there after testing it.

protoMsg := v.(proto.Message)
protoMsg.Reset()

if pu, ok := protoMsg.(proto.Unmarshaler); ok {

Same problem, removing this will break use case of gogoproto.customtype = "MyType"

stevendanna added a commit to stevendanna/cockroach that referenced this issue Feb 18, 2021
This change enables future upgrades of google.golang.org/grpc.

Recent changes to google.golang.org/grpc have changed the default
proto codec such that it is incompatible with gogoproto:

grpc/grpc-go#3958

While `proto.Marshal` does do a check for the Marshal/Unmarshal it
appears to do this through reflection methods that will panic on
fields using gogoproto.nullable.

To avoid this, we install a proto codec that does an interface check
before dispatching to proto.Marshal/proto.Unmarshal.

Note that I haven't replicated the protobuffer caching that previously
existed in google.golang.org/grpc. The referenced change claims
removing the caching may be a small performance win; however, they
also removed the typecheck that we need to do, so it is unclear.

Fixes cockroachdb#60531

Release note: None
stevendanna added a commit to stevendanna/cockroach that referenced this issue Mar 10, 2021
This change enables future upgrades of google.golang.org/grpc.

Recent changes to google.golang.org/grpc have changed the default
proto codec such that it is incompatible with gogoproto:

grpc/grpc-go#3958

While `proto.Marshal` does do a check for the Marshal/Unmarshal it
appears to do this through reflection methods that will panic on
fields using gogoproto.nullable.

To avoid this, we install a proto codec that does an interface check
before dispatching to proto.Marshal/proto.Unmarshal.

Note that I haven't replicated the protobuffer caching that previously
existed in google.golang.org/grpc. The referenced change claims
removing the caching may be a small performance win; however, they
also removed the typecheck that we need to do, so it is unclear.

Fixes cockroachdb#60531

Release note: None
stevendanna added a commit to stevendanna/cockroach that referenced this issue Mar 18, 2021
This change enables future upgrades of google.golang.org/grpc.

Recent changes to google.golang.org/grpc have changed the default
proto codec such that it is incompatible with gogoproto:

grpc/grpc-go#3958

While `proto.Marshal` does do a check for the Marshal/Unmarshal it
appears to do this through reflection methods that will panic on
fields using gogoproto.nullable.

To avoid this, we install a proto codec that does an interface check
before dispatching to proto.Marshal/proto.Unmarshal.

Note that I haven't replicated the protobuffer caching that previously
existed in google.golang.org/grpc. The referenced change claims
removing the caching may be a small performance win; however, they
also removed the typecheck that we need to do, so it is unclear.

Fixes cockroachdb#60531

Release note: None
stevendanna added a commit to stevendanna/cockroach that referenced this issue Mar 23, 2021
This change enables future upgrades of google.golang.org/grpc.

Recent changes to google.golang.org/grpc have changed the default
proto codec such that it is incompatible with gogoproto:

grpc/grpc-go#3958

While `proto.Marshal` does do a check for the Marshal/Unmarshal it
appears to do this through reflection methods that will panic on
fields using gogoproto.nullable.

To avoid this, we install a proto codec that does an interface check
before dispatching to proto.Marshal/proto.Unmarshal.

Note that I haven't replicated the protobuffer caching that previously
existed in google.golang.org/grpc. The referenced change claims
removing the caching may be a small performance win; however, they
also removed the typecheck that we need to do, so it is unclear.

Fixes cockroachdb#60531

Release note: None
craig bot pushed a commit to cockroachdb/cockroach that referenced this issue Mar 23, 2021
61333: rpc: install custom proto codec r=[erikgrinaker,pbardea] a=stevendanna

This change enables future upgrades of google.golang.org/grpc.

Recent changes to google.golang.org/grpc have changed the default
proto codec such that it is incompatible with gogoproto:

grpc/grpc-go#3958

While `proto.Marshal` does do a check for the Marshal/Unmarshal it
appears to do this through reflection methods that will panic on
fields using gogoproto.nullable.

To avoid this, we install a proto codec that does an interface check
before dispatching to proto.Marshal/proto.Unmarshal.

Note that I haven't replicated the protobuffer caching that previously
existed in google.golang.org/grpc. The referenced change claims
removing the caching may be a small performance win; however, they
also removed the typecheck that we need to do, so it is unclear.

Fixes #60531

Release note: None

62118: sql: revive BenchmarkTracing r=irfansharif a=irfansharif

Fixes #62081. It had rotted after we removed the trace.mode setting. It
now makes use of a testing knob if enabled, installs real spans all sql
statements. We'll defer the actual investigation into the cause of the
slow-down in future PRs (prototyped in #62227).

    $ make bench PKG=./pkg/bench  TESTFLAGS='-benchtime=10000x -count 20' \
        BENCHES='BenchmarkTracing' BENCHTIMEOUT=50m > benchmark-tracing.txt
    $ cat benchmark-tracing.txt | grep -v tracing=t | sed 's/tracing=f//' > old.txt
    $ cat benchmark-tracing.txt | grep -v tracing=f | sed 's/tracing=t//' > new.txt
    $ benchstat old.txt new.txt

    name                                   old time/op    new time/op    delta
    Tracing/Cockroach//Scan1-24               382µs ± 2%     412µs ± 8%   +7.68%  (p=0.000 n=10+10)
    Tracing/Cockroach//Insert-24              579µs ± 2%     609µs ± 6%   +5.10%  (p=0.000 n=10+10)
    Tracing/MultinodeCockroach//Scan1-24      799µs ± 2%     885µs ± 1%  +10.76%  (p=0.000 n=10+9)
    Tracing/MultinodeCockroach//Insert-24    1.09ms ± 1%    1.14ms ± 2%   +5.04%  (p=0.000 n=9+10)

    name                                   old alloc/op   new alloc/op   delta
    Tracing/Cockroach//Scan1-24              25.3kB ± 5%    32.0kB ± 4%  +26.55%  (p=0.000 n=10+10)
    Tracing/Cockroach//Insert-24             38.0kB ± 6%    42.2kB ± 5%  +11.02%  (p=0.000 n=10+10)
    Tracing/MultinodeCockroach//Scan1-24     77.7kB ± 8%    97.1kB ±12%  +25.05%  (p=0.000 n=10+10)
    Tracing/MultinodeCockroach//Insert-24     107kB ± 8%     115kB ± 7%   +7.66%  (p=0.023 n=10+10)

    name                                   old allocs/op  new allocs/op  delta
    Tracing/Cockroach//Scan1-24                 256 ± 1%       280 ± 1%   +9.10%  (p=0.000 n=9+9)
    Tracing/Cockroach//Insert-24                301 ± 2%       321 ± 1%   +6.64%  (p=0.000 n=10+10)
    Tracing/MultinodeCockroach//Scan1-24        787 ± 2%       921 ± 2%  +17.04%  (p=0.000 n=9+9)
    Tracing/MultinodeCockroach//Insert-24       748 ± 1%       805 ± 2%   +7.61%  (p=0.000 n=9+9)

Release note: None

62448: Update README.md r=j-low a=kannanlakshmi

Updated quick start section to include a link to CockroachCloud

Co-authored-by: Steven Danna <danna@cockroachlabs.com>
Co-authored-by: irfan sharif <irfanmahmoudsharif@gmail.com>
Co-authored-by: Lakshmi Kannan <36172966+kannanlakshmi@users.noreply.github.com>
craig bot pushed a commit to cockroachdb/cockroach that referenced this issue Mar 23, 2021
61333: rpc: install custom proto codec r=[erikgrinaker,pbardea] a=stevendanna

This change enables future upgrades of google.golang.org/grpc.

Recent changes to google.golang.org/grpc have changed the default
proto codec such that it is incompatible with gogoproto:

grpc/grpc-go#3958

While `proto.Marshal` does do a check for the Marshal/Unmarshal it
appears to do this through reflection methods that will panic on
fields using gogoproto.nullable.

To avoid this, we install a proto codec that does an interface check
before dispatching to proto.Marshal/proto.Unmarshal.

Note that I haven't replicated the protobuffer caching that previously
existed in google.golang.org/grpc. The referenced change claims
removing the caching may be a small performance win; however, they
also removed the typecheck that we need to do, so it is unclear.

Fixes #60531

Release note: None

Co-authored-by: Steven Danna <danna@cockroachlabs.com>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants