store/tikv,executor: redesign the latch scheduler #7711
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Check maxCommitTS on each key, instead of each slot, so hash collision will not lead to transaction retry.
disksing
reviewed
Sep 17, 2018
store/tikv/latch/latch.go
Outdated
| find = n | ||
| break | ||
| } | ||
| // TODO: Invalidate old data. |
There was a problem hiding this comment.
We cannot merge this PR before fixing this TODO. Because without it we will get an OOM.
disksing
reviewed
Sep 27, 2018
|
|
||
| func (l *latch) isEmpty() bool { | ||
| return l.waitingQueueHead == 0 && !l.hasMoreWaiting | ||
| next *node |
There was a problem hiding this comment.
list.Listwill make unnecessary allocation.
Use
type Element struct {
// The value stored with this element.
Value interface{}
// contains filtered or unexported fields
}
is similar to
type node struct {
Value *nodeValue
}
type nodeValue {
slotID int
key []byte
maxCommitTS uint64
value *Lock
}
list.Listis a doubly linked list, while a single linked list is sufficient here.- list data struct is simple and common enough to implement
store/tikv/latch/latch.go
Outdated
|
|
||
| // Try to limits the memory usage. | ||
| if latch.count > latchListCount { | ||
| latch.recycle(lock.startTS) |
There was a problem hiding this comment.
Should recyclebe moved into if find == nil {}?
store/tikv/latch/latch.go
Outdated
| for i := 0; i < len(latch.waiting); i++ { | ||
| waiting := latch.waiting[i] | ||
| if bytes.Compare(waiting.keys[waiting.acquiredCount], key) == 0 { | ||
| nextLock = waiting |
There was a problem hiding this comment.
Is it possible that there are more than 1 Locks in the waiting list have the same key?
There was a problem hiding this comment.
Possible! you find a bug.
I should only wake up the first one.
Wake up the first one is in a FIFO manner, there are still room for improvement here to choose which one to wake up.
|
Do we have any test/benchmark result? |
|
PTAL @zhangjinpeng1987 |
|
I use a key range of When latch scheduler is disabled: When latch scheduler is enabled: There will be near 100x performance improment. |
disksing
reviewed
Sep 27, 2018
store/tikv/latch/scheduler.go
Outdated
| @@ -40,13 +44,30 @@ func NewScheduler(size uint) *LatchesScheduler { | |||
| return scheduler | |||
| } | |||
|
|
|||
| const checkInterval = 10 * time.Minute | |||
| const expireDuration = 2 * time.Hour | |||
There was a problem hiding this comment.
Too long. Be aware that now a transaction can last for at most 10 minutes. So we should be safe to clean up commit history before 10 minutes.
store/tikv/latch/latch.go
Outdated
|
|
||
| // Handle the head node. | ||
| if tsoSub(currentTS, l.queue.maxCommitTS) >= expireDuration && l.queue.value == nil { | ||
| l.queue = nil |
store/tikv/latch/latch.go
Outdated
| if tsoSub(currentTS, l.queue.maxCommitTS) >= expireDuration && l.queue.value == nil { | ||
| l.queue = nil | ||
| } | ||
| return |
store/tikv/latch/latch.go
Outdated
| } | ||
|
|
||
| prev := l.queue | ||
| curr := l.queue.next |
There was a problem hiding this comment.
Better initialize as prev = nil, curr = l.queue, so don't have to handle head node separately. Or you can consider using the indirect pointer hack suggested by Linus Torvalds https://medium.com/@bartobri/applying-the-linus-tarvolds-good-taste-coding-requirement-99749f37684a
There was a problem hiding this comment.
Actually, I found that trick myself before I saw any article about it. (I remember I wrote a blog post http://www.zenlife.tk/fake-list-head.md around that time)
|
LGTM. |
| type latch struct { | ||
| queue *node | ||
| count int | ||
| waiting []*Lock |
There was a problem hiding this comment.
Why not each node has a waiting queue?
There was a problem hiding this comment.
Waiting queue is moved from each node to the latch for those reasons:
- nodes in the queue is inserted every now and then, if each node has is waiting queue, the queue would be created and destroyed. There will be more allocations, and it's less memory efficient.
- I have an assumption that the waiting queue would not be large, when a list is small enough, an array is very efficient.
- You may still remember the "first waiting one automatically become running" problem and the old code is complex enough to handle different states. If each node doesn't have the waiting queue, the problem could be avoid.
@zhangjinpeng1987
| if idx < len(latch.waiting) { | ||
| nextLock = latch.waiting[idx] | ||
| // Delete element latch.waiting[idx] from the array. | ||
| copy(latch.waiting[idx:], latch.waiting[idx+1:]) |
There was a problem hiding this comment.
It's better to use a list for waiting locks.
There was a problem hiding this comment.
@coocood What's your advise?
|
@tiancaiamao Have you tested sysbench OLTP_RW-pareto scenario? |
|
/run-all-tests |
tiancaiamao
added a commit
to tiancaiamao/tidb
that referenced
this pull request
Oct 10, 2018
Check maxCommitTS on each key, instead of each slot, so hash collision will not lead to transaction retry.
zz-jason
pushed a commit
that referenced
this pull request
Oct 12, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
The old latch scheduler checks
maxCommitTSon each slot, different key could hash to the same slot,then the latch scheduler will report a fake transaction confliction.
What is changed and how it works?
Each key will have their own
maxCommitTS, so hash collision will not result in a fake transaction confliction.Check List
Tests
Code changes
RefreshCommitTSPTAL @coocood