storage/engine: batch allocations in MVCCIterate

[petermattis]

name                old time/op    new time/op    delta
KVScan1_SQL-32         206µs ± 4%     208µs ± 6%     ~     (p=0.365 n=19+20)
KVScan10_SQL-32        291µs ± 6%     287µs ± 5%     ~     (p=0.134 n=20+20)
KVScan100_SQL-32      1.00ms ± 9%    0.95ms ± 8%   -4.73%  (p=0.000 n=20+19)
KVScan1000_SQL-32     7.80ms ± 4%    7.51ms ± 8%   -3.66%  (p=0.000 n=20+19)
KVScan10000_SQL-32    76.9ms ± 4%    73.4ms ± 6%   -4.59%  (p=0.000 n=19+18)

name                old allocs/op  new allocs/op  delta
KVScan1_SQL-32           208 ± 0%       205 ± 0%   -1.44%  (p=0.000 n=20+20)
KVScan10_SQL-32          299 ± 0%       260 ± 0%  -13.04%  (p=0.000 n=19+18)
KVScan100_SQL-32       1.14k ± 0%     0.74k ± 0%  -34.72%  (p=0.000 n=20+20)
KVScan1000_SQL-32      9.45k ± 0%     5.46k ± 0%  -42.26%  (p=0.000 n=19+18)
KVScan10000_SQL-32     92.5k ± 0%     52.6k ± 0%  -43.18%  (p=0.000 n=20+19)

---

This change is

[petermattis]

@bdarnell The effects of these allocation reductions do add up. In the table below "old" refers to 61be7da which is just before the recent spate of optimizations.

name                old time/op    new time/op    delta
KVScan1_SQL-32         215µs ± 5%     208µs ± 6%   -2.89%  (p=0.001 n=20+20)
KVScan10_SQL-32        310µs ± 6%     287µs ± 5%   -7.34%  (p=0.000 n=20+20)
KVScan100_SQL-32      1.09ms ± 5%    0.95ms ± 8%  -12.54%  (p=0.000 n=20+19)
KVScan1000_SQL-32     8.67ms ± 3%    7.51ms ± 8%  -13.36%  (p=0.000 n=19+19)
KVScan10000_SQL-32    87.7ms ±17%    73.4ms ± 6%  -16.36%  (p=0.000 n=20+18)

name                old allocs/op  new allocs/op  delta
KVScan1_SQL-32           229 ± 0%       205 ± 0%  -10.48%  (p=0.000 n=20+20)
KVScan10_SQL-32          390 ± 0%       260 ± 0%  -33.33%  (p=0.000 n=19+18)
KVScan100_SQL-32       1.93k ± 0%     0.74k ± 0%  -61.48%  (p=0.000 n=20+20)
KVScan1000_SQL-32      17.3k ± 0%      5.5k ± 0%  -68.38%  (p=0.000 n=16+18)
KVScan10000_SQL-32      171k ± 0%       53k ± 0%  -69.17%  (p=0.000 n=18+19)

[dt]

Review status: 0 of 2 files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

---

storage/engine/mvcc.go, line 555 [r1] (raw file):
This feels a little weird to me as state on the getBuffer. Would be easier to reason about if it were just a bool arg to getInternal instead?

---

storage/engine/mvcc.go, line 738 [r1] (raw file):
Is isUnsafeValue ever true here?

---

storage/engine/mvcc.go, line 1589 [r1] (raw file):
should isUnsafeValue get flipped back to false here?

---

Comments from Reviewable

[petermattis]

Review status: 0 of 2 files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

---

storage/engine/mvcc.go, line 555 [r1] (raw file):
This could definitely be a bool arg, but I don't want an isUnsafeValue *bool parameter as well because that would be stack allocated. I could make isUnsafeValue a return value, but then I would have to change a bunch of mvccGetInternal callers which don't care. My preference is to leave as is, but push-back if you think there is a cleaner way.

---

storage/engine/mvcc.go, line 738 [r1] (raw file):
Probably not. This is just for paranoia.

---

storage/engine/mvcc.go, line 1589 [r1] (raw file):
Yeah, good idea. Done.

---

Comments from Reviewable

[dt]

Review status: 0 of 2 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

storage/engine/mvcc.go, line 555 [r1] (raw file):
Fair, I just find it a little harder to reason about behavior controlled by state not in the function call.

What about passing a chunkAllocator to mvccGetInternal (or maybe even all the way down to Iterator) so it just returns safe values, (should be the same since we we're just copying right after anyway, right?)

---

Comments from Reviewable

[petermattis]

Review status: 0 of 2 files reviewed at latest revision, 3 unresolved discussions, some commit checks pending.

---

storage/engine/mvcc.go, line 555 [r1] (raw file):
That's an interesting idea, though I wanted to restrict the use of chunkAllocator to this one use case. Notice that chunkAllocator is a slice that starts off as nil. How would I indicate in the iterator that I wanted to use the allocator vs not?

I've pushed another commit that adds a parameter and return value to mvccGetInternal. PTAL and let me know if it is more palatable.

---

Comments from Reviewable

[dt]

---

Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

storage/engine/mvcc.go, line 555 [r1] (raw file):
👍
I find this easier to reason about and less prone to mistakes.

---

Comments from Reviewable