storage: use RWMutex in NodeLiveness

[ajwerner]

When at Mutex profiles for heavily loaded TPC-C clusters we noticed that a lot
of time was being spent blocked on a RWMutex held by Replica.leaseGoodToGo which
underneath was reading NodeLiveness state in a read-only way. This PR adds a
RWMutex to NodeLiveness to eliminate contention. Prior to this change we
observed nearly 60% of lock contention on leaseGoodToGo. After we observe
closer to 20%.

Release note: None

[cockroach-teamcity]

This change is 

[ajwerner]

Before:

After:

[nvanbenschoten]

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

---

pkg/storage/node_liveness.go, line 598 at r1 (raw file):

// a former liveness update on restart.
func (nl *NodeLiveness) Self() (*storagepb.Liveness, error) {
	nl.mu.Lock()

Read lock here as well?

---

pkg/storage/node_liveness.go, line 619 at r1 (raw file):

	nl.mu.RLock()
	defer nl.mu.RUnlock()
	lMap := IsLiveMap{}

Let's pull this allocation out of the lock.

---

pkg/storage/node_liveness.go, line 845 at r1 (raw file):

	}

	nl.mu.Lock()

Read lock?

---

pkg/storage/node_liveness.go, line 857 at r1 (raw file):

// registered callbacks if the node became live in the process.
func (nl *NodeLiveness) maybeUpdate(new storagepb.Liveness) {
	nl.mu.Lock()

While we're here, I wonder if we should improve this. There are a few easy wins. For starters, if shouldReplaceLiveness is false some non-negligible percent of time (I'm not sure about this) then it's probably worth initially grabbing a read-lock and only upgrading to a write-lock if necessary. Doing this would also give us the ability to optimistically allocate the callbacks slice outside of the lock.

---

pkg/storage/node_liveness.go, line 935 at r1 (raw file):

	maxOffset := nl.clock.MaxOffset()

	nl.mu.Lock()

Read lock?

[ajwerner]

Updated, PTAL.

Reviewable status: complete! 0 of 0 LGTMs obtained

[nvanbenschoten]

mod the question about whether the optimistic read-locking in maybeUpdate could be a pessimization.

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

---

pkg/storage/node_liveness.go, line 866 at r2 (raw file):

	// Note that this works fine even if `old` is empty.
	should := shouldReplaceLiveness(old, new)
	if !should {

Nice! Any idea how frequently we hit this path? Running a roachtest manually with a bit of instrumentation would be enough to get some feel for this.

---

pkg/storage/node_liveness.go, line 872 at r2 (raw file):

	callbacks := make([]IsLiveCallback, 0, numCallbacks)
	nl.mu.Lock()
	old = nl.mu.nodes[new.NodeID]

Drop a comment above this that we check shouldReplaceLiveness again now that we're under and exclusive lock.

[ajwerner]

Nice! Any idea how frequently we hit this path? Running a roachtest manually with a bit of instrumentation would be enough to get some feel for this.

It does indeed seem to be a pessimization. We seem to hit the fast path roughly 1/3rd of the time on a 3 node cluster and 1/9th of the time on a 9 node cluster (pattern?)

I'll rip it out

[nvanbenschoten]

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

[ajwerner]

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)