Use specific types when reading offset table #3393

colega · 2022-11-04T13:47:00Z

What this PR does

Modified the index.ReadOffsetTable function to use a specific type instead of a slice, that escapes to heap (for every label value in the postings offset table).

I will upstream this change to Prometheus, and then use the upstreamed version again when it's available, but wanted to bring it here first, especially because we already have benchmarks for this.

name          old time/op    new time/op    delta
BinaryReader    42.7µs ± 3%    33.9µs ± 8%  -20.63%  (p=0.008 n=5+5)

name          old alloc/op   new alloc/op   delta
BinaryReader    41.7kB ± 0%    35.3kB ± 0%  -15.31%  (p=0.000 n=5+4)

name          old allocs/op  new allocs/op  delta
BinaryReader       623 ± 0%       423 ± 0%  -32.10%  (p=0.008 n=5+5)

I also added a benchmark with a larger block size:

name                                old time/op    new time/op    delta
BinaryReader_LargerBlock/benchmark    2.72ms ± 4%    1.89ms ± 3%  -30.51%  (p=0.008 n=5+5)

name                                old alloc/op   new alloc/op   delta
BinaryReader_LargerBlock/benchmark    1.70MB ± 0%    1.06MB ± 0%     ~     (p=0.079 n=4+5)

name                                old allocs/op  new allocs/op  delta
BinaryReader_LargerBlock/benchmark     40.2k ± 0%     20.1k ± 0%  -49.93%  (p=0.008 n=5+5)

Which issue(s) this PR fixes or relates to

None, just was surfing the code.

Checklist

Tests updated
Documentation added
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

Instead of reading a generic-ish []string, we can read a generic type which would be specifically labels.Label. This avoid allocating a slice that escapes to the heap, making it both faster and more efficient in terms of memory management. name old time/op new time/op delta BinaryReader 42.7µs ± 3% 33.9µs ± 8% -20.63% (p=0.008 n=5+5) name old alloc/op new alloc/op delta BinaryReader 41.7kB ± 0% 35.3kB ± 0% -15.31% (p=0.000 n=5+4) name old allocs/op new allocs/op delta BinaryReader 623 ± 0% 423 ± 0% -32.10% (p=0.008 n=5+5) Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

I initially assumed that we can just check for label emptiness, but we can't as there's a special entry in the postings offsets table that is an empty name/value label that corresponds to the "all postings" entry. Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

56quarters

Changes seem reasonable to me but I'm not a TSDB expert

56quarters · 2022-11-04T14:11:13Z

pkg/storegateway/indexheader/binary_reader.go

+// ReadOffsetTable reads an offset table and at the given position calls f for each
+// found entry. If f returns an error it stops decoding and returns the received error.
+// TODO: use the upstreamed version from Prometheus tsdb/index package when available.
+func readOffsetTable[T any](bs index.ByteSlice, off uint64, read func(d *encoding.Decbuf) (T, error), f func(T, uint64, int) error) error {


nit: Does this need to be generic? It's always called with labels.Label, right?

Since I'm upstreaming the change to Prometheus I want to keep code exactly the same to just make a drop-in replacement later.

But meh, maybe you're right, it will be very easy to replace it anyway. Removing in 8633e9b

We'll bring them back once we start using the Prometheus version. Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

pstibrany

Looks good, thanks!

pstibrany · 2022-11-07T10:31:41Z

pkg/storegateway/indexheader/binary_reader.go

@@ -533,84 +534,79 @@ func newFileBinaryReader(path string, postingOffsetsInMemSampling int, cfg Binar
 		return nil, errors.Wrap(err, "read symbols")
 	}

-	var lastKey []string
+	var lastLbl labels.Label
+	var lastSet bool


super-nit: lastSet := false (it's just more explicit)

This was an internal fight for me, being more explicit vs being consistent with the var declaration above.

Changed in 1d2cfb6

pstibrany · 2022-11-07T10:32:18Z

pkg/storegateway/indexheader/binary_reader.go

 			prevRng = index.Range{Start: int64(off + postingLengthFieldSize)}
 			return nil
 		}); err != nil {
 			return nil, errors.Wrap(err, "read postings table")
 		}
-		if lastKey != nil {
+		if lastLbl.Name != "" {


Shouldn't this be: if lastSet ? (Although it doesn't change the logic, unless someone writes postings for empty label name)

You're right, I'll change it.

Empty label name is a valid case for "all postings" entry, but if it's last, it would be just a noop here anyway.

Applied in 30b5533

pstibrany · 2022-11-07T10:37:36Z

pkg/storegateway/indexheader/binary_reader.go

-					r.postings[lastKey[0]].lastValOffset = int64(off - crc32.Size)
-					lastKey = nil
+					r.postings[lastLbl.Name].lastValOffset = int64(off - crc32.Size)
+					lastLbl = labels.Label{}


Why not lastSet = false? (Or: why is this even needed? We overwrite lastSet and lastLbl shortly anyway).

Even if not strictly needed, doing lastSet = false here may better explain the intention.

Instead of adding yet another useless line, I just removed this lastLbl = labels.Label{} line.

Once you have a last, you can't unhave it.

30b5533

(Please can you ACK that you're okay with this change so I can merge?)

works for me.

pracucci

LGTM (modulo Peter's comments)

pracucci · 2022-11-07T12:47:34Z

pkg/storegateway/indexheader/binary_reader.go

-					r.postings[lastKey[0]].lastValOffset = int64(off - crc32.Size)
-					lastKey = nil
+					r.postings[lastLbl.Name].lastValOffset = int64(off - crc32.Size)
+					lastLbl = labels.Label{}


Even if not strictly needed, doing lastSet = false here may better explain the intention.

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

* Add BenchmarkBinaryReader_LargerBlock Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com> * Use specific types when reading offset table Instead of reading a generic-ish []string, we can read a generic type which would be specifically labels.Label. This avoid allocating a slice that escapes to the heap, making it both faster and more efficient in terms of memory management. name old time/op new time/op delta BinaryReader 42.7µs ± 3% 33.9µs ± 8% -20.63% (p=0.008 n=5+5) name old alloc/op new alloc/op delta BinaryReader 41.7kB ± 0% 35.3kB ± 0% -15.31% (p=0.000 n=5+4) name old allocs/op new allocs/op delta BinaryReader 623 ± 0% 423 ± 0% -32.10% (p=0.008 n=5+5) Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com> * Update CHANGELOG.md Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com> * Use a specific flag to signal that lastLbl was set I initially assumed that we can just check for label emptiness, but we can't as there's a special entry in the postings offsets table that is an empty name/value label that corresponds to the "all postings" entry. Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com> * Remove generics as we don't need them now We'll bring them back once we start using the Prometheus version. Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com> * Apply rewiew comments Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com> * Check for lastSet Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com> Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

colega added 2 commits November 4, 2022 14:39

Add BenchmarkBinaryReader_LargerBlock

43c7336

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

colega requested a review from a team as a code owner November 4, 2022 13:47

Update CHANGELOG.md

a1e585c

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

colega marked this pull request as draft November 4, 2022 14:41

56quarters reviewed Nov 4, 2022

View reviewed changes

Remove generics as we don't need them now

8633e9b

We'll bring them back once we start using the Prometheus version. Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

colega marked this pull request as ready for review November 4, 2022 15:16

pstibrany approved these changes Nov 7, 2022

View reviewed changes

pracucci approved these changes Nov 7, 2022

View reviewed changes

colega added 2 commits November 7, 2022 13:55

Apply rewiew comments

30b5533

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

Check for lastSet

1d2cfb6

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

colega merged commit 5d144a5 into main Nov 7, 2022

colega deleted the use-specific-types-when-reading-offset-table branch November 7, 2022 14:07

This was referenced Nov 7, 2022

Use unsafe labels when reading the index header #3397

Merged

Use []byte when reading postings offset table #3436

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use specific types when reading offset table #3393

Use specific types when reading offset table #3393

colega commented Nov 4, 2022 •

edited

Loading

56quarters left a comment

56quarters Nov 4, 2022

colega Nov 4, 2022

pstibrany left a comment

pstibrany Nov 7, 2022

colega Nov 7, 2022 •

edited

Loading

pstibrany Nov 7, 2022

colega Nov 7, 2022 •

edited

Loading

pstibrany Nov 7, 2022

pracucci Nov 7, 2022

colega Nov 7, 2022

colega Nov 7, 2022

pstibrany Nov 7, 2022

pracucci Nov 7, 2022

pracucci left a comment

pracucci Nov 7, 2022

Use specific types when reading offset table #3393

Use specific types when reading offset table #3393

Conversation

colega commented Nov 4, 2022 • edited Loading

What this PR does

Which issue(s) this PR fixes or relates to

Checklist

56quarters left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pstibrany left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colega Nov 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colega Nov 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pracucci left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colega commented Nov 4, 2022 •

edited

Loading

colega Nov 7, 2022 •

edited

Loading

colega Nov 7, 2022 •

edited

Loading