sstable: lazily allocate ValueFetcher

[RaduBerinde]

Currently the type valueBlockReader implements both
block.GetLazyValueForPrefixAndValueHandler and base.ValueFetcher.
The lifetime of the ValueFetcher needs to extend beyond that
of the iterator.

This commit splits it into two objects - valueBlockReader continues
to implement block.GetLazyValueForPrefixAndValueHandler but the new
valueBlockFetcher now implements base.ValueFetcher. The
valueBlockReader lazily allocates the valueBlockFetcher the first
time it is asked to produce a LazyValue. The result is that we
should avoid the allocation if we are never positioned on a KV with
non-in-place value (which is the norm when we get the latest version
of a single key).

name                              old time/op    new time/op    delta
RandSeekInSST/v4/single-level-10     691ns ± 2%     668ns ± 3%    -3.38%  (p=0.000 n=8+7)
RandSeekInSST/v4/two-level-10       1.57µs ± 2%    1.54µs ± 4%      ~     (p=0.067 n=7+8)
RandSeekInSST/v5/single-level-10     479ns ± 1%     425ns ± 1%   -11.28%  (p=0.001 n=7+7)
RandSeekInSST/v5/two-level-10        967ns ± 4%     868ns ± 4%   -10.25%  (p=0.001 n=7+7)

name                              old alloc/op   new alloc/op   delta
RandSeekInSST/v4/single-level-10      288B ± 0%      122B ± 2%   -57.81%  (p=0.000 n=8+8)
RandSeekInSST/v4/two-level-10         288B ± 0%      122B ± 1%   -57.74%  (p=0.000 n=8+7)
RandSeekInSST/v5/single-level-10      288B ± 0%       11B ± 0%   -96.18%  (p=0.000 n=8+7)
RandSeekInSST/v5/two-level-10         288B ± 0%       11B ± 0%   -96.18%  (p=0.000 n=8+8)

name                              old allocs/op  new allocs/op  delta
RandSeekInSST/v4/single-level-10      1.00 ± 0%      0.00       -100.00%  (p=0.000 n=8+8)
RandSeekInSST/v4/two-level-10         1.00 ± 0%      0.00       -100.00%  (p=0.000 n=8+8)
RandSeekInSST/v5/single-level-10      1.00 ± 0%      0.00       -100.00%  (p=0.000 n=8+8)
RandSeekInSST/v5/two-level-10         1.00 ± 0%      0.00       -100.00%  (p=0.000 n=8+8)

[cockroach-teamcity]

This change is 

[sumeerbhola]

nice!

Reviewed 5 of 5 files at r1.
Reviewable status: all files reviewed (commit messages unreviewed), 4 unresolved discussions (waiting on @jbowens and @RaduBerinde)

---

sstable/value_block.go line 725 at r1 (raw file):

// valueBlockReader implements GetLazyValueForPrefixAndValueHandler; it is used
// to create a LazyValue that can be used to retrieve a values in a value block.

nit: a value ...

---

sstable/value_block.go line 752 at r1 (raw file):

		//
		// Since it is a relatively small object, we could allocate multiple
		// instances together, using a sync.Pool.

I didn't understand this TODO, in that we don't have a good way to put back in the sync.Pool. Could you elaborate?

---

sstable/value_block.go line 856 at r1 (raw file):

	f.vbiCache.Release()
	// Set the handle to empty since Release does not nil the Handle.value. If
	// we were to reopen this valueBlockReader and retrieve the same

nit: reopen this valueBlockFetcher`.

---

sstable/block/kv.go line 77 at r1 (raw file):

	// GetLazyValueForPrefixAndValueHandle returns a LazyValue for the given value
	// prefix and value. The result is only valid until the next call to
	// GetLazyValueForPrefixAndValueHandle.

Can you add:
Use LazyValue.Clone if the lifetime of the LazyValue needs to be extended. For more details, see the "memory management" comment where LazyValue is declared.

[RaduBerinde]

TFTR!

Reviewable status: 3 of 5 files reviewed, 2 unresolved discussions (waiting on @jbowens and @sumeerbhola)

---

sstable/value_block.go line 725 at r1 (raw file):

Previously, sumeerbhola wrote…

nit: a value ...

Fixed. Does the name valueBlockReader still make sense? It's more like a "valueBlockLazyValueConstructor` (but that sounds pretty terrible)

---

sstable/value_block.go line 752 at r1 (raw file):

Previously, sumeerbhola wrote…

I didn't understand this TODO, in that we don't have a good way to put back in the sync.Pool. Could you elaborate?

Pool object would be something like:

struct {
  nUsed int
  instances[16]
}

Alloc is: get an object from the pool, use &instances[nUsed], increase nUsed, then put it back in the pool if nUsed < 16.

Basically just a bulk allocator (but using sync.Pool instead of a single structure with a mutex).

Improved the comment.

---

sstable/block/kv.go line 77 at r1 (raw file):

Previously, sumeerbhola wrote…

Can you add:
Use LazyValue.Clone if the lifetime of the LazyValue needs to be extended. For more details, see the "memory management" comment where LazyValue is declared.

Done.