-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
use proto.Buffer API for protobuf codec and cache proto.Buffer structs #1010
Changes from 1 commit
e946d53
f8e2d0b
d9756fc
3ab447e
3a7b0e8
0f18cff
0b1ccd4
a1e068f
a21119d
e16c62c
586bba2
510b880
34bf3db
c8c475c
8431a0d
f2b8177
225b3ae
f841695
776137a
93d4020
d30dcba
126640b
18485fe
93c3e12
5ef6c9b
42d712a
1517ac9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
… cap
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,12 +34,15 @@ | |
package grpc | ||
|
||
import ( | ||
"math" | ||
"sync" | ||
|
||
"github.com/golang/protobuf/proto" | ||
) | ||
|
||
// Codec defines the interface gRPC uses to encode and decode messages. | ||
// Note that implementations of this interface must be thread safe; | ||
// a Codec's methods can be called from concurrent goroutines. | ||
type Codec interface { | ||
// Marshal returns the wire format of v. | ||
Marshal(v interface{}) ([]byte, error) | ||
|
@@ -55,21 +58,28 @@ type protoCodec struct { | |
} | ||
|
||
type cachedProtoBuffer struct { | ||
lastMarshaledSize int32 | ||
buffer proto.Buffer | ||
lastMarshaledSize uint32 | ||
proto.Buffer | ||
} | ||
|
||
func capToMaxInt32(val int) uint32 { | ||
if val > math.MaxInt32 { | ||
return uint32(math.MaxInt32) | ||
} | ||
return uint32(val) | ||
} | ||
|
||
func (p protoCodec) marshal(v interface{}, cb *cachedProtoBuffer) ([]byte, error) { | ||
protoMsg := v.(proto.Message) | ||
newSlice := make([]byte, 0, cb.lastMarshaledSize) | ||
|
||
cb.buffer.SetBuf(newSlice) | ||
cb.buffer.Reset() | ||
if err := cb.buffer.Marshal(protoMsg); err != nil { | ||
cb.SetBuf(newSlice) | ||
cb.Reset() | ||
if err := cb.Marshal(protoMsg); err != nil { | ||
return nil, err | ||
} | ||
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. if err := buffer.Marshal(protoMsg); err != nil { 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 imagine, if there were an error while Marshaling, we'd still want to put the buffer back in the pool? 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. woops thanks good catch I missed that. put all of the cleanup into a defer |
||
out := cb.buffer.Bytes() | ||
cb.lastMarshaledSize = int32(len(out)) | ||
out := cb.Bytes() | ||
cb.lastMarshaledSize = capToMaxInt32(len(out)) | ||
return out, nil | ||
} | ||
|
||
|
@@ -78,16 +88,16 @@ func (p protoCodec) Marshal(v interface{}) ([]byte, error) { | |
out, err := p.marshal(v, cb) | ||
|
||
// put back buffer and lose the ref to the slice | ||
cb.buffer.SetBuf(nil) | ||
cb.SetBuf(nil) | ||
protoBufferPool.Put(cb) | ||
return out, err | ||
} | ||
|
||
func (p protoCodec) Unmarshal(data []byte, v interface{}) error { | ||
cb := protoBufferPool.Get().(*cachedProtoBuffer) | ||
cb.buffer.SetBuf(data) | ||
err := cb.buffer.Unmarshal(v.(proto.Message)) | ||
cb.buffer.SetBuf(nil) | ||
cb.SetBuf(data) | ||
err := cb.Unmarshal(v.(proto.Message)) | ||
cb.SetBuf(nil) | ||
protoBufferPool.Put(cb) | ||
return err | ||
} | ||
|
@@ -100,7 +110,7 @@ var ( | |
protoBufferPool = &sync.Pool{ | ||
New: func() interface{} { | ||
return &cachedProtoBuffer{ | ||
buffer: proto.Buffer{}, | ||
Buffer: proto.Buffer{}, | ||
lastMarshaledSize: 16, | ||
} | ||
}, | ||
|
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.
dumb question: are codecs expected to be thread safe? will Codecs ever have user visible state? I know the one below doesn't but this is becoming part of public API, so it may be worth documenting how to use this more clearly.
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.
actually the
Codec
interface is here currently, this PR moves it from where it was (used to be in https://github.com/grpc/grpc-go/blob/master/rpc_util.go#L57).Added a comment though to mention that implementations must be thread safe.