proto: document whether custom proto.Unmarshaler should Reset or not

[awalterschulze]

The proto.Unmarshaler interface expects the generated Unmarshal method to call Reset, as stated in the documentation here https://github.com/golang/protobuf/blob/master/proto/decode.go#L380-L387

// Unmarshaler is the interface representing objects that can
// unmarshal themselves.  The method should reset the receiver before
// decoding starts.  The argument points to data that may be
// overwritten, so implementations should not keep references to the
// buffer.
type Unmarshaler interface {
	Unmarshal([]byte) error
}

If we look at the code at
https://github.com/golang/protobuf/blob/master/proto/decode.go#L407-L422

func Unmarshal(buf []byte, pb Message) error {
	pb.Reset()
	return UnmarshalMerge(buf, pb)
}

func UnmarshalMerge(buf []byte, pb Message) error {
	// If the object can unmarshal itself, let it.
	if u, ok := pb.(Unmarshaler); ok {
		return u.Unmarshal(buf)
	}
	return NewBuffer(buf).Unmarshal(pb)
}

1. We see that this will cause Reset to be called twice, which I suspect will result in two allocations, instead of one. When you are generating an Unmarshal method, this type of thing really impacts performance.

2. We can also see from this code that this will mess up the implementation of UnmarshalMerge, as the results will not be unmarshaling will not be merged if Reset is called in the generated Unmarshal method.

I propose we update the comment to not expect the generated Unmarshal method to call Reset.

[bcmills]

I propose we update the comment to not expect the generated Unmarshal method to call Reset.

Yep, the current comment is just plain wrong. (I think @randall77 has a patch that updates the comment as part of a much larger refactoring. He or @cherrymui may have more information on when that's likely to land.)

[cherrymui]

Yes, @randall77's patch fixes the comment. @dsnet is working on the landing.

[dsnet]

I'm trying to get this work landing in the git repo in the near term, next week or two. but it will land in a separate branch, and will have to sit there for some time.

The new implementation causes much chaos in tests because of brittle tests using reflect.DeepEqual to compare protos. We wrote the cmp package to help perform comparisons in a more correct manner, but it will take time for people to fix tests.

[awalterschulze]

Thank you for keeping us in the loop :)

It would be great if we could not pull in another dependency though.

[dsnet]

The dev branch already has the updated comments.

It's unfortunate that we are changing the semantics, but the only sane behavior is for messages not to reset themselves, otherwise it is impossible for UnmarshalMerge to work properly.