storage: avoid allocations for range-local key generation

[petermattis]

Reorganized the range-local key generation code to utilize a new
RangeIDPrefixBuf abstraction. Use this new functionality in
replicaStateKeys to avoid allocations when loading and saving
range-local data.

This removes 2.2% of allocations in a write-only workload.

---

This change is 

[petermattis]

Review status: 0 of 15 files reviewed at latest revision, 2 unresolved discussions.

---

pkg/storage/replica_state.go, line 38 at r1 (raw file):

//
// Note that this struct is not safe for concurrent use. It is usually
// protected by Replica.raftMu or a goroutine local variable is used.

The concurrency issue isn't a big deal in practice as we only call the methods on replicaStateKeys while holding Replica.raftMu.

---

pkg/storage/replica_state.go, line 39 at r1 (raw file):

// Note that this struct is not safe for concurrent use. It is usually
// protected by Replica.raftMu or a goroutine local variable is used.
type replicaStateKeys struct {

I'm not fond of this type name. Suggestions?

---

Comments from Reviewable

[petermattis]

Crap. Data races. Hold off on looking at this.

[petermattis]

Fixed the race. replica_raftstorage.go:snapshot() is called without Replica.raftMu held and this can't use the new Replica.state functionality.

RFAL.

[bdarnell]

, although this seems like a fairly big change for a 2% reduction in allocations.

---

Reviewed 12 of 15 files at r1, 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

pkg/storage/replica_raftstorage.go, line 358 at r2 (raw file):

// snapshot creates an OutgoingSnapshot containing a rocksdb snapshot for the
// given range. Note that snapshot() is called without Replica.raftMu held.

It's called without even having access to a Replica (and in fact this is the whole reason it's a static function instead of a method, to guarantee that it depends only on the engine.Reader)

---

pkg/storage/replica_state.go, line 39 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

I'm not fond of this type name. Suggestions?

replicaStateLoader?

---

Comments from Reviewable

[petermattis]

seems like a fairly big change

Well, large parts of it are fairly mechanical.

---

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

---

pkg/storage/replica_raftstorage.go, line 358 at r2 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

It's called without even having access to a Replica (and in fact this is the whole reason it's a static function instead of a method, to guarantee that it depends only on the engine.Reader)

Well, we could pass the Replica now. The only caller is Replica.GetSnapshot. The comment is meant to indicate that we shouldn't.

---

pkg/storage/replica_state.go, line 39 at r1 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

replicaStateLoader?

Ok. I'll change in a bit.

---

Comments from Reviewable