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

Fix buffer reuse #624

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions Makefile 100755 → 100644
Expand Up @@ -137,6 +137,7 @@ regenerate:
make -C test/issue503 regenerate
make -C test/issue530 regenerate
make -C test/issue617 regenerate
make -C test/protobuffer regenerate

make gofmt

Expand Down
24 changes: 24 additions & 0 deletions proto/table_marshal.go
Expand Up @@ -2930,10 +2930,23 @@ func Size(pb Message) int {
return info.Size(pb)
}

type reverseMarshaler interface {
MarshalToSizedBuffer([]byte) (int, error)
Size() int
}

// Marshal takes a protocol buffer message
// and encodes it into the wire format, returning the data.
// This is the main entry point.
func Marshal(pb Message) ([]byte, error) {
if m, ok := pb.(reverseMarshaler); ok {
b := make([]byte, m.Size())
n, err := m.MarshalToSizedBuffer(b)
if err != nil {
return nil, err
}
return b[:n], nil
}
if m, ok := pb.(newMarshaler); ok {
siz := m.XXX_Size()
b := make([]byte, 0, siz)
Expand Down Expand Up @@ -2965,6 +2978,17 @@ func (p *Buffer) Marshal(pb Message) error {
if _, ok := pb.(Marshaler); ok {
return fmt.Errorf("proto: deterministic not supported by the Marshal method of %T", pb)
}
if _, ok := pb.(reverseMarshaler); ok {
return fmt.Errorf("proto: deterministic not supported by the Marshal method of %T", pb)
}

}
if m, ok := pb.(reverseMarshaler); ok {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently I think I would prefer too rather not adding another interface here and keep calling the XXX methods here.
With the direct calling we will skip the p.deterministic setting on the buffer.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do have a safer approach here:
#627
Keeping the current interfaces/flow.

siz := m.Size()
p.grow(siz) // make sure buf has enough capacity
n, err := m.MarshalToSizedBuffer(p.buf[len(p.buf) : len(p.buf)+siz])
p.buf = p.buf[:len(p.buf)+n]
return err
}
if m, ok := pb.(newMarshaler); ok {
siz := m.XXX_Size()
Expand Down
6 changes: 2 additions & 4 deletions protoc-gen-gogo/generator/generator.go
Expand Up @@ -2717,8 +2717,7 @@ func (g *Generator) generateCommonMethods(mc *msgCtx) {
if gogoproto.IsMarshaler(g.file.FileDescriptorProto, mc.message.DescriptorProto) ||
gogoproto.IsUnsafeMarshaler(g.file.FileDescriptorProto, mc.message.DescriptorProto) {
if gogoproto.IsStableMarshaler(g.file.FileDescriptorProto, mc.message.DescriptorProto) {
g.P("b = b[:cap(b)]")
g.P("n, err := m.MarshalToSizedBuffer(b)")
g.P("n, err := m.MarshalTo(b)")
g.P("if err != nil {")
g.In()
g.P("return nil, err")
Expand All @@ -2731,8 +2730,7 @@ func (g *Generator) generateCommonMethods(mc *msgCtx) {
g.P("return xxx_messageInfo_", mc.goName, ".Marshal(b, m, deterministic)")
g.P("} else {")
g.In()
g.P("b = b[:cap(b)]")
g.P("n, err := m.MarshalToSizedBuffer(b)")
g.P("n, err := m.MarshalTo(b)")
g.P("if err != nil {")
g.In()
g.P("return nil, err")
Expand Down
3 changes: 1 addition & 2 deletions test/asymetric-issue125/asym.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 2 additions & 4 deletions test/casttype/combos/both/casttype.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 2 additions & 4 deletions test/casttype/combos/marshaler/casttype.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 2 additions & 4 deletions test/castvalue/combos/both/castvalue.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 2 additions & 4 deletions test/castvalue/combos/marshaler/castvalue.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.