Message Reset() should use "clear" to empty m.known & m.ext

[brevin-yoke]

go1.21 add new builtin function clear() https://tip.golang.org/ref/spec#Clear

Message interface Reset() should use clear to empty all map

[stapelberg]

You are mentioning m.known and m.ext, so are you talking about the types/dynamicpb package?

Can you explain what the motivation is to use clear()? Are you observing a performance improvement by switching to clear(), or are you suggesting it because it’s cleaner?

[brevin-yoke]

Yes, I am using types/dynamicpb.
I am on vacation, I will benchmark when I am free (maybe a week later), I think clear() back to reduce gc pressure

[brevin-yoke]

I used the MacBook Pro M2 (8 Cores) to test the new Reset2

func (m *Message) Reset2() {
	clear(m.known)
	clear(m.ext)
	m.unknown = nil
}

The result shows that Reset2 can reduce memory allocation

Benchmark Result:

goos: darwin
goarch: arm64
pkg: google.golang.org/protobuf/types/dynamicpb
BenchmarkMessageReset
BenchmarkMessageReset-8               	  157383	      7577 ns/op	    8412 B/op	       9 allocs/op
BenchmarkMessageResetByClear
BenchmarkMessageResetByClear-8        	  218808	      5698 ns/op	      43 B/op	       0 allocs/op
BenchmarkEmptyMessageReset
BenchmarkEmptyMessageReset-8          	29989315	        38.15 ns/op	      96 B/op	       2 allocs/op
BenchmarkEmptyMessageResetByClear
BenchmarkEmptyMessageResetByClear-8   	356020316	         3.234 ns/op	       0 B/op	       0 allocs/op
PASS

Benchmark Code:

package dynamicpb_test

import (
	test3pb "google.golang.org/protobuf/internal/testprotos/test3"
	"google.golang.org/protobuf/reflect/protoreflect"
	"google.golang.org/protobuf/types/dynamicpb"
	"testing"
)

func BenchmarkMessageReset(b *testing.B) {
	message := (*test3pb.TestAllTypes)(nil)
	desc := message.ProtoReflect().Descriptor()
	obj := dynamicpb.NewMessage(desc)
	fields := make([]protoreflect.FieldDescriptor, desc.Fields().Len())
	values := make([]protoreflect.Value, desc.Fields().Len())
	for i := 0; i < desc.Fields().Len(); i++ {
		fd := desc.Fields().Get(i)
		fields[i] = fd
		values[i] = obj.NewField(fd)
	}
	b.ResetTimer()
	b.ReportAllocs()
	for i := 0; i < b.N; i++ {
		for i := 0; i < desc.Fields().Len(); i++ {
			obj.Set(fields[i], values[i])
		}
		obj.Reset()
	}
}

func BenchmarkMessageResetByClear(b *testing.B) {
	message := (*test3pb.TestAllTypes)(nil)
	desc := message.ProtoReflect().Descriptor()
	obj := dynamicpb.NewMessage(desc)
	fields := make([]protoreflect.FieldDescriptor, desc.Fields().Len())
	values := make([]protoreflect.Value, desc.Fields().Len())
	for i := 0; i < desc.Fields().Len(); i++ {
		fd := desc.Fields().Get(i)
		fields[i] = fd
		values[i] = obj.NewField(fd)
	}
	b.ResetTimer()
	b.ReportAllocs()
	for i := 0; i < b.N; i++ {
		for i := 0; i < desc.Fields().Len(); i++ {
			obj.Set(fields[i], values[i])
		}
		obj.Reset2()
	}
}

func BenchmarkEmptyMessageReset(b *testing.B) {
	message := (*test3pb.TestAllTypes)(nil)
	obj := dynamicpb.NewMessage(message.ProtoReflect().Descriptor())
	b.ResetTimer()
	b.ReportAllocs()
	for i := 0; i < b.N; i++ {
		obj.Reset()
	}
}

func BenchmarkEmptyMessageResetByClear(b *testing.B) {
	message := (*test3pb.TestAllTypes)(nil)
	obj := dynamicpb.NewMessage(message.ProtoReflect().Descriptor())
	b.ResetTimer()
	b.ReportAllocs()
	for i := 0; i < b.N; i++ {
		obj.Reset2()
	}
}

[stapelberg]

Thank you for the details. Would you be willing to contribute this change? See https://github.com/protocolbuffers/protobuf-go/blob/master/CONTRIBUTING.md

[brevin-yoke]

Thank you for the details. Would you be willing to contribute this change? See https://github.com/protocolbuffers/protobuf-go/blob/master/CONTRIBUTING.md

change has been submitted for review:

https://go-review.googlesource.com/c/protobuf/+/649175

[stapelberg]

@aktau Why did you close this issue? The CL is still under review.

[aktau]

I had been too optimistic when reading change has been submitted above, and had assumed that the CL description had just missed the "closes #1674" annotation.