protoCodec: return early if proto.Marshaler #1689
Conversation
If the object to marshal implements proto.Marshaler, delegate to that immediately instead of pre-allocating a buffer. (*proto.Buffer).Marshal has the same logic, so the []byte buffer we pre-allocate in codec.go would never be used. This is mainly for users of gogoproto. If you turn on the "marshaler" and "sizer" gogoproto options, the generated Marshal() method already pre-allocates a buffer of the appropriate size for marshaling the entire object.
|
Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement. After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.
Regards, |
|
FYI still working on signing the license. |
|
I think I signed the license. |
|
I associated my GH account, so maybe now? |
|
PR #1478 was attempting to do the same thing, but also for proto.Unmarshaler. Could you add that, too, and run our benchmarks before/after the change to make sure this does not hurt performance for non-self-marshaling proto types? |
If the object to unmarshal implements proto.Unmarshaler, delegate to that immediately. This perhaps saves a bit of work preparing the cached the proto.Buffer object which would not end up being used for the proto.Unmarshaler case. Note that I moved the obj.Reset() call above the delegation to obj.Unmarshal(). This maintains the grpc behavior of proto.Unmarshalers always being Reset() before being delegated to, which is consistent to how proto.Unmarshal() behaves (proto.Buffer does not call Reset() in Unmarshal).
|
Thank you for looking. I have pushed the Unmarshaler change as requested. Below is benchmark comparison output. I used these benchmark options: |
| @@ -79,10 +84,17 @@ func (p protoCodec) Marshal(v interface{}) ([]byte, error) { | |||
| } | |||
|
|
|||
| func (p protoCodec) Unmarshal(data []byte, v interface{}) error { | |||
| protoMsg := v.(proto.Message) | |||
| protoMsg.Reset() | |||
There was a problem hiding this comment.
Per proto.Unmarshaler: "The method should reset the receiver before decoding starts."
So this should not be necessary above the call to Unmarshal. Also, buffer.Unmarshal says "Unlike proto.Unmarshal, this does not reset pb before starting to unmarshal."
There was a problem hiding this comment.
I looked at some of our Unmarshalers generated via gogoproto and they don't reset themselves in their Unmarshal. I think it would be dangerous to not reset here. See gogo/protobuf#334 and golang/protobuf#424.
There was a problem hiding this comment.
dfawley
added
the
Type: Performance
If the object to marshal implements proto.Marshaler, delegate to that
immediately instead of pre-allocating a buffer. (*proto.Buffer).Marshal
has the same logic, so the []byte buffer we pre-allocate in codec.go
would never be used.
This is mainly for users of gogoproto. If you turn on the "marshaler"
and "sizer" gogoproto options, the generated Marshal() method already
pre-allocates a buffer of the appropriate size for marshaling the
entire object.