internal/manifest: specialize LevelIterator seek methods

[jbowens]

Previously LevelIterator's seek methods accepted an opaque
func(*TableMetadata)bool to perform comparisons. This commit specializes each
instance as separate functions, ensuring comparisons can be inlined where
possible. Special attention is given to SeekGE, which is the hot path for point
lookups.

goos: darwin
goarch: arm64
pkg: github.com/cockroachdb/pebble/internal/manifest
cpu: Apple M1 Pro
                       │   old.txt   │             newest2.txt             │
                       │   sec/op    │   sec/op     vs base                │
LevelIteratorSeekGE-10   144.3n ± 2%   110.0n ± 1%  -23.74% (p=0.000 n=10)

[cockroach-teamcity]

This change is 

[RaduBerinde]

Nice!

Consider adding some unit tests for the new iterator methods

Reviewable status: 0 of 4 files reviewed, 8 unresolved discussions (waiting on @annrpom)

---

internal/manifest/btree.go line 1099 at r2 (raw file):

// seekSeqNumL0 seeks an iterator over L0 files (ordered by sequence number) to
// the provided table metadata if it exists.

[nit] And if it doesn't? Next file in L0 ordering?

---

internal/manifest/btree.go line 1127 at r2 (raw file):

			return
		}
		i.descend(i.n, i.pos)

descend() pushes onto i.s and we don't need that here, we could just inline the part we need

---

internal/manifest/btree.go line 1131 at r2 (raw file):

}

// seekLargest repositions the iterator over the first table whose largest key

seekLargestGE

---

internal/manifest/btree.go line 1133 at r2 (raw file):

// seekLargest repositions the iterator over the first table whose largest key
// is an upper bound for the given user key. seekLargest requires the iterator's
// B-Tree to be ordered by user keys (i.e, L1+ or a single sublevel of L0).

[nit] If there is no such table, positions the iterator before the first table.

---

internal/manifest/btree.go line 1170 at r2 (raw file):

}

// seekSmallest repositions the iterator over the first table whose smallest key

seekSmallestGT

---

internal/manifest/btree.go line 1171 at r2 (raw file):

// seekSmallest repositions the iterator over the first table whose smallest key
// is a lower bound for the given user key. seekSmallest requires the iterator's

is NOT a lower bound! (better yet just say it is > userKey)

You can also consider making this return the last table with smallest key < userKey, and don't do Prev() in the calling code. We'd have to modify the code below like this:

			h := int(uint(j+k+1) >> 1) // avoid overflow when computing h
			// j < h ≤ k
			if cmp(i.n.items[h].Smallest().UserKey, userKey) < 0 {
				j = h  // preserves INVARIANT A
			} else {
				k = h-1 // preserves INVARIANT B
			}

---

internal/manifest/btree.go line 1173 at r2 (raw file):

// is a lower bound for the given user key. seekSmallest requires the iterator's
// B-Tree to be ordered by user keys (i.e, L1+ or a single sublevel of L0).
func (i *iterator) seekSmallest(cmp base.Compare, userKey []byte) {

If there is no such table, positions the iterator after the last table.

---

internal/manifest/btree.go line 1188 at r2 (raw file):

			// j ≤ h < k
			if cmp(i.n.items[h].Smallest().UserKey, userKey) < 0 {
				j = h + 1 // preserves INVARIANT A

Are you sure about this? This table could still be the one we're looking for if the next table starts after userKey? Feels like this should be:

			h := int(uint(j+k+1) >> 1) // avoid overflow when computing h
			// j ≤ h < k
			if cmp(i.n.items[h].Smallest().UserKey, userKey) <= 0 {
				j = h  // preserves INVARIANT A
			} else {
				k = h-1 // preserves INVARIANT B
			}

[jbowens]

Reviewable status: 0 of 5 files reviewed, 7 unresolved discussions (waiting on @annrpom and @RaduBerinde)

---

internal/manifest/btree.go line 1099 at r2 (raw file):

Previously, RaduBerinde wrote…

[nit] And if it doesn't? Next file in L0 ordering?

Updated this to use the btree node find func, so that all calls to Find can make use.

---

internal/manifest/btree.go line 1127 at r2 (raw file):

Previously, RaduBerinde wrote…

descend() pushes onto i.s and we don't need that here, we could just inline the part we need

We do need the stack bit because the expectation is that the iterator is now positioned over the provided file.

---

internal/manifest/btree.go line 1131 at r2 (raw file):

Previously, RaduBerinde wrote…

seekLargestGE

I ended up inlining this func because it's only called once, and the semantics of the seekSmallestLT func are closely intertwined with the semantics of LevelIterator.SeekLT

---

internal/manifest/btree.go line 1170 at r2 (raw file):

Previously, RaduBerinde wrote…

seekSmallestGT

I ended up inlining this func because it's only called once, and the semantics of this func are closely intertwined with the semantics of LevelIterator.SeekLT

---

internal/manifest/btree.go line 1171 at r2 (raw file):

Previously, RaduBerinde wrote…

is NOT a lower bound! (better yet just say it is > userKey)

You can also consider making this return the last table with smallest key < userKey, and don't do Prev() in the calling code. We'd have to modify the code below like this:

			h := int(uint(j+k+1) >> 1) // avoid overflow when computing h
			// j < h ≤ k
			if cmp(i.n.items[h].Smallest().UserKey, userKey) < 0 {
				j = h  // preserves INVARIANT A
			} else {
				k = h-1 // preserves INVARIANT B
			}

If our seek key is less than all tables smallest keys, we want to ultimately end up before the first table. This is also complicated by the iterator start and end bounds which are both inclusive

[jbowens]

@RaduBerinde i made some material changes if u want to take another look

Reviewable status: 0 of 5 files reviewed, 7 unresolved discussions (waiting on @annrpom and @RaduBerinde)

[RaduBerinde]

Reviewable status: 0 of 5 files reviewed, 8 unresolved discussions (waiting on @annrpom and @jbowens)

---

internal/manifest/level_metadata.go line 588 at r7 (raw file):

		if i.iter.n.leaf {
			if i.iter.pos == i.iter.n.count {
				i.iter.next()

[nit] I guess it doesn't matter, but it would be more intuitive if we set i.iter.pos to i.iter.n.count-1 in this case.

[jbowens]

TFTR!