engine: pool rocksDBBatch and RocksDBBatchBuilder allocations

[nvanbenschoten]

This addresses the first three items from #30512.

I'm not seeing any big throughput improvements when running kv0
on my laptop (maybe ~1%, hard to tell), but I can see that this worked.
Total allocated space due to the three items in the issue dropped from
8.95% (1137.04 MB over a 1m run) to 0.32% (37.45 MB over a 1m run).

Before

After

Release note (performance improvement): Pool allocations of rocksDBBatch
and RocksDBBatchBuilder objects.

[cockroach-teamcity]

This change is 

[cockroach-teamcity]

This change is 

[nvanbenschoten]

If people like this then I'll probably follow it up with a similar change to pool rocksDBReadOnly objects. They're only responsible for 0.65% of allocated space in this TPC-C run, but their lifetimes are so clean that I think it makes sense to do.

[nvanbenschoten]

If people like this then I'll probably follow it up with a similar change to pool rocksDBReadOnly objects. They're only responsible for 0.65% of allocated space in this TPC-C run, but their lifetimes are so clean that I think it makes sense to do.

[petermattis]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/storage/engine/batch.go, line 105 at r1 (raw file):

func (b *RocksDBBatchBuilder) reset() {
	if b.repr != nil {
		b.repr = b.repr[:headerSize]

If an unusually large batch was written, this could cause the memory to be held on to until the pool is cleaned. Do we want to worry about that?

---

pkg/storage/engine/rocksdb.go, line 1944 at r1 (raw file):

		cpy := make([]byte, len(repr))
		copy(cpy, repr)
		return cpy

Did you audit all the calls to Repr()? I'm worried this could be a pessimization because my expectation was that Repr() didn't allocate and now it does. For example, kvBatchSnapshotStrategy.Send seems to call Batch.Repr() in a loop.

[a-robinson]

Nothing to add beyond @petermattis's comments

Reviewed 3 of 3 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained

[nvanbenschoten]

Sorry about sitting on this for so long. It wasn't going to make it into 2.1 so I put it on the backburner.

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/storage/engine/batch.go, line 105 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

If an unusually large batch was written, this could cause the memory to be held on to until the pool is cleaned. Do we want to worry about that?

Heh, @jordanlewis and I had a discussion about this before we began auditing allocations. The conclusion we came to was that the pool will flush all objects on each GC, so there's a low chance that an oversized object will live indefinitely.

Still, I've had the concern before and it's generally a good idea to play it safe, so I'm happy to add in some protection here. How does a 1MB retention limit sound?

---

pkg/storage/engine/rocksdb.go, line 1944 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Did you audit all the calls to Repr()? I'm worried this could be a pessimization because my expectation was that Repr() didn't allocate and now it does. For example, kvBatchSnapshotStrategy.Send seems to call Batch.Repr() in a loop.

That's a great point. Done.

Splitting out a separate Len method is a win on its own because we still were allocating in Repr when flushing mutations. This means that kvBatchSnapshotStrategy.Send was almost certainly allocating on each len(b.Repr()) call already.

[petermattis]

Yeah, this isn't appropriate for a backport.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

---

pkg/storage/engine/batch.go, line 105 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Heh, @jordanlewis and I had a discussion about this before we began auditing allocations. The conclusion we came to was that the pool will flush all objects on each GC, so there's a low chance that an oversized object will live indefinitely.

Still, I've had the concern before and it's generally a good idea to play it safe, so I'm happy to add in some protection here. How does a 1MB retention limit sound?

1MB sounds good. Note that GC runs several times a second on small heaps, but can slow down to a run every few seconds on large heaps.