Optimize `Reader.ReadByte()`

[ash2k]

grpc-go/mem/buffer_slice.go

Lines 201 to 218 in 19e4128

	// ReadByte reads a single byte.
	func (r *Reader) ReadByte() (byte, error) {
	if r.len == 0 {
	return 0, io.EOF
	}
	// There may be any number of empty buffers in the slice, clear them all until a
	// non-empty buffer is reached. This is guaranteed to exit since r.len is not 0.
	for r.freeFirstBufferIfEmpty() {
	}
	b := r.data[0].ReadOnlyData()[r.bufferIdx]
	r.len--
	r.bufferIdx++
	// Free the first buffer in the slice if the last byte was read
	r.freeFirstBufferIfEmpty()
	return b, nil
	}

I wonder if we can get rid of (or move) the for loop there by maintaining the precondition that the next buffer is not empty? Perhaps via (a private) constructor that ensures this. The loop can be moved to the bottom of the function - at least we'll only call freeFirstBufferIfEmpty() once.

I think if len(r.data) == 0 || r.bufferIdx != len(r.data[0].ReadOnlyData()) { can also be simplified to if len(r.data) == 0 || r.bufferIdx != r.data[0].Len().

Another idea - the buffers in the slice are probably never empty, yet we have the loop there. Maybe we can check that there are no empty buffers and eliminate them (by copying into a new slice) one time in the constructor and then remove the loop? The slice will (almost?) never be allocated in practice but we'll get rid of the loop 100% of the time.

i.e. if we make both changes, the result will be the same code but without the for there.

WDYT?

[easwars]

@ash2k Do you happen to have any benchmark results for the changes you are proposing here? Thanks.

@arjan-bal : FYI

[ash2k]

@easwars No, I do not. The profiler screenshot is from our production.

[arjan-bal]

Not accounting for compiler optimizations, it seems that the time spent in the for loop should be negligible if it breaks in the first couple of iterations. Restructuring the code to maintain the invariant of having a non-empty buffer at the head seems to only be shifting the computation to another part of the code, without actually reducing the total amount of computation.

@ash2k, is this a case of the compiler not being able to optimize the existing code? If so, I think we should reproduce this behaviour in a mirco-benchmark to understand it better. Maybe there's a way to structure the code in such a way that helps the compiler optimize it. We can also consult the Golang team if we have a repro.

---

I think if len(r.data) == 0 || r.bufferIdx != len(r.data[0].ReadOnlyData()) { can also be simplified to if len(r.data) == 0 || r.bufferIdx != r.data[0].Len().

This seems fine to me.

Another idea - the buffers in the slice are probably never empty, yet we have the loop there. Maybe we can check that there are no empty buffers and eliminate them (by copying into a new slice) one time in the constructor and then remove the loop?

This would introduce a new heap allocation for some users. I would feel more comfortable making such a change once we have a better understanding of the root cause and have determined that it's not possible to improve performance without the extra allocation.