Reuse buffers when marshaling messages in unary Connect protocol

[jhump]

The marshalAppender interface was previously only used in the envelope writer. That means it was already reusing buffers from the pool for marshaling messages in all other protocols (Connect streaming, gRPC, and gRPC-Web); it was just missing from here.

The envelope writer logic is much more complicated, where it tries to use a copy to save the allocation of a bytes.Buffer struct to wrap the []byte. I'm curious if that's been benchmarked and how big of a difference it really makes. I did not add the same logic here, but I can if we really think it's important. Do let me know!

I was also looking through the implementation of bytes.Buffer.Write, which under the does a copy between bytes slices. And in the case where marshalling did not have to increase the buffer capacity, the src and dest slices will actually be the same. So I dug into the runtime source code to see how it handles overlapping regions, hoping that maybe it's smart enough to also no-op when src == dest. It is not that smart. So it does indeed perform a copy of bytes even though it's effectively a no-op 🤷 ..

[CLAassistant]

All committers have signed the CLA.

[emcfarlane]

copy(dst, src) looks like a noop. Ran a quick benchmark:

package main

import (
	"crypto/rand"
	"testing"
)

var slice []byte

func BenchmarkCopySame(b *testing.B) {
	b.ReportAllocs()
	size := 1024
	slice = make([]byte, size*size*size)
	rand.Read(slice)
	chunkSize := size * size
	b.Run("CopySame", func(b *testing.B) {
		for i := 0; i < b.N; i++ {
			chunk := slice[i : chunkSize+i]
			copy(slice[i:], chunk)
		}
	})
	b.Run("CopyDiff", func(b *testing.B) {
		for i := 0; i < b.N; i++ {
			chunk := slice[i+2 : chunkSize+i+2]
			copy(slice[i:], chunk)
		}
	})
}

BenchmarkCopySame/CopySame-8            1000000000               0.5857 ns/op          0 B/op          0 allocs/op
BenchmarkCopySame/CopyDiff-8               64106             18709 ns/op               0 B/op          0 allocs/op

[jhump]

@emcfarlane, I suspect that's on your laptop (an Apple silicon Mac, right?). I was looking at the impl for AMD64. (For these low-level things, it's all assembly.)

We'd need to run the benchmark on an x86 machine in the cloud to verify.

[emcfarlane]

Ah right, yeah be good to check. I didn't look at the source. And yep apple silicon: goos: darwin, goarch: arm64.

[akshayjshah]

We can always include the benchmark in this PR (temporarily) - it's all running on x86 on Github Actions, so we can easily look at the output. We'd need to add a line to the make test definition or the CI github action config temporarily, but that's easy enough.

[akshayjshah]

This is great! Nice catch, Josh. Any further optimizations can (I think) be follow-on PRs.