-
Notifications
You must be signed in to change notification settings - Fork 4.7k
grpc: support per-message compression and enforce embedding in ServerTransportStream implementations #8972
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
base: master
Are you sure you want to change the base?
grpc: support per-message compression and enforce embedding in ServerTransportStream implementations #8972
Changes from 4 commits
e311529
846f339
b1e38ee
1feab8c
a7ff095
99f8640
731535b
489f488
9e906cb
a8bf7be
04e6bdc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,6 +21,7 @@ package grpc | |
| import ( | ||
| "context" | ||
| "errors" | ||
| "fmt" | ||
| "io" | ||
| "math" | ||
| rand "math/rand/v2" | ||
|
|
@@ -51,6 +52,33 @@ import ( | |
|
|
||
| var metadataFromOutgoingContextRaw = internal.FromOutgoingContextRaw.(func(context.Context) (metadata.MD, [][]string, bool)) | ||
|
|
||
| // SetMessageCompression enables or disables per-message compression on a stream | ||
| // if a compressor is specified for the stream (e.g. using UseCompressor or | ||
| // SetSendCompressor) and if the encoding type is supported by the receiver. | ||
| // By default, message compression is enabled, but is a no-op if compression | ||
| // is not enabled on the stream. | ||
| // | ||
| // This method must not be called concurrently with SendMsg. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we instead have the I don't expect people to invoke this method repeatedly to cause any performance concerns. Therefore, simplifying the API seems like a better win.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this is advisable. If a call to Sorry for the conflicting advice. |
||
| // | ||
| // # Experimental | ||
| // | ||
| // Notice: This API is EXPERIMENTAL and may be changed or removed in a | ||
| // later release. | ||
| func SetMessageCompression(ctx context.Context, enable bool) error { | ||
|
arjan-bal marked this conversation as resolved.
Outdated
|
||
| opts, ok := ctx.Value(compressKey{}).(*compressOptions) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are a few things here that I'm not convinced about:
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the suggestion! I thought about this approach too, but returning a new context doesn't work for streams.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As you mentioned, I wanted to share my thinking on the compressKey overhead. Since SetMessageCompression is already a no-op without a compressor, I considered adding it only when a compressor is actually set. But the tricky part is when SetSendCompressor is called first — the compressor isn't known at stream creation time. So I kept it as is for now. Happy to explore this further if you think it's worth optimizing.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One more thought — to address the mutability concern, I'm planning to
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Presently we have ~24 allocs/op for streaming RPCs and ~140 allocs/op for unary RPCs for client and server side combined. Even a single extra heap allocation does show up as a <1% QPS drop in our benchmarks when sending very small messages. Here is how to run the benchmarks for unary RPCs: We should ensure that users not utilizing the new
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I ran the benchmarks and the results confirm your concern:
I'll fix this by treating the absence of compressKey as
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated the implementation to address the allocation concern. Server side: moved doNotCompress into transport.ServerStream directly, no context allocation needed. Benchmark results (unary, compression=off): Benchmark results (unary, compression=gzip): The +2 allocs in gzip case is expected since compressKey is only added when a compressor is set. @arjan-bal @easwars
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for optimizing this. The results seem good. |
||
| if !ok || opts == nil { | ||
| return fmt.Errorf("grpc: SetMessageCompression called on an uninitialized or non-stream context") | ||
|
arjan-bal marked this conversation as resolved.
Outdated
|
||
| } | ||
| opts.DoNotCompress = !enable | ||
| return nil | ||
| } | ||
|
|
||
| type compressOptions struct { | ||
| DoNotCompress bool | ||
|
Dostonlv marked this conversation as resolved.
Outdated
|
||
| } | ||
|
|
||
| type compressKey struct{} | ||
|
|
||
| // StreamHandler defines the handler called by gRPC server to complete the | ||
| // execution of a streaming RPC. srv is the service implementation on which the | ||
| // RPC was invoked. | ||
|
|
@@ -299,6 +327,7 @@ func newClientStreamWithParams(ctx context.Context, desc *StreamDesc, cc *Client | |
| } else { | ||
| ctx, cancel = context.WithCancel(ctx) | ||
| } | ||
| ctx = context.WithValue(ctx, compressKey{}, &compressOptions{}) | ||
| defer func() { | ||
| if err != nil { | ||
| cancel() | ||
|
|
@@ -954,7 +983,11 @@ func (cs *clientStream) SendMsg(m any) (err error) { | |
| } | ||
|
|
||
| // load hdr, payload, data | ||
| hdr, data, payload, pf, err := prepareMsg(m, cs.codec, cs.compressorV0, cs.compressorV1, cs.cc.dopts.copts.BufferPool) | ||
| compV0, compV1 := cs.compressorV0, cs.compressorV1 | ||
| if opts, ok := cs.ctx.Value(compressKey{}).(*compressOptions); ok && opts.DoNotCompress { | ||
| compV0, compV1 = nil, nil | ||
| } | ||
| hdr, data, payload, pf, err := prepareMsg(m, cs.codec, compV0, compV1, cs.cc.dopts.copts.BufferPool) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
@@ -1461,7 +1494,11 @@ func (as *addrConnStream) SendMsg(m any) (err error) { | |
| } | ||
|
|
||
| // load hdr, payload, data | ||
| hdr, data, payload, pf, err := prepareMsg(m, as.codec, as.sendCompressorV0, as.sendCompressorV1, as.ac.dopts.copts.BufferPool) | ||
| compV0, compV1 := as.sendCompressorV0, as.sendCompressorV1 | ||
| if opts, ok := as.ctx.Value(compressKey{}).(*compressOptions); ok && opts.DoNotCompress { | ||
| compV0, compV1 = nil, nil | ||
| } | ||
| hdr, data, payload, pf, err := prepareMsg(m, as.codec, compV0, compV1, as.ac.dopts.copts.BufferPool) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
@@ -1741,7 +1778,11 @@ func (ss *serverStream) SendMsg(m any) (err error) { | |
| } | ||
|
|
||
| // load hdr, payload, data | ||
| hdr, data, payload, pf, err := prepareMsg(m, ss.codec, ss.compressorV0, ss.compressorV1, ss.p.bufferPool) | ||
| compV0, compV1 := ss.compressorV0, ss.compressorV1 | ||
| if opts, ok := ss.ctx.Value(compressKey{}).(*compressOptions); ok && opts.DoNotCompress { | ||
| compV0, compV1 = nil, nil | ||
| } | ||
| hdr, data, payload, pf, err := prepareMsg(m, ss.codec, compV0, compV1, ss.p.bufferPool) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
|
arjan-bal marked this conversation as resolved.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is executed before the server handler is invoked. This would mean that stream.IsCompressionEnabled would always return true. Am I missing something here? Do we really need this check here?
And if we really need this check, why is it missing for the streaming case?