From b0d217f66456abab5dc8c6c5e6fd375d6449c7ef Mon Sep 17 00:00:00 2001 From: Damien Neil Date: Mon, 6 Jan 2020 11:17:07 -0800 Subject: [PATCH] proto, internal/impl: don't create fast path Size for legacy Marshalers Implementations of the legacy Marshaler type have no way to efficiently compute the size of the message. Rather than generating an inefficient fast-path Size method which marshals the message and examines the length of the result, don't generate a fast-path at all. Drop the requirement that a fast-path MarshalAppend requires a corresponding Size. Avoids O(N^2) behavior when marshaling a legacy Marshaler that recursively calls proto.Marshal. Change-Id: I4793cf32275d08f29c8e1a1a44a193d9a5724058 Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/213443 Reviewed-by: Joe Tsai --- internal/impl/legacy_message.go | 7 ------- proto/encode.go | 14 ++++++++------ proto/size.go | 9 ++++++++- runtime/protoiface/methods.go | 2 +- 4 files changed, 17 insertions(+), 15 deletions(-) diff --git a/internal/impl/legacy_message.go b/internal/impl/legacy_message.go index aaf6d60f..ac96e711 100644 --- a/internal/impl/legacy_message.go +++ b/internal/impl/legacy_message.go @@ -51,7 +51,6 @@ func legacyLoadMessageInfo(t reflect.Type, name pref.FullName) *MessageInfo { v := reflect.Zero(t).Interface() if _, ok := v.(legacyMarshaler); ok { mi.methods.MarshalAppend = legacyMarshalAppend - mi.methods.Size = legacySize // We have no way to tell whether the type's Marshal method // supports deterministic serialization or not, but this @@ -364,7 +363,6 @@ type legacyUnmarshaler interface { } var legacyProtoMethods = &piface.Methods{ - Size: legacySize, MarshalAppend: legacyMarshalAppend, Unmarshal: legacyUnmarshal, @@ -375,11 +373,6 @@ var legacyProtoMethods = &piface.Methods{ Flags: piface.SupportMarshalDeterministic, } -func legacySize(m protoreflect.Message, opts piface.MarshalOptions) int { - b, _ := legacyMarshalAppend(nil, m, opts) - return len(b) -} - func legacyMarshalAppend(b []byte, m protoreflect.Message, opts piface.MarshalOptions) ([]byte, error) { v := m.(unwrapper).protoUnwrap() marshaler, ok := v.(legacyMarshaler) diff --git a/proto/encode.go b/proto/encode.go index 702190f4..0801ce48 100644 --- a/proto/encode.go +++ b/proto/encode.go @@ -100,13 +100,15 @@ func (o MarshalOptions) MarshalAppend(b []byte, m Message) ([]byte, error) { func (o MarshalOptions) marshalMessage(b []byte, m protoreflect.Message) ([]byte, error) { if methods := protoMethods(m); methods != nil && methods.MarshalAppend != nil && !(o.Deterministic && methods.Flags&protoiface.SupportMarshalDeterministic == 0) { - sz := methods.Size(m, protoiface.MarshalOptions(o)) - if cap(b) < len(b)+sz { - x := make([]byte, len(b), growcap(cap(b), len(b)+sz)) - copy(x, b) - b = x + if methods.Size != nil { + sz := methods.Size(m, protoiface.MarshalOptions(o)) + if cap(b) < len(b)+sz { + x := make([]byte, len(b), growcap(cap(b), len(b)+sz)) + copy(x, b) + b = x + } + o.UseCachedSize = true } - o.UseCachedSize = true return methods.MarshalAppend(b, m, protoiface.MarshalOptions(o)) } return o.marshalMessageSlow(b, m) diff --git a/proto/size.go b/proto/size.go index 99475805..beaa9251 100644 --- a/proto/size.go +++ b/proto/size.go @@ -22,9 +22,16 @@ func (o MarshalOptions) Size(m Message) int { } func sizeMessage(m protoreflect.Message) (size int) { - if methods := protoMethods(m); methods != nil && methods.Size != nil { + methods := protoMethods(m) + if methods != nil && methods.Size != nil { return methods.Size(m, protoiface.MarshalOptions{}) } + if methods != nil && methods.MarshalAppend != nil { + // This is not efficient, but we don't have any choice. + // This case is mainly used for legacy types with a Marshal method. + b, _ := methods.MarshalAppend(nil, m, protoiface.MarshalOptions{}) + return len(b) + } return sizeMessageSlow(m) } diff --git a/runtime/protoiface/methods.go b/runtime/protoiface/methods.go index e0adf9df..b7ba129b 100644 --- a/runtime/protoiface/methods.go +++ b/runtime/protoiface/methods.go @@ -34,7 +34,7 @@ type Methods struct { Size func(m protoreflect.Message, opts MarshalOptions) int // MarshalAppend appends the wire-format encoding of m to b, returning the result. - // Size must be provided if a custom MarshalAppend is provided. + // Size should be provided if a custom MarshalAppend is provided. // It must not perform required field checks. MarshalAppend func(b []byte, m protoreflect.Message, opts MarshalOptions) ([]byte, error)