storage/spanlatch: create spanlatch.Manager using immutable btrees

[nvanbenschoten]

Informs #4768.
Informs #31904.

This change was inspired by #31904 and is a progression of the thinking started in #4768 (comment).

The change introduces spanlatch.Manager, which will replace the CommandQueue in a future PR. The new type isn't hooked up yet because doing so will require a lot of plumbing changes in that storage package that are best kept in a separate PR. The structure uses a new strategy that reduces lock contention, simplifies the code, avoids allocations, and makes #31904 easier to implement.

The primary objective, reducing lock contention, is addressed by minimizing the amount of work we perform under the exclusive "sequencing" mutex while locking the structure. This is made possible by employing a copy-on-write strategy. Before this change, commands would lock the queue, create a large slice of prerequisites, insert into the queue and unlock. After the change, commands lock the manager, grab an immutable snapshot of the manager's trees in O(1) time, insert into the manager, and unlock. They can then iterate over the immutable tree snapshot outside of the lock. Effectively, this means that the work performed under lock is linear with respect to the number of spans that a command declares but NO LONGER linear with respect to the number of other commands that it will wait on. This is important because Replica.beginCmds repeatedly comes up as the largest source of mutex contention in our system, especially on hot ranges.

The use of immutable snapshots also simplifies the code significantly. We're no longer copying our prereqs into a slice so we no longer need to carefully determine which transitive dependencies we do or don't need to wait on explicitly. This also makes lock cancellation trivial because we no longer explicitly hold on to our prereqs at all. Instead, we simply iterate through the snapshot outside of the lock.

While rewriting the structure, I also spent some time optimizing its allocations. Under normal operation, acquiring a latch now incurs only a single allocation - that being for the spanlatch.Guard. All other allocations are avoided through object pooling where appropriate. The overhead of using a copy-on-write technique is almost entirely avoided by atomically reference counting immutable btree nodes, which allows us to release them back into the btree node pools when they're no longer needed. This means that we don't expect any allocations when inserting into the internal trees, even with the copy-on-write policy.

Finally, this will make the approach taken in #31904 much more natural. Instead of tracking dependents and prerequisites for speculative reads and then iterating through them to find overlaps after, we can use the immutable snapshots directly! We can grab a snapshot and sequence ourselves as usual, but avoid waiting for prereqs. We then execute optimistically before finally checking whether we overlapped any of our prereqs. The great thing about this is that we already have the prereqs in an interval tree structure, so we get an efficient validation check for free.

Naming changes

Before	After
CommandQueue	spanlatch.Manager
"enter the command queue"	"acquire span latches"
"exit the command queue"	"release span latches"
"wait for prereq commands"	"wait for latches to be released"

The use of the word "latch" is based on the definition of latches presented by Goetz Graefe in https://15721.courses.cs.cmu.edu/spring2016/papers/a16-graefe.pdf (see https://i.stack.imgur.com/fSRzd.png). An important reason for avoiding the word "lock" here is that it is critical for understanding that we don't confuse the operational locking performed by the CommandQueue/spanlatch.Manager with the transaction-scoped locking enforced by intents and our transactional concurrency control model.

Microbenchmarks

NOTE: these are single-threaded benchmarks that don't benefit at all from the concurrency improvements enabled by this new structure.

name                              old time/op    new time/op    delta
ReadOnlyMix/size=1-4                 706ns ±20%     404ns ±10%  -42.81%  (p=0.008 n=5+5)
ReadOnlyMix/size=4-4                 649ns ±23%     382ns ± 5%  -41.13%  (p=0.008 n=5+5)
ReadOnlyMix/size=16-4                611ns ±16%     367ns ± 5%  -39.83%  (p=0.008 n=5+5)
ReadOnlyMix/size=64-4                692ns ±14%     370ns ± 1%  -46.49%  (p=0.016 n=5+4)
ReadOnlyMix/size=128-4               637ns ±22%     398ns ±14%  -37.48%  (p=0.008 n=5+5)
ReadOnlyMix/size=256-4               676ns ±15%     385ns ± 4%  -43.01%  (p=0.008 n=5+5)
ReadWriteMix/readsPerWrite=0-4      12.2µs ± 4%     0.6µs ±17%  -94.85%  (p=0.008 n=5+5)
ReadWriteMix/readsPerWrite=1-4      7.88µs ± 2%    0.55µs ± 7%  -92.99%  (p=0.008 n=5+5)
ReadWriteMix/readsPerWrite=4-4      4.19µs ± 3%    0.58µs ± 5%  -86.26%  (p=0.008 n=5+5)
ReadWriteMix/readsPerWrite=16-4     2.09µs ± 6%    0.54µs ±13%  -74.13%  (p=0.008 n=5+5)
ReadWriteMix/readsPerWrite=64-4      875ns ±17%     423ns ±29%  -51.64%  (p=0.008 n=5+5)
ReadWriteMix/readsPerWrite=128-4     655ns ± 6%     362ns ±16%  -44.71%  (p=0.008 n=5+5)
ReadWriteMix/readsPerWrite=256-4     549ns ±16%     314ns ±13%  -42.73%  (p=0.008 n=5+5)

name                              old alloc/op   new alloc/op   delta
ReadOnlyMix/size=1-4                  223B ± 0%      160B ± 0%  -28.25%  (p=0.079 n=4+5)
ReadOnlyMix/size=4-4                  223B ± 0%      160B ± 0%  -28.25%  (p=0.008 n=5+5)
ReadOnlyMix/size=16-4                 223B ± 0%      160B ± 0%  -28.25%  (p=0.008 n=5+5)
ReadOnlyMix/size=64-4                 223B ± 0%      160B ± 0%  -28.25%  (p=0.008 n=5+5)
ReadOnlyMix/size=128-4                217B ± 4%      160B ± 0%  -26.27%  (p=0.008 n=5+5)
ReadOnlyMix/size=256-4                223B ± 0%      160B ± 0%  -28.25%  (p=0.079 n=4+5)
ReadWriteMix/readsPerWrite=0-4      1.25kB ± 0%    0.16kB ± 0%  -87.15%  (p=0.008 n=5+5)
ReadWriteMix/readsPerWrite=1-4      1.00kB ± 0%    0.16kB ± 0%  -84.00%  (p=0.079 n=4+5)
ReadWriteMix/readsPerWrite=4-4        708B ± 0%      160B ± 0%  -77.40%  (p=0.079 n=4+5)
ReadWriteMix/readsPerWrite=16-4       513B ± 0%      160B ± 0%  -68.81%  (p=0.008 n=5+5)
ReadWriteMix/readsPerWrite=64-4       264B ± 0%      160B ± 0%  -39.39%  (p=0.008 n=5+5)
ReadWriteMix/readsPerWrite=128-4      221B ± 0%      160B ± 0%  -27.60%  (p=0.079 n=4+5)
ReadWriteMix/readsPerWrite=256-4      198B ± 0%      160B ± 0%  -19.35%  (p=0.008 n=5+5)

name                              old allocs/op  new allocs/op  delta
ReadOnlyMix/size=1-4                  1.00 ± 0%      1.00 ± 0%     ~     (all equal)
ReadOnlyMix/size=4-4                  1.00 ± 0%      1.00 ± 0%     ~     (all equal)
ReadOnlyMix/size=16-4                 1.00 ± 0%      1.00 ± 0%     ~     (all equal)
ReadOnlyMix/size=64-4                 1.00 ± 0%      1.00 ± 0%     ~     (all equal)
ReadOnlyMix/size=128-4                1.00 ± 0%      1.00 ± 0%     ~     (all equal)
ReadOnlyMix/size=256-4                1.00 ± 0%      1.00 ± 0%     ~     (all equal)
ReadWriteMix/readsPerWrite=0-4        38.0 ± 0%       1.0 ± 0%  -97.37%  (p=0.008 n=5+5)
ReadWriteMix/readsPerWrite=1-4        24.0 ± 0%       1.0 ± 0%  -95.83%  (p=0.008 n=5+5)
ReadWriteMix/readsPerWrite=4-4        12.0 ± 0%       1.0 ± 0%  -91.67%  (p=0.008 n=5+5)
ReadWriteMix/readsPerWrite=16-4       5.00 ± 0%      1.00 ± 0%  -80.00%  (p=0.008 n=5+5)
ReadWriteMix/readsPerWrite=64-4       2.00 ± 0%      1.00 ± 0%  -50.00%  (p=0.008 n=5+5)
ReadWriteMix/readsPerWrite=128-4      1.00 ± 0%      1.00 ± 0%     ~     (all equal)
ReadWriteMix/readsPerWrite=256-4      1.00 ± 0%      1.00 ± 0%     ~     (all equal)

There are a few interesting things to point about about these benchmark results:

• The ReadOnlyMix results demonstrate a fixed improvement, regardless of size. This is due to the replacement of the hash-map with a linked-list for the readSet structure.
• The ReadWriteMix is more interesting. We see that the spanlatch implementation is faster across the board. This is especially true with a high write/read ratio.
• We see that the allocated memory stays constant regardless of the write/read ratio in the spanlatch implementation. This is due to the memory recylcing that it performs on btree nodes. It is not the case for the CommandQueue implementation.

Release note: None

[cockroach-teamcity]

This change is 

[nvanbenschoten]

There's still plenty of room to optimize the btree implementation that we use here. Until this point we've closely follower github.com/google/btree, but there are some good ideas that we could pull from pebble's btree. For instance, we could inline item and node pointer arrays in nodes to avoid unnecessary indirection. We could also specialize the implementation to this use to avoid interfaces. Short of this, we could at least cache the interface's Range() value to avoid repeatedly calling the method when searching.

[nvanbenschoten]

I've update this PR to use the interval btree type and the signal type added in #32165, #32251, #32164 to create a new spanlatch.Manager. The commits here don't replace the CommandQueue yet, but that's the immediate follow-up to this PR.

[nvanbenschoten]

A few things to note about the testing here:

1. I copied the CommandQueue benchmarks so that we could perform apples-to-apples microbenchmark comparisons. The results are posted above.
2. The ReadOnlyMix benchmarks aren't particularly interesting because read-only access without any intermingled writes doesn't hit the interval trees in either implementation. The ReadWriteMix variants are much more telling.
3. the tests in manager_test.go are direct adaptations of the ones in command_queue_test.go.

[ajwerner]

I definitely need another pass. This is just the nits I've spotted in the first skim

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/storage/spanlatch/doc.go, line 35 at r5 (raw file):

      key ranges was permitted. Conceptually, the structure became an interval
      tree of sync.RWMutexes.
    * The structure become timestamp-aware and concurrent access of non-causal

s/become/became/

---

pkg/storage/spanlatch/manager.go, line 77 at r5 (raw file):

}

// latches are stored in the Manager's btrees. The represent the latching of a

s/The/They/

---

pkg/storage/spanlatch/manager.go, line 137 at r5 (raw file):

	}

	// Guard would be an ideal candidate for object pooling, but without

Nit: move the guard and latch construction to a helper

---

pkg/storage/spanlatch/manager.go, line 280 at r5 (raw file):

	realloc := len(sm.rSet) > 16
	for latch := range sm.rSet {
		latch.setInRSet(false)

if the TODO is about exploiting the efficient map clearing idiom, I think it needs to be the only statement in the loop https://go-review.googlesource.com/c/go/+/110055/.

perhaps rewrite this as:

for latch := range sm.rSet {
    latch.setInRSet(false)
    sm.trees[spanset.SpanReadOnly].Set(latch)
}
if realloc := len(sm.rSet) > 16; realloc {
    sm.rSet = make(map[*latch]struct{})
} else {
    for latch := range sm.rSet {
        delete(sm.rSet, latch)
    }
}

[nvanbenschoten]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/storage/command_queue_test.go, line 809 at r4 (raw file):

Previously, petermattis (Peter Mattis) wrote…

I suppose you'll be renaming all of these instances of CommandQueue as well in a future PR.

Yes, I'll be ripping out every single reference I can find to it.

---

pkg/storage/spanlatch/doc.go, line 20 at r4 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Perhaps mention that this is the evolution of complexity. Something like: s/Managers's/The evolution of/g.

Done.

---

pkg/storage/spanlatch/manager.go, line 62 at r10 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Nit: I'd prefer to see this spelled out as readSet and inReadSet.

We could avoid the use of a map by instead using a circularly linked list. latch would need next, prev *latch fields. You can remove an element from such a list without knowing its position. See util/cache.Entry and util/cache.entryList for an example of what I'm thinking of.

That's a really cool idea! It provides a nice speedup:

name                                          old time/op    new time/op    delta
LatchManagerReadOnlyMix/size=1-4                 683ns ± 9%     404ns ±10%  -40.85%  (p=0.008 n=5+5)
LatchManagerReadOnlyMix/size=4-4                 660ns ± 7%     382ns ± 5%  -42.17%  (p=0.008 n=5+5)
LatchManagerReadOnlyMix/size=16-4                684ns ±10%     367ns ± 5%  -46.27%  (p=0.008 n=5+5)
LatchManagerReadOnlyMix/size=64-4                683ns ± 8%     370ns ± 1%  -45.75%  (p=0.016 n=5+4)
LatchManagerReadOnlyMix/size=128-4               678ns ± 4%     398ns ±14%  -41.27%  (p=0.008 n=5+5)
LatchManagerReadOnlyMix/size=256-4               652ns ± 4%     385ns ± 4%  -40.95%  (p=0.008 n=5+5)
LatchManagerReadWriteMix/readsPerWrite=0-4       594ns ±16%     629ns ±17%     ~     (p=0.222 n=5+5)
LatchManagerReadWriteMix/readsPerWrite=1-4       603ns ± 1%     552ns ± 7%   -8.39%  (p=0.008 n=5+5)
LatchManagerReadWriteMix/readsPerWrite=4-4       621ns ± 4%     576ns ± 5%   -7.28%  (p=0.008 n=5+5)
LatchManagerReadWriteMix/readsPerWrite=16-4      649ns ± 2%     541ns ±13%  -16.69%  (p=0.008 n=5+5)
LatchManagerReadWriteMix/readsPerWrite=64-4      474ns ± 5%     423ns ±29%     ~     (p=0.151 n=5+5)
LatchManagerReadWriteMix/readsPerWrite=128-4     413ns ± 2%     362ns ±16%     ~     (p=0.095 n=5+5)
LatchManagerReadWriteMix/readsPerWrite=256-4     448ns ±14%     314ns ±13%  -29.85%  (p=0.008 n=5+5)

name                                          old alloc/op   new alloc/op   delta
LatchManagerReadOnlyMix/size=1-4                  191B ± 0%      160B ± 0%  -16.23%  (p=0.008 n=5+5)
LatchManagerReadOnlyMix/size=4-4                  191B ± 0%      160B ± 0%  -16.23%  (p=0.008 n=5+5)
LatchManagerReadOnlyMix/size=16-4                 191B ± 0%      160B ± 0%  -16.23%  (p=0.008 n=5+5)
LatchManagerReadOnlyMix/size=64-4                 191B ± 0%      160B ± 0%     ~     (p=0.079 n=4+5)
LatchManagerReadOnlyMix/size=128-4                191B ± 0%      160B ± 0%  -16.23%  (p=0.008 n=5+5)
LatchManagerReadOnlyMix/size=256-4                191B ± 0%      160B ± 0%  -16.23%  (p=0.008 n=5+5)
LatchManagerReadWriteMix/readsPerWrite=0-4        144B ± 0%      160B ± 0%  +11.11%  (p=0.008 n=5+5)
LatchManagerReadWriteMix/readsPerWrite=1-4        144B ± 0%      160B ± 0%  +11.11%  (p=0.008 n=5+5)
LatchManagerReadWriteMix/readsPerWrite=4-4        144B ± 0%      160B ± 0%  +11.11%  (p=0.008 n=5+5)
LatchManagerReadWriteMix/readsPerWrite=16-4       144B ± 0%      160B ± 0%  +11.11%  (p=0.008 n=5+5)
LatchManagerReadWriteMix/readsPerWrite=64-4       144B ± 0%      160B ± 0%  +11.11%  (p=0.008 n=5+5)
LatchManagerReadWriteMix/readsPerWrite=128-4      144B ± 0%      160B ± 0%  +11.11%  (p=0.008 n=5+5)
LatchManagerReadWriteMix/readsPerWrite=256-4      144B ± 0%      160B ± 0%  +11.11%  (p=0.008 n=5+5)

name                                          old allocs/op  new allocs/op  delta
LatchManagerReadOnlyMix/size=1-4                  1.00 ± 0%      1.00 ± 0%     ~     (all equal)
LatchManagerReadOnlyMix/size=4-4                  1.00 ± 0%      1.00 ± 0%     ~     (all equal)
LatchManagerReadOnlyMix/size=16-4                 1.00 ± 0%      1.00 ± 0%     ~     (all equal)
LatchManagerReadOnlyMix/size=64-4                 1.00 ± 0%      1.00 ± 0%     ~     (all equal)
LatchManagerReadOnlyMix/size=128-4                1.00 ± 0%      1.00 ± 0%     ~     (all equal)
LatchManagerReadOnlyMix/size=256-4                1.00 ± 0%      1.00 ± 0%     ~     (all equal)
LatchManagerReadWriteMix/readsPerWrite=0-4        1.00 ± 0%      1.00 ± 0%     ~     (all equal)
LatchManagerReadWriteMix/readsPerWrite=1-4        1.00 ± 0%      1.00 ± 0%     ~     (all equal)
LatchManagerReadWriteMix/readsPerWrite=4-4        1.00 ± 0%      1.00 ± 0%     ~     (all equal)
LatchManagerReadWriteMix/readsPerWrite=16-4       1.00 ± 0%      1.00 ± 0%     ~     (all equal)
LatchManagerReadWriteMix/readsPerWrite=64-4       1.00 ± 0%      1.00 ± 0%     ~     (all equal)
LatchManagerReadWriteMix/readsPerWrite=128-4      1.00 ± 0%      1.00 ± 0%     ~     (all equal)
LatchManagerReadWriteMix/readsPerWrite=256-4      1.00 ± 0%      1.00 ± 0%     ~     (all equal)

---

pkg/storage/spanlatch/manager.go, line 228 at r10 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Looks like you always have a snapshot associated with a guard. Rather than passing the snapshot on the stack, it might be better (faster) to embed the snapshot in the guard and to change Manager.snapshot() to take a *snapshot which it fills in.

But then we have to allocate that entire object on the heap and keep the memory around for the entire lifetime of the Guard. Do you think that will be faster?

---

pkg/storage/spanlatch/manager.go, line 250 at r10 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Perhaps follow the Locked naming convention. E.g. snapshotLocked and insertLocked.

Done.

---

pkg/storage/spanlatch/interval_btree.go, line 15 at r2 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Note that there are various bits in the UI that refer to "Command Queue". Let's file an issue to change the name there as well.

I have a series of changes lined up to eradicate that word.

[ajwerner]

Reviewed 1 of 7 files at r4, 1 of 1 files at r5, 2 of 4 files at r7, 3 of 7 files at r9, 1 of 1 files at r10, 1 of 7 files at r14, 1 of 5 files at r16.
Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/storage/spanlatch/list.go, line 20 at r16 (raw file):

type latchList struct {
	root latch
	len  int

while it's reasonable and clean to track len (and it's done in container/list to be able to implement O(1) Length), it seems like given the general memory consciousness of this package, it's safe to omit latchList.len if in front() you make the nil condition ll.root.next == nil || ll.root.next == &ll.root

---

pkg/storage/spanlatch/manager.go, line 246 at r16 (raw file):

// flushReadSetLocked flushes the read set into the read interval tree.
func (sm *scopedManager) flushReadSetLocked() {
	for sm.readSet.len > 0 {

if you decide to eliminate.len then I guess this could look like:

for latch := sm.readSet.front(); latch != nil; latch = sm.readSet.front() {
   sm.readSet.remove(latch)
   sm.trees[spanset.SpanReadOnly].Set(latch)
}

[petermattis]

I didn't fully scrutinize all of the details or testing here. Let me know if you think something deserve particular attention and I'll give it a thorough look.

Reviewable status: complete! 1 of 0 LGTMs obtained

---

pkg/storage/spanlatch/list.go, line 20 at r16 (raw file):

Previously, ajwerner wrote…

while it's reasonable and clean to track len (and it's done in container/list to be able to implement O(1) Length), it seems like given the general memory consciousness of this package, it's safe to omit latchList.len if in front() you make the nil condition ll.root.next == nil || ll.root.next == &ll.root

The memory savings are minimal as there are a constant number of latchLists per Manager. That said, I'd remove len because it doesn't seem necessary per @ajwerner's suggestion.

---

pkg/storage/spanlatch/list.go, line 30 at r16 (raw file):

}

func (ll *latchList) lazyInit() {

Do you need this lazyInit stuff? For the usage in spanlatch.Manager I think an init method could be called when the Manager is created.

---

pkg/storage/spanlatch/manager.go, line 62 at r10 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

That's a really cool idea! It provides a nice speedup:

name                                          old time/op    new time/op    delta
LatchManagerReadOnlyMix/size=1-4                 683ns ± 9%     404ns ±10%  -40.85%  (p=0.008 n=5+5)
LatchManagerReadOnlyMix/size=4-4                 660ns ± 7%     382ns ± 5%  -42.17%  (p=0.008 n=5+5)
LatchManagerReadOnlyMix/size=16-4                684ns ±10%     367ns ± 5%  -46.27%  (p=0.008 n=5+5)
LatchManagerReadOnlyMix/size=64-4                683ns ± 8%     370ns ± 1%  -45.75%  (p=0.016 n=5+4)
LatchManagerReadOnlyMix/size=128-4               678ns ± 4%     398ns ±14%  -41.27%  (p=0.008 n=5+5)
LatchManagerReadOnlyMix/size=256-4               652ns ± 4%     385ns ± 4%  -40.95%  (p=0.008 n=5+5)
LatchManagerReadWriteMix/readsPerWrite=0-4       594ns ±16%     629ns ±17%     ~     (p=0.222 n=5+5)
LatchManagerReadWriteMix/readsPerWrite=1-4       603ns ± 1%     552ns ± 7%   -8.39%  (p=0.008 n=5+5)
LatchManagerReadWriteMix/readsPerWrite=4-4       621ns ± 4%     576ns ± 5%   -7.28%  (p=0.008 n=5+5)
LatchManagerReadWriteMix/readsPerWrite=16-4      649ns ± 2%     541ns ±13%  -16.69%  (p=0.008 n=5+5)
LatchManagerReadWriteMix/readsPerWrite=64-4      474ns ± 5%     423ns ±29%     ~     (p=0.151 n=5+5)
LatchManagerReadWriteMix/readsPerWrite=128-4     413ns ± 2%     362ns ±16%     ~     (p=0.095 n=5+5)
LatchManagerReadWriteMix/readsPerWrite=256-4     448ns ±14%     314ns ±13%  -29.85%  (p=0.008 n=5+5)

name                                          old alloc/op   new alloc/op   delta
LatchManagerReadOnlyMix/size=1-4                  191B ± 0%      160B ± 0%  -16.23%  (p=0.008 n=5+5)
LatchManagerReadOnlyMix/size=4-4                  191B ± 0%      160B ± 0%  -16.23%  (p=0.008 n=5+5)
LatchManagerReadOnlyMix/size=16-4                 191B ± 0%      160B ± 0%  -16.23%  (p=0.008 n=5+5)
LatchManagerReadOnlyMix/size=64-4                 191B ± 0%      160B ± 0%     ~     (p=0.079 n=4+5)
LatchManagerReadOnlyMix/size=128-4                191B ± 0%      160B ± 0%  -16.23%  (p=0.008 n=5+5)
LatchManagerReadOnlyMix/size=256-4                191B ± 0%      160B ± 0%  -16.23%  (p=0.008 n=5+5)
LatchManagerReadWriteMix/readsPerWrite=0-4        144B ± 0%      160B ± 0%  +11.11%  (p=0.008 n=5+5)
LatchManagerReadWriteMix/readsPerWrite=1-4        144B ± 0%      160B ± 0%  +11.11%  (p=0.008 n=5+5)
LatchManagerReadWriteMix/readsPerWrite=4-4        144B ± 0%      160B ± 0%  +11.11%  (p=0.008 n=5+5)
LatchManagerReadWriteMix/readsPerWrite=16-4       144B ± 0%      160B ± 0%  +11.11%  (p=0.008 n=5+5)
LatchManagerReadWriteMix/readsPerWrite=64-4       144B ± 0%      160B ± 0%  +11.11%  (p=0.008 n=5+5)
LatchManagerReadWriteMix/readsPerWrite=128-4      144B ± 0%      160B ± 0%  +11.11%  (p=0.008 n=5+5)
LatchManagerReadWriteMix/readsPerWrite=256-4      144B ± 0%      160B ± 0%  +11.11%  (p=0.008 n=5+5)

name                                          old allocs/op  new allocs/op  delta
LatchManagerReadOnlyMix/size=1-4                  1.00 ± 0%      1.00 ± 0%     ~     (all equal)
LatchManagerReadOnlyMix/size=4-4                  1.00 ± 0%      1.00 ± 0%     ~     (all equal)
LatchManagerReadOnlyMix/size=16-4                 1.00 ± 0%      1.00 ± 0%     ~     (all equal)
LatchManagerReadOnlyMix/size=64-4                 1.00 ± 0%      1.00 ± 0%     ~     (all equal)
LatchManagerReadOnlyMix/size=128-4                1.00 ± 0%      1.00 ± 0%     ~     (all equal)
LatchManagerReadOnlyMix/size=256-4                1.00 ± 0%      1.00 ± 0%     ~     (all equal)
LatchManagerReadWriteMix/readsPerWrite=0-4        1.00 ± 0%      1.00 ± 0%     ~     (all equal)
LatchManagerReadWriteMix/readsPerWrite=1-4        1.00 ± 0%      1.00 ± 0%     ~     (all equal)
LatchManagerReadWriteMix/readsPerWrite=4-4        1.00 ± 0%      1.00 ± 0%     ~     (all equal)
LatchManagerReadWriteMix/readsPerWrite=16-4       1.00 ± 0%      1.00 ± 0%     ~     (all equal)
LatchManagerReadWriteMix/readsPerWrite=64-4       1.00 ± 0%      1.00 ± 0%     ~     (all equal)
LatchManagerReadWriteMix/readsPerWrite=128-4      1.00 ± 0%      1.00 ± 0%     ~     (all equal)
LatchManagerReadWriteMix/readsPerWrite=256-4      1.00 ± 0%      1.00 ± 0%     ~     (all equal)

💯

---

pkg/storage/spanlatch/manager.go, line 228 at r10 (raw file):

Do you think that will be faster?

I don't know. Perhaps add it to a TODO list to investigate after this PR goes in. Probably a very minor benefit if any.

[ajwerner]

Reviewed 4 of 9 files at r11, 3 of 4 files at r12, 1 of 2 files at r13, 5 of 7 files at r14, 1 of 1 files at r15, 2 of 5 files at r16.
Reviewable status: complete! 2 of 0 LGTMs obtained

---

pkg/storage/spanlatch/manager.go, line 350 at r14 (raw file):

// before returning.
func (m *Manager) wait(ctx context.Context, lg *Guard, ts hlc.Timestamp, snap snapshot) error {
	for s := spanset.SpanScope(0); s < spanset.NumSpanScope; s++ {

Just a question for discussion, can the order in which latches are examined impact performance? It seems like if we could wait on the longest blocking item first then we'd increase the rate of hitting the fast path on the signal and the number of goroutine yields on the select. I don't have good intuition about what it would take to come up with a heuristic to guess as when a latch will be removed. Do we expect reads to happen faster than writes? Do we expect global things to take longer than local? All of this may be premature optimization. It might be worth trying to see how often you hit the fast path and if the number is low (maybe even as low as something like 50%), then maybe there's a cheap win here.

---

pkg/storage/spanlatch/manager.go, line 265 at r16 (raw file):

				switch a {
				case spanset.SpanReadOnly:
					// Add reads to the rSet. They only need to enter the read

total nit: s/rSet/readSet/

[nvanbenschoten]

TFTRs!

bors r+

Reviewable status: complete! 2 of 0 LGTMs obtained

---

pkg/storage/spanlatch/list.go, line 20 at r16 (raw file):

Previously, petermattis (Peter Mattis) wrote…

The memory savings are minimal as there are a constant number of latchLists per Manager. That said, I'd remove len because it doesn't seem necessary per @ajwerner's suggestion.

I actually did exactly what's being suggested here at first, but I realized that we're going to want metrics on this soon enough and being able to track how many reads are in the readSet will be important.

---

pkg/storage/spanlatch/list.go, line 30 at r16 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Do you need this lazyInit stuff? For the usage in spanlatch.Manager I think an init method could be called when the Manager is created.

This allows the zero value for the entire spanlatch.Manager to be used directly, which is super nice. We don't have or need a Manager constructor.

---

pkg/storage/spanlatch/manager.go, line 228 at r10 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Do you think that will be faster?

I don't know. Perhaps add it to a TODO list to investigate after this PR goes in. Probably a very minor benefit if any.

I gave it a shot and it didn't seem to help:

name                              old time/op    new time/op    delta
ReadOnlyMix/size=1-4                 404ns ±10%     561ns ±14%  +38.91%  (p=0.008 n=5+5)
ReadOnlyMix/size=4-4                 382ns ± 5%     533ns ±17%  +39.60%  (p=0.008 n=5+5)
ReadOnlyMix/size=16-4                367ns ± 5%     500ns ±17%  +36.04%  (p=0.008 n=5+5)
ReadOnlyMix/size=64-4                370ns ± 1%     518ns ± 8%  +39.92%  (p=0.016 n=4+5)
ReadOnlyMix/size=128-4               398ns ±14%     548ns ± 8%  +37.50%  (p=0.008 n=5+5)
ReadOnlyMix/size=256-4               385ns ± 4%     546ns ± 5%  +41.92%  (p=0.008 n=5+5)
ReadWriteMix/readsPerWrite=0-4       629ns ±17%     755ns ±14%     ~     (p=0.056 n=5+5)
ReadWriteMix/readsPerWrite=1-4       552ns ± 7%     729ns ± 9%  +31.93%  (p=0.008 n=5+5)
ReadWriteMix/readsPerWrite=4-4       576ns ± 5%     673ns ±20%  +16.84%  (p=0.008 n=5+5)
ReadWriteMix/readsPerWrite=16-4      541ns ±13%     632ns ± 1%  +16.89%  (p=0.008 n=5+5)
ReadWriteMix/readsPerWrite=64-4      423ns ±29%     552ns ±31%  +30.50%  (p=0.032 n=5+5)
ReadWriteMix/readsPerWrite=128-4     362ns ±16%     426ns ± 3%  +17.44%  (p=0.016 n=5+5)
ReadWriteMix/readsPerWrite=256-4     314ns ±13%     405ns ± 6%  +28.94%  (p=0.008 n=5+5)

name                              old alloc/op   new alloc/op   delta
ReadOnlyMix/size=1-4                  160B ± 0%      224B ± 0%  +40.00%  (p=0.008 n=5+5)
ReadOnlyMix/size=4-4                  160B ± 0%      224B ± 0%  +40.00%  (p=0.008 n=5+5)
ReadOnlyMix/size=16-4                 160B ± 0%      224B ± 0%  +40.00%  (p=0.008 n=5+5)
ReadOnlyMix/size=64-4                 160B ± 0%      224B ± 0%  +40.00%  (p=0.008 n=5+5)
ReadOnlyMix/size=128-4                160B ± 0%      224B ± 0%  +40.00%  (p=0.008 n=5+5)
ReadOnlyMix/size=256-4                160B ± 0%      224B ± 0%  +40.00%  (p=0.008 n=5+5)
ReadWriteMix/readsPerWrite=0-4        160B ± 0%      224B ± 0%  +40.00%  (p=0.008 n=5+5)
ReadWriteMix/readsPerWrite=1-4        160B ± 0%      224B ± 0%  +40.00%  (p=0.008 n=5+5)
ReadWriteMix/readsPerWrite=4-4        160B ± 0%      224B ± 0%  +40.00%  (p=0.008 n=5+5)
ReadWriteMix/readsPerWrite=16-4       160B ± 0%      224B ± 0%  +40.00%  (p=0.008 n=5+5)
ReadWriteMix/readsPerWrite=64-4       160B ± 0%      224B ± 0%  +40.00%  (p=0.008 n=5+5)
ReadWriteMix/readsPerWrite=128-4      160B ± 0%      224B ± 0%  +40.00%  (p=0.008 n=5+5)
ReadWriteMix/readsPerWrite=256-4      160B ± 0%      224B ± 0%  +40.00%  (p=0.008 n=5+5)

---

pkg/storage/spanlatch/manager.go, line 350 at r14 (raw file):
This is an interesting idea. We expect writes to hold their latches significantly longer that reads, so it should be a clear win to wait on them first so that we select from fewer channels in total. Done.

Do we expect reads to happen faster than writes?

Yes.

Do we expect global things to take longer than local?

Not necessarily. I don't think there's any real correlation here.

---

pkg/storage/spanlatch/manager.go, line 246 at r16 (raw file):

Previously, ajwerner wrote…

if you decide to eliminate.len then I guess this could look like:

for latch := sm.readSet.front(); latch != nil; latch = sm.readSet.front() {
   sm.readSet.remove(latch)
   sm.trees[spanset.SpanReadOnly].Set(latch)
}

See discussion above.

---

pkg/storage/spanlatch/manager.go, line 265 at r16 (raw file):

Previously, ajwerner wrote…

total nit: s/rSet/readSet/

Not a nit, a botched refactor :) Done.

[nvanbenschoten]

bors r-

[craig]

Canceled

[ajwerner]

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

---

pkg/storage/spanlatch/manager.go, line 350 at r14 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

This is an interesting idea. We expect writes to hold their latches significantly longer that reads, so it should be a clear win to wait on them first so that we select from fewer channels in total. Done.

Do we expect reads to happen faster than writes?

Yes.

Do we expect global things to take longer than local?

Not necessarily. I don't think there's any real correlation here.

Cool, the next steps to push this idea further would be:

1. set a to SpanReadWrite before setting it SpanReadOnly (0) in the for loop
2. sort the latches in newGuard with the highest timestamps first as my intuition is that the high timestamp latches are expected to finish last.

[nvanbenschoten]

bors r+

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

---

pkg/storage/spanlatch/manager.go, line 350 at r14 (raw file):

set a to SpanReadWrite before setting it SpanReadOnly (0) in the for loop

But we would want a=SpanReadOnly before a=SpanReadWrite, right? Because then the order of access will be readSpan+tree[SpanReadWrite], writeSpan+tree[SpanReadWrite], writeSpan+tree[SpanReadOnly]. Either way, in practice we never actually see requests with read and write spans together.

sort the latches in newGuard with the highest timestamps first as my intuition is that the high timestamp latches are expected to finish last.

All of the latches in newGuard have the same timestamp. Also, anything that requires sorting will almost certainly cost more than doing nothing at all. We're dealing on the order of double-digit ns at this point.

[craig]

Build succeeded

• GitHub CI (Cockroach)

[tbg]

💯

Reviewed 9 of 9 files at r11, 4 of 4 files at r12, 2 of 2 files at r13, 7 of 7 files at r14, 1 of 1 files at r15, 4 of 5 files at r16, 1 of 1 files at r17.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale)

---

pkg/storage/spanlatch/manager.go, line 314 at r14 (raw file):

}

func (m *Manager) nextID() uint64 {

nit: nextIDLocked()

---

pkg/storage/spanlatch/manager_test.go, line 122 at r14 (raw file):

	m := New()

	// Try latch with no overlapping already-acquired lathes.

lathes

[tbg]

Reviewed 1 of 1 files at r18.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale)

[nvanbenschoten]

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

---

pkg/storage/spanlatch/manager.go, line 314 at r14 (raw file):

Previously, tbg (Tobias Grieger) wrote…

nit: nextIDLocked()

Will address in next PR.