Add alternate implementation of index-header reader that does not use mmap

[charleskorn]

What this PR does

This PR adds an alternate implementation of the store gateway's index-header reader that does not use mmap. The motivations for this change and references are covered in #3465.

cc: @56quarters, @stevesg

Which issue(s) this PR fixes or relates to

#3465

Notes to reviewers

After review is finished, DO NOT MERGE as-is. @charleskorn and @56quarters will handle fixing up the squashed message.

The most important parts of this PR to examine:

• When the experimental streaming index-header reader is disabled, everything should work exactly as it does today.
• There should be nothing fundamentally wrong with this architecture that will prevent us from switching over to it entirely in the future.

We expect the performance of this implementation to not be equivalent to the existing mmap implementation at first. The goal of this PR is not to merge a perfect replacement that we'll immediately be able to switch to in production. Instead, we'd like to get things into main as they are so that we can iterate on performance, making small changes, based on further testing.

Changes

There's a lot of commits here and it doesn't make sense to review them individually. However, there's also a lot of code here. The organization of our changes is described below. Some knowledge of the current architecture of the store-gateway is assumed.

(note that all paths are relative to pkg/storegateway)

Encoding components

• indexheader/encoding/reader.go/FileReader: this is the lowest level part of this change. This is a buffering wrapper around file operations that provides some convenience methods for seeking, reading, peeking.
• indexheader/encoding/encoding.go/Decbuf: this is a copy / modification of the equivalent Prometheus file. It includes methods for reading integers, strings, and bytes from binary files using a FileReader instance. This is the important part: it uses standard go file operations that the goruntime knows how to schedule if they block. Compare this to mmap which causes invisible blocking operations that hang the entire store-gateway.
• indexheader/encoding/factory.go/DecbufFactory: this code is responsible for creating new Decbuf instances in a variety of ways. It is also responsible for cleaning up after Decbuf instances are no longer needed. This is because it pools the underlying bufio.Readers used.

TSDB index components

• indexheader/index/symbols.go/Symbols: this is a copy/modification of the equivalent Prometheus file. It includes a modified Symbol reader that works with our DecbufFactory (which in turn uses standard go file operations). It also includes a bulk API for reverse lookups since this is required by the StreamBinaryReader
• indexheader/index/positings.go/PostingOffsetTable: an interface abstracting the differences between how index v1 postings are looked up vs index v2 postings. The existing BinaryReader used a bunch of if statements to pick how to work on postings. We feel this is cleaner. This file contains the v1 and v2 implementations of this interface using largely the same logic as the existing BinaryReader.

New index-header Reader

• indexheader/stream_binary_reader.go/StreamBinaryReader: the alternate implementation of the Reader interface that uses all the previously described file-based logic for reading index-headers from disk. The interesting part of this class is the logic run when instances are first created: the index-header table-of-contents is loaded, symbols are loaded and validated, postings are loaded and validated.
• indexheader/reader_pool.go/ReaderPool: there are some limited changes here to allow the StreamBinaryReader to be used when index-header Readers are lazy loaded or eagerly loaded.

Checklist

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

[charleskorn]

The CI build for this PR has been stuck in "Expected - waiting for status to be reported" for two days now. However, there's no CI build queued according to GitHub Actions.

The internet suggests closing and reopening the PR might get things going again, so I'm going to try that...

[colega]

Submitting a (very) partial review

[pracucci]

I reviewed all the logic except the tests. You did an impressive work! 👏 The code is well documented, and you kept the Prometheus code as much as possible so doing a side-by-side diff of the logic has been trivial. Very good job!

I haven't found any issue (in a couple of cases actually depend on the answer to some questions). I left few comments and questions. Thanks!

[pracucci]

This looks very small. Thoughts on this?

[56quarters]

I don't have any justification for this number. Since they're being pooled, I suppose it doesn't hurt to make them larger. My only concern would be if increasing the size of this buffer means os.File.Read() reads more data every time it's invoked.

[pracucci]

These are the questions we should answer running a benchmark on a real TSDB block.

[colega]

I would add a comment saying that this number was chosen without any justification, so future readers or maintainers would not hesitate to tune it if benchmarks show that it can be improved. (Otherwise anyone reading this might think "oh, there probably was a good reason to have this number")

[pracucci]

[non blocking] My biggest concern is that we open a file description each time. It's called in called in hot path, like Symbols.Lookup() and Symbols.ReverseLookup(). Not a blocker for this PR, but I would like to hear your thoughts on this.

[56quarters]

I've been worrying about this but after deploying a test image in a dev cluster, opening the file doesn't seem to account for much CPU time (according to phlare). The zone with this index-header reader enabled seems to be about as fast as the existing mmap implementation as far as I can tell.

If it turned out to be an issue, I was thinking about maintaining a (small) pool of file handles for each index-header.

[56quarters]

Actually, maybe it is significant. Looking more closely at the profile: Symbols.Lookup() is about 0.27% of total time and os.OpenFile called by Symbols.Lookup() is 0.12% of total time (based on the load test that we've been running). So I guess 1/2 the time spent by the symbol reader is opening the file even if it's not a large part of time spent overall. I'll add pooling of file handles to the list of future performance improvements.

[pracucci]

I'll add pooling of file handles to the list of future performance improvements.

👍

[pracucci]

Is it safe to directly return b? Could the underlying buffer be reused?

[56quarters]

It's safe to return and use until the next read of the underlying FileReader. After that, the bytes are invalidated. FileReader.Skip() (via bufio.Reader.Discard()) is guaranteed not to read from disk as long as n is less than the amount of data buffered. Because of the Peek(n) call above, we know that at least n bytes are buffered.

I was wondering about this today and was in the process of writing a unit test to verify. I'll push that test along with other code review changes.

[56quarters]

Fixed an instance where we were doing this efea391

All other uses of UvarintBytes are in order to skip bytes or immediately yoloStringd and compared with another string (such as the binary search Symbols does).

[pracucci]

It's safe to return and use until the next read of the underlying FileReader

I would add a comment to this function to explain that.

[56quarters]

The docs for this method mention it but I'll make it more prominent.

[pracucci]

Amazing job! I'm approving it. The logic looks very solid to me and, in the spirit outlined in the PR description, I would merge it by EOW so that code will be rolled out (disabled) next week to dev. However, I would wait for @colega review too and block the merge if he has any concern.

[colega]

Logic looks solid, I think we can merge this.

I've left some nitpicks, mostly style-related and also comments to revisit with performance-improvements.

There's a bunch of hard to follow code that was brought from Prometheus where it was already hard to follow, so I'm not commenting on that.

[colega]

There's no need to YOLO just for a comparison: golang/go@69cd91a

Suggested change

	return yoloString(d.UvarintBytes()) > sym
	return string(d.UvarintBytes()) > sym

Although that test only checks equality, I tested with comparison and it also doesn't allocate:

func TestCompareTempString(t *testing.T) {
	s := "foo"
	b := []byte(s)
	n := testing.AllocsPerRun(1000, func() {
		if string(b) > s {
			t.Fatalf("strings are not equal: '%v' and '%v'", string(b), s)
		}
		if string(b) < s {
			t.Fatalf("strings are not equal: '%v' and '%v'", string(b), s)
		}
	})
	if n != 0 {
		t.Fatalf("want 0 allocs, got %v", n)
	}
}

[56quarters]

Nice. I'll make the change to use string everywhere we're just doing a comparison.

[colega]

Same as above, this package aliases should be streamencoding and streamindex.

[colega]

Nit, but this should be checked right after reading the version. We can't assume that in future versions the index version would be the next thing to read.