storage: ignore non-intent locks in intentInterleavingIter
#109570
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, 5 of 5 files at r2, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten and @sumeerbhola)
pkg/storage/intent_interleaving_iter.go line 716 at r2 (raw file):
// seeks intentIter to the first intent key equal to (if i.dir > 0) or after // seekKey and decodes it into i.intentKey. func (i *intentInterleavingIter) seekIntentIterAndDecodeLockKey(
nit: seekIntentIterAndDecodeLockKey seeks ...
pkg/storage/intent_interleaving_iter.go line 719 at r2 (raw file):
seekKey EngineKey, limitKey roachpb.Key, ) (iterState pebble.IterValidityState, err error) { if i.dir < 0 {
I see we do this elsewhere, so feel free to disregard to keep things uniform, but would this logic be better expressed as:
switch i.dir {
case 1:
iterState, err = i.intentIter.SeekEngineKeyGEWithLimit(seekKey, limitKey)
case -1:
iterState, err = i.intentIter.SeekEngineKeyLTWithLimit(seekKey, limitKey)
default:
return iterState, errors.AssertionFailedf(" ... ")
}pkg/storage/intent_interleaving_iter.go line 731 at r2 (raw file):
// steps intentIter to the next intent key and decodes it into i.intentKey. func (i *intentInterleavingIter) stepIntentIterAndDecodeLockKey(
nit: stepIntentIterAndDecodeLockKey steps ...
pkg/storage/intent_interleaving_iter_test.go line 588 at r2 (raw file):
// Lock. str := lock.Shared if rng.Int31n(2) == 0 {
If you take the suggestion about generating more than 1 shared locks, we might want to pull this outside the loop.
pkg/storage/intent_interleaving_iter_test.go line 598 at r2 (raw file):
lockLTKey := LockTableKey{Key: key, Strength: str, TxnUUID: txnUUID} lkv = append(lkv, lockKeyValue{ key: lockLTKey, val: lockVal, liveKey: hasLock && i == 0})
Given multiple shared locks can be compatible with each other, if we're in the str == lock.Shared case, would it be interesting to generate more than 1 shared locks here?
nvanbenschoten
force-pushed
the
nvanbenschoten/intentInterLocks
branch
from
4323593 to
339bf7a
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/storage/intent_interleaving_iter.go line 716 at r2 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
nit: seekIntentIterAndDecodeLockKey seeks ...
Done.
pkg/storage/intent_interleaving_iter.go line 719 at r2 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
I see we do this elsewhere, so feel free to disregard to keep things uniform, but would this logic be better expressed as:
switch i.dir { case 1: iterState, err = i.intentIter.SeekEngineKeyGEWithLimit(seekKey, limitKey) case -1: iterState, err = i.intentIter.SeekEngineKeyLTWithLimit(seekKey, limitKey) default: return iterState, errors.AssertionFailedf(" ... ") }
Yeah, i.dir < 0 and i.dir > 0 seem to be the preferred way to use this field in the file, so I'll follow the same pattern here.
pkg/storage/intent_interleaving_iter.go line 731 at r2 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
nit: stepIntentIterAndDecodeLockKey steps ...
Done.
pkg/storage/intent_interleaving_iter_test.go line 588 at r2 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
If you take the suggestion about generating more than 1 shared locks, we might want to pull this outside the loop.
Done.
pkg/storage/intent_interleaving_iter_test.go line 598 at r2 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
Given multiple shared locks can be compatible with each other, if we're in the
str == lock.Sharedcase, would it be interesting to generate more than 1 shared locks here?
Done.
There was a problem hiding this comment.
Reviewed 5 of 5 files at r3, all commit messages.
Reviewable status:
pkg/storage/intent_interleaving_iter.go line 586 at r4 (raw file):
limitKey = i.makeUpperLimitKey() } if _, err := i.seekIntentIterAndDecodeLockKey(EngineKey{Key: intentSeekKey}, limitKey); err != nil {
Given that the strength is the first byte in the version, I think we can do better and seek to the versions that begin with the intent strength. Similarly for the SeekLT. That should eliminate the need to iterate past non-intent locks in these seek cases.
For intentInterleavingIter.Prev, since byte 3 is going to sort before the others, stepping back will first encounter the other locks of the preceding roachpb.Key. For intentIntentInterleaving.Next, stepping forward will first encounter the other locks of the same roachpb.Key. So for both these cases (where we call NextEngineKeyWithLimit or PrevEngineKeyWithLimit) we do need iteration. The one concern I have with introducing a loop where we didn't have one before (and I haven't thought through this properly), is the few cases of the form
var limitKey roachpb.Key
if i.iterValid && !i.prefix {
limitKey = i.makeUpperLimitKey()
}
iterState, err := i.intentIter.NextEngineKeyWithLimit(limitKey)
where we are not in prefix iteration AND we don't produce a limitKey. We won't stop until we hit the iterator bounds if there are no intents but lot of other locks. These cases need some examination, since continuing until the iterator bounds is not good.
pkg/storage/intent_interleaving_iter.go line 778 at r4 (raw file):
return false, err } lockTableKey, err := engineKey.ToLockTableKey()
This could be optimized if we often expect to find non-intent locks, since there is no need to do all the work in ToLockTableKey. A TODO is fine.
…marks All intents are now separated. Epic: None Release note: None
Informs cockroachdb#100193. This commit updates the intentInterleavingIter to ignore locks in the lock table keyspace with strengths other than lock.Intent (i.e. shared and exclusive locks). Future versions of the iterator may expose information to users about whether any non-intent locks were observed and, if so, which keys they were found on. For now, no such information is exposed. This change is needed for replicated locks. Non-locking reads will not interact with these locks at all, so we want the intentInterleavingIter to ignore them. Meanwhile, locking reads and writes will use a different abstraction to peek into the lock table and look for conflicts. Release note: None
…Iter This commit configures the intentInterleavingIter to dynamically adjust the number of iterations that it will try across the locks on single user key while searching for intents in the lock table before it does a SeekGE/SeekLT to skip to the locks on the next/previous user key. This places a bound on the amount of work that non-locking iteration will perform for keys with many (shared) locks. As a result, scan speed will no longer be proportional to the total number of locks on the keys that it scans over. Scan speed will still be proportional to the number of keys that have locks, but we expect that keys with locks will also have values, so this per-locked-key cost is acceptable. Don't merge, this needs more testing.
nvanbenschoten
force-pushed
the
nvanbenschoten/intentInterLocks
branch
from
339bf7a to
6cd9334
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/storage/intent_interleaving_iter.go line 586 at r4 (raw file):
Previously, sumeerbhola wrote…
Given that the strength is the first byte in the version, I think we can do better and seek to the versions that begin with the intent strength. Similarly for the SeekLT. That should eliminate the need to iterate past non-intent locks in these seek cases.
For
intentInterleavingIter.Prev, since byte 3 is going to sort before the others, stepping back will first encounter the other locks of the precedingroachpb.Key. ForintentIntentInterleaving.Next, stepping forward will first encounter the other locks of the sameroachpb.Key. So for both these cases (where we callNextEngineKeyWithLimitorPrevEngineKeyWithLimit) we do need iteration. The one concern I have with introducing a loop where we didn't have one before (and I haven't thought through this properly), is the few cases of the formvar limitKey roachpb.Key if i.iterValid && !i.prefix { limitKey = i.makeUpperLimitKey() } iterState, err := i.intentIter.NextEngineKeyWithLimit(limitKey)where we are not in prefix iteration AND we don't produce a
limitKey. We won't stop until we hit the iterator bounds if there are no intents but lot of other locks. These cases need some examination, since continuing until the iterator bounds is not good.
@sumeerbhola I added a third commit here which configures the intentInterleavingIter to dynamically adjust the number of iterations that it will try across the locks on single user key before it does a SeekGE/SeekLT to skip to the locks on the next/previous user key. The commit also short-circuits without any version iteration for prefix iterators.
I believe this is where we landed after our conversation last week, though I wasn't sure if you still thought the limitKey could be integrated into this logic. Before I add a set of tests for the behavior, do you mind verifying that it's what you expect and that we should proceed with the approach?
pkg/storage/intent_interleaving_iter.go line 778 at r4 (raw file):
Previously, sumeerbhola wrote…
This could be optimized if we often expect to find non-intent locks, since there is no need to do all the work in
ToLockTableKey. A TODO is fine.
Done.
There was a problem hiding this comment.
Reviewable status:
pkg/storage/intent_interleaving_iter.go line 228 at r7 (raw file):
// adjustment algorithm that is used by pebbleMVCCScanner when scanning over // mvcc versions for a key. maxIntentItersBeforeSeek int
I don't see where we are updating this value, for the "dynamic adjustment"
pkg/storage/intent_interleaving_iter.go line 760 at r7 (raw file):
// Otherwise, iterate over the locks on each key until we find an intent. itersBeforeSeek := i.maxIntentItersBeforeSeek / 2 if itersBeforeSeek == 0 && i.intentIterState != pebble.IterValid {
we need a valid iterator to do a seek, so makes sense that we are stepping when i.intentIterState != pebble.IterValid but why is this a conjunction with itersBeforeSeek == 0?
pkg/storage/intent_interleaving_iter.go line 823 at r7 (raw file):
} } else { i.intentKeyBuf = append(i.intentKeyBuf[:0], i.intentKey...)
this will only happen in the first iteration, yes?
Informs #100193.
Closes #109644.
This commit updates the intentInterleavingIter to ignore locks in the lock table keyspace with strengths other than lock.Intent (i.e. shared and exclusive locks). Future versions of the iterator may expose information to users about whether any non-intent locks were observed and, if so, which keys they were found on. For now, no such information is exposed.
This change is needed for replicated locks. Non-locking reads will not interact with these locks at all, so we want the intentInterleavingIter to ignore them. Meanwhile, locking reads and writes will use a different abstraction to peek into the lock table and look for conflicts.
Benchmark delta without Locks
Benchmarks with Locks
Release note: None