chunkenc: Slightly optimize xorWrite/xoRead (#11476)

With these changes, the "happy path" when the leading and trailing
number of bits don't need an update, fewer operations are needed.

The change is probably very marginal (no change in the benchmark added
here, but the benchmark also doesn't cover non-changing values), and
an argument could me made that avoiding pointers also has its
benefits.

However, I think that reducing the number of return values improves
readability. Which convinced me that I should at least propose this.

Signed-off-by: beorn7 <beorn@grafana.com>

tsdb/chunkenc/histogram.go

@@ -528,7 +528,7 @@ func (a *HistogramAppender) Recode(
 }
 
 func (a *HistogramAppender) writeSumDelta(v float64) {
-	a.leading, a.trailing = xorWrite(a.b, v, a.sum, a.leading, a.trailing)
+	xorWrite(a.b, v, a.sum, &a.leading, &a.trailing)
 }
 
 type histogramIterator struct {
@@ -867,11 +867,10 @@ func (it *histogramIterator) Next() ValueType {
 }
 
 func (it *histogramIterator) readSum() bool {
-	sum, leading, trailing, err := xorRead(&it.br, it.sum, it.leading, it.trailing)
+	err := xorRead(&it.br, &it.sum, &it.leading, &it.trailing)
 	if err != nil {
 		it.err = err
 		return false
 	}
-	it.sum, it.leading, it.trailing = sum, leading, trailing
 	return true
 }

tsdb/chunkenc/xor.go

@@ -222,7 +222,7 @@ func bitRange(x int64, nbits uint8) bool {
 }
 
 func (a *xorAppender) writeVDelta(v float64) {
-	a.leading, a.trailing = xorWrite(a.b, v, a.v, a.leading, a.trailing)
+	xorWrite(a.b, v, a.v, &a.leading, &a.trailing)
 }
 
 type xorIterator struct {
@@ -386,44 +386,42 @@ func (it *xorIterator) Next() ValueType {
 }
 
 func (it *xorIterator) readValue() ValueType {
-	val, leading, trailing, err := xorRead(&it.br, it.val, it.leading, it.trailing)
+	err := xorRead(&it.br, &it.val, &it.leading, &it.trailing)
 	if err != nil {
 		it.err = err
 		return ValNone
 	}
-	it.val, it.leading, it.trailing = val, leading, trailing
 	it.numRead++
 	return ValFloat
 }
 
-func xorWrite(
+func xorWrite(b *bstream, newValue, currentValue float64, leading, trailing *uint8) {
-	b *bstream,
-	newValue, currentValue float64,
-	currentLeading, currentTrailing uint8,
-) (newLeading, newTrailing uint8) {
 	delta := math.Float64bits(newValue) ^ math.Float64bits(currentValue)
 
 	if delta == 0 {
 		b.writeBit(zero)
-		return currentLeading, currentTrailing
+		return
 	}
 	b.writeBit(one)
 
-	newLeading = uint8(bits.LeadingZeros64(delta))
+	newLeading := uint8(bits.LeadingZeros64(delta))
-	newTrailing = uint8(bits.TrailingZeros64(delta))
+	newTrailing := uint8(bits.TrailingZeros64(delta))
 
 	// Clamp number of leading zeros to avoid overflow when encoding.
 	if newLeading >= 32 {
 		newLeading = 31
 	}
 
-	if currentLeading != 0xff && newLeading >= currentLeading && newTrailing >= currentTrailing {
+	if *leading != 0xff && newLeading >= *leading && newTrailing >= *trailing {
 		// In this case, we stick with the current leading/trailing.
 		b.writeBit(zero)
-		b.writeBits(delta>>currentTrailing, 64-int(currentLeading)-int(currentTrailing))
+		b.writeBits(delta>>*trailing, 64-int(*leading)-int(*trailing))
-		return currentLeading, currentTrailing
+		return
 	}
 
+	// Update leading/trailing for the caller.
+	*leading, *trailing = newLeading, newTrailing
+
 	b.writeBit(one)
 	b.writeBits(uint64(newLeading), 5)
 
@@ -435,42 +433,43 @@ func xorWrite(
 	sigbits := 64 - newLeading - newTrailing
 	b.writeBits(uint64(sigbits), 6)
 	b.writeBits(delta>>newTrailing, int(sigbits))
-	return
 }
 
-func xorRead(
+func xorRead(br *bstreamReader, value *float64, leading, trailing *uint8) error {
-	br *bstreamReader, currentValue float64, currentLeading, currentTrailing uint8,
+	bit, err := br.readBitFast()
-) (newValue float64, newLeading, newTrailing uint8, err error) {
-	var bit bit
-	var bits uint64
-
-	bit, err = br.readBitFast()
 	if err != nil {
 		bit, err = br.readBit()
 	}
 	if err != nil {
-		return
+		return err
 	}
 	if bit == zero {
-		return currentValue, currentLeading, currentTrailing, nil
+		return nil
 	}
 	bit, err = br.readBitFast()
 	if err != nil {
 		bit, err = br.readBit()
 	}
 	if err != nil {
-		return
+		return err
 	}
+
+	var (
+		bits                           uint64
+		newLeading, newTrailing, mbits uint8
+	)
+
 	if bit == zero {
 		// Reuse leading/trailing zero bits.
-		newLeading, newTrailing = currentLeading, currentTrailing
+		newLeading, newTrailing = *leading, *trailing
+		mbits = 64 - newLeading - newTrailing
 	} else {
 		bits, err = br.readBitsFast(5)
 		if err != nil {
 			bits, err = br.readBits(5)
 		}
 		if err != nil {
-			return
+			return err
 		}
 		newLeading = uint8(bits)
 
@@ -479,29 +478,29 @@ func xorRead(
 			bits, err = br.readBits(6)
 		}
 		if err != nil {
-			return
+			return err
 		}
-		mbits := uint8(bits)
+		mbits = uint8(bits)
 		// 0 significant bits here means we overflowed and we actually
 		// need 64; see comment in xrWrite.
 		if mbits == 0 {
 			mbits = 64
 		}
 		newTrailing = 64 - newLeading - mbits
+		// Update leading/trailing zero bits for the caller.
+		*leading, *trailing = newLeading, newTrailing
 	}
-
-	mbits := 64 - newLeading - newTrailing
 	bits, err = br.readBitsFast(mbits)
 	if err != nil {
 		bits, err = br.readBits(mbits)
 	}
 	if err != nil {
-		return
+		return err
 	}
-	vbits := math.Float64bits(currentValue)
+	vbits := math.Float64bits(*value)
 	vbits ^= bits << newTrailing
-	newValue = math.Float64frombits(vbits)
+	*value = math.Float64frombits(vbits)
-	return
+	return nil
 }
 
 // OOOXORChunk holds a XORChunk and overrides the Encoding() method.

tsdb/chunkenc/xor_test.go

@@ -0,0 +1,43 @@
+// Copyright 2022 The Prometheus Authors
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package chunkenc
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func BenchmarkXorRead(b *testing.B) {
+	c := NewXORChunk()
+	app, err := c.Appender()
+	require.NoError(b, err)
+	for i := int64(0); i < 120*1000; i += 1000 {
+		app.Append(i, float64(i)+float64(i)/10+float64(i)/100+float64(i)/1000)
+	}
+
+	b.ReportAllocs()
+	b.ResetTimer()
+
+	var it Iterator
+	for i := 0; i < b.N; i++ {
+		var ts int64
+		var v float64
+		it = c.Iterator(it)
+		for it.Next() != ValNone {
+			ts, v = it.At()
+		}
+		_, _ = ts, v
+	}
+}