concurrency: refactors in preparation for lock promotion

[arulajmani]

See individual commits for details.

References #110435

[blathers-crl]

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[cockroach-teamcity]

This change is 

[nvanbenschoten]

Reviewed 1 of 1 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani)

---

pkg/kv/kvserver/concurrency/lock_table.go line 2725 at r1 (raw file):

// insertLockingRequest inserts the locking request, trying to access the lock
// with the supplied strength, at the correct position into the lock's wait

s/into/in/

---

pkg/kv/kvserver/concurrency/lock_table.go line 2794 at r2 (raw file):

// arrival time -- requests that arrive later sort after requests that arrive
// earlier, and vice-versa.
func (kl *keyLocks) sortsAfter(g1, g2 *lockTableGuardImpl) bool {

A sortsBefore and sortsAfter method which don't reference each other and which don't use some shared logic feels like a recipe for bugs. Do we need both?

---

pkg/kv/kvserver/concurrency/lock_table.go line 2794 at r2 (raw file):

// arrival time -- requests that arrive later sort after requests that arrive
// earlier, and vice-versa.
func (kl *keyLocks) sortsAfter(g1, g2 *lockTableGuardImpl) bool {

Does this need to be a method on keyLocks? Are we going to scan through the held locks to detect promotions on each call?

An alternative approach here would be something like the following:

type queueOrder struct {
    reqSeqNum uint64
    lockPromotion bool
}

func (o1 queueOrder) before(o2 queueOrder) bool { ... }

We'd then compute and store an immutable queueOrder on the queuedGuard.

[arulajmani]

RFAL!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten)

---

pkg/kv/kvserver/concurrency/lock_table.go line 2794 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

A sortsBefore and sortsAfter method which don't reference each other and which don't use some shared logic feels like a recipe for bugs. Do we need both?

Yeah, I call one from the other in #118484.

I ended up removing the before -- the only place that was previously using it was verify, which I rewrote to (what I think to) be a bit clearer.

---

pkg/kv/kvserver/concurrency/lock_table.go line 2794 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Does this need to be a method on keyLocks? Are we going to scan through the held locks to detect promotions on each call?

An alternative approach here would be something like the following:

type queueOrder struct {
    reqSeqNum uint64
    lockPromotion bool
}

func (o1 queueOrder) before(o2 queueOrder) bool { ... }

We'd then compute and store an immutable queueOrder on the queuedGuard.

Discussed offline. SGTM. Done!

[nvanbenschoten]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani)

---

pkg/kv/kvserver/concurrency/lock_table.go line 2794 at r2 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Discussed offline. SGTM. Done!

Are these still needed now that we have queueOrder?

[arulajmani]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten)

---

pkg/kv/kvserver/concurrency/lock_table.go line 2794 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Are these still needed now that we have queueOrder?

Ugh accidentally pressed u one too many times and didn't realize :)

My bad; removed.

[nvanbenschoten]

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @arulajmani)

[arulajmani]

Thanks!

bors r=nvanbenschoten

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)