Locktable improvements#45040
Conversation
sumeerbhola
commented
Feb 12, 2020
- eliminate the doneWaiting slice that is used as a return value for many lockState methods
- make waitElsewhere state be used only for a replicated lock that is held
- refactoring to use informActiveWaiters() when lock becomes free
|
This change is |
sumeerbhola
left a comment
There was a problem hiding this comment.
This has 3 commits. I need to add some tests.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten)
nvb
left a comment
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, 1 of 1 files at r2, 1 of 1 files at r3.
Reviewable status:
pkg/storage/concurrency/lock_table.go, line 306 at r1 (raw file):
func (g *lockTableGuardImpl) doneWaitingAtLock(hasReservation bool, l *lockState) { g.mu.Lock() if !hasReservation {
Could you give this condition a comment?
pkg/storage/concurrency/lock_table.go, line 1086 at r2 (raw file):
g := qg.guard g.mu.Lock() delete(g.mu.locks, l)
nit: move this below the if qg.active { block so that it mirrors the waitingReaders case above.
pkg/storage/concurrency/lock_table.go, line 1320 at r3 (raw file):
l.queuedWriters.Remove(e) if qg.active { g.doneWaitingAtLock(true, l)
nit: we're inconsistent about the order that we call doneWaitingAtLock and then conditionally set l.distinguishedWaiter = nil here and above. Let's be consistent so that it's easier to spot bugs.
3548861 to
a1f1467
Compare
sumeerbhola
left a comment
There was a problem hiding this comment.
TFTR!
I've added a TODO for a maxLocks test: since none of that functionality is tested (let alone the changes to that in this PR), I plan to do it in a followup PR. The waitingState.held change and other changes are covered by existing tests.
Reviewable status:
pkg/storage/concurrency/lock_table.go, line 306 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Could you give this condition a comment?
Done
pkg/storage/concurrency/lock_table.go, line 1086 at r2 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
nit: move this below the
if qg.active {block so that it mirrors thewaitingReaderscase above.
Done
pkg/storage/concurrency/lock_table.go, line 944 at r3 (raw file):
} // TODO: change to always informActiveWaiters since this is a transition from // !held to held.
This TODO is fixed in latest commit
pkg/storage/concurrency/lock_table.go, line 1320 at r3 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
nit: we're inconsistent about the order that we call
doneWaitingAtLockand then conditionally setl.distinguishedWaiter = nilhere and above. Let's be consistent so that it's easier to spot bugs.
Done
17a70f0 to
ed30c07
Compare
- fix a TODO to not return slice of doneWaiting *lockTableGuardImpls from various lockState methods and instead have those methods do the needed work. - change waitElsewhere to only be used when the lock is held in replicated mode. Also, when under memory pressure, narrow the exception to clearing the lock to only replicated locks with no distinguished waiter. The motivation for this change is that when the lockTable is clearing its internal state, the only sequencing information remaining is for locks held in replicated mode. All others, locks held in unreplicated mode, or not held locks with reservations, should permit waiters to race. This race will only recreate the queue for a lock after the lock is acquired, since before then it will appear uncontended. - refactor to use informActiveWaiters() when lock becomes free. - update waitingState.held on each waiter when locks are acquired. Release note: None
ed30c07 to
0d2fb83
Compare
|
bors r+ |
45040: Locktable improvements r=sumeerbhola a=sumeerbhola - eliminate the doneWaiting slice that is used as a return value for many lockState methods - make waitElsewhere state be used only for a replicated lock that is held - refactoring to use informActiveWaiters() when lock becomes free Co-authored-by: sumeerbhola <sumeer@cockroachlabs.com>
Build succeeded |