benchmarks: expensive/inefficient use of bytes.Buffer and .Reset() in benchmarking

[odeke-em]

If we examine say node_test.go we can see this code

iavl/node_test.go

Lines 164 to 168 in ed98044

	for i := 0; i < sub.N; i++ {
	var buf bytes.Buffer
	buf.Reset()
	_ = node.writeBytes(&buf)
	}

and

iavl/node_test.go

Lines 172 to 176 in ed98044

	for i := 0; i < sub.N; i++ {
	var buf bytes.Buffer
	buf.Grow(node.encodedSize())
	_ = node.writeBytes(&buf)
	}

but notice in there, we unfortunately do the following

        var buf bytes.Buffer 
 	buf.Reset() 
 	_ = node.writeBytes(&buf) 

where after creating a new bytes.Buffer, we invoke buf.Reset() -- this is firstly unnecessary, but even more, this pollutes the benchmark with the allocation to a bytes.Buffer yet we are supposed to be focusing on benchmarking node.writeBytes. With the use of buf.Reset() it seems like the callers wanted to reuse the bytes.Buffer. The correct usage is the following

buf := new(bytes.Buffer)
 for i := 0; i < sub.N; i++ { 
 	_ = node.writeBytes(buf) 
 	buf.Reset() 
 } 

The results are stark when fixed

$ benchstat before after
name                             old time/op    new time/op    delta
Node_WriteBytes/NoPreAllocate-8     308ns ± 2%     156ns ± 5%   -49.35%  (p=0.000 n=10+10)
Node_WriteBytes/PreAllocate-8       277ns ± 4%     157ns ± 4%   -43.36%  (p=0.000 n=9+10)

name                             old alloc/op   new alloc/op   delta
Node_WriteBytes/NoPreAllocate-8      272B ± 0%        0B       -100.00%  (p=0.000 n=10+10)
Node_WriteBytes/PreAllocate-8        128B ± 0%        0B       -100.00%  (p=0.000 n=10+10)

name                             old allocs/op  new allocs/op  delta
Node_WriteBytes/NoPreAllocate-8      3.00 ± 0%      0.00       -100.00%  (p=0.000 n=10+10)
Node_WriteBytes/PreAllocate-8        2.00 ± 0%      0.00       -100.00%  (p=0.000 n=10+10)

[odeke-em]

So I see the purpose here is to pass in 2 different styles of *bytes.Buffer, one freshly allocated then the other with .Grow invoked, however, the use of buf.Reset() is unnecessary as the buffer is already fresh.

[odeke-em]

The benchmarks are probably to serve as a reminder to the authors, but I don't think the Node_WriteBytes/*Allocate distinctions are necessary given that they measure use of *bytes.Buffer, but whatever. Just the buf.Reset() needs to be removed.

[robert-zaremba]

thanks!