This change is 

After some experiments, it seems like the allocation is hidden inside protoutil.Unmarshal.

There are more Unmarshal calls in this package which probably could be optimized too.
Upd: done.

After some experiments, it seems like the allocation is hidden inside protoutil.Unmarshal.

Yeah, gogoproto/Protobuf is very inefficient in terms of allocations. We've considered using some other encoding, e.g. Tobi was playing around with FlatBuffers last year, but it hasn't been prioritized. In the short term, it doesn't seem viable to replace it.

I think there was a reason why we use protoutil.Unmarshal() over Message.Unmarshal(), I think there's even a linter for it. I'll have to refresh my memory on this stuff.

I think there was a reason why we use protoutil.Unmarshal() over Message.Unmarshal(), I think there's even a linter for it. I'll have to refresh my memory on this stuff.

Interesting.

When looking at code, I concluded that protoutil.Unmarshal pretty much calls Reset and Unmarshal on the message. We don't need the Reset in all these cases because the message is created fresh, so we can just call Unmarshal on the concrete message.

When looking at code, I concluded that protoutil.Unmarshal pretty much calls Reset and Unmarshal on the message. We don't need the Reset in all these cases because the message is created fresh, so we can just call Unmarshal on the concrete message.

Yeah, looks about right, this is probably fine then.

We do have this lint indeed.

That's what I thought:

    lint_test.go:1295:
        clusterversion/setting.go:140:  if err := curVer.Unmarshal(encoded.([]byte)); err != nil { <- forbidden; use 'protoutil.Unmarshal' instead
    lint_test.go:1295:
        clusterversion/setting.go:157:  if err := clusterVersion.Unmarshal(val); err != nil { <- forbidden; use 'protoutil.Unmarshal' instead
    lint_test.go:1295:
        clusterversion/setting.go:168:  if err := newCV.Unmarshal(newRawProto); err != nil { <- forbidden; use 'protoutil.Unmarshal' instead
    lint_test.go:1295:
        clusterversion/setting.go:177:  if err := oldCV.Unmarshal(curRawProto); err != nil { <- forbidden; use 'protoutil.Unmarshal' instead
    lint_test.go:1295:
        clusterversion/setting.go:213:  if err := ver.Unmarshal(rawProto); err != nil { <- forbidden; use 'protoutil.Unmarshal' instead

It's probably okay anyway, but I'll have a closer look at this when I have time.

The linter was introduced here: #18455. I think it's overly restrictive. It costs unnecessary allocations.

I tried generics:

func unmarshal[Message protoutil.Message](m Message, b []byte) error {
	m.Reset()
	return m.Unmarshal(b)
}

...

if err := unmarshal[*ClusterVersion](&curVer, encoded.([]byte)); err != nil {
	log.Fatalf(ctx, "%v", err)
}

but it doesn't remove the allocation. I don't think the calls are inlined in this case.

So the only thing we can do is either lift or workaround this restriction. Maybe we can silence the linter knowing that this case is safe.

Btw, I think we could add a generic protoutil.UnmarshalNew() which always creates a new message, guaranteeing that it's safe to omit Reset(). Something like:

func UnmarshalNew[M Message](data []byte) (M, error) {
  var m M
  return m.Unmarshal(data)
}

Btw, I think we could add a generic protoutil.UnmarshalNew() which always creates a new message, guaranteeing that it's safe to omit Reset(). Something like:

func UnmarshalNew[M Message](data []byte) (M, error) {
  var m M
  return m.Unmarshal(data)
}

Tried that:

func UnmarshalNew[M Message](data []byte) (M, error) {
	var m M
	err := m.Unmarshal(data)
	return m, err
}

// and then
curVer, err := protoutil.UnmarshalNew[ClusterVersion](encoded.([]byte))
if err != nil {
	log.Fatalf(ctx, "%v", err)
}

This does not build:

pkg/clusterversion/setting.go:139:40: ClusterVersion does not satisfy protoutil.Message (method MarshalTo has pointer receiver)

Setting the M type to pointer doesn't help either: var m M becomes a nil, and then there is a nil deref panic.

Setting the M type to pointer doesn't help either: var m M becomes a nil, and then there is a nil deref panic.

Can do m := new(M) or something, perhaps.

Either way, as you pointed out on Slack, this may not help with performance anyway due to how Go monomorphizes generic functions.

bors r=erikgrinaker

🕐 Waiting for PR status (GitHub check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set.

bors retry

Build succeeded:

• Bazel Essential CI (Cockroach)