Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

storage: investigate busy loop in split queue #18646

Closed
tbg opened this issue Sep 20, 2017 · 1 comment
Closed

storage: investigate busy loop in split queue #18646

tbg opened this issue Sep 20, 2017 · 1 comment
Assignees
@tbg
Labels
A-kv-distribution Relating to rebalancing and leasing. C-investigation Further steps needed to qualify. C-label will change.
Milestone
2.1

Comments

@tbg
Copy link
Member

tbg commented Sep 20, 2017

Both #18199 and #15997 observed the split queue showing up in profiles/participating in obvious contention.

In #15997 we were writing a range chock full of illegal split keys. The split operation should back off more and more (owing to the doubling of the in-memory split target size), but this perhaps doesn't happen. Also, reading an engine snapshot in a tight loop appears to have interfered pretty severely with Raft handling in my testing, leading to up to 50s instances of handleRaftReady.
@tbg tbg added this to the 1.2 milestone Sep 20, 2017
@tbg tbg self-assigned this Sep 20, 2017
@petermattis
Copy link
Collaborator

We don't double the split target size if an error is returned from EnsureSafeSplitKey. Perhaps we should.

Removing the engine snapshot from MVCCFindSplitKey is trivial. There is also some other low-hanging performance changes there which I'll send out a PR for. But it will be worthwhile to figure out what is causing the long handleRaftReady. If it is the engine snapshot, then I clearly don't understand something.

petermattis added a commit to petermattis/cockroach that referenced this issue Sep 20, 2017
@petermattis
storage/engine: optimize MVCCFindSplitKey
4da09e7 
Use an explicit iterator instead of Reader.Iterate() in order to reduce
allocations. Specifically, we can avoid 2 allocations per key/value.

Do not create a snapshot when calling MVCCFindSplitKey as internally it
uses a single iterator which has an implicit snapshot.

Remove the unused rangeID parameter to MVCCFindSplitKey. This parameter
was only being used to lookup the range stats to log them under V(2).

name                                     old time/op    new time/op    delta
MVCCFindSplitKey_RocksDB/valueSize=32-8     341ms ±23%     269ms ± 8%   -21.29%  (p=0.000 n=10+10)

name                                     old speed      new speed      delta
MVCCFindSplitKey_RocksDB/valueSize=32-8   198MB/s ±19%   250MB/s ± 8%   +26.34%  (p=0.000 n=10+10)

name                                     old alloc/op   new alloc/op   delta
MVCCFindSplitKey_RocksDB/valueSize=32-8    36.6MB ± 0%     0.0MB ± 0%  -100.00%  (p=0.000 n=9+10)

name                                     old allocs/op  new allocs/op  delta
MVCCFindSplitKey_RocksDB/valueSize=32-8     1.16M ± 0%     0.00M ± 0%  -100.00%  (p=0.000 n=9+10)

See cockroachdb#18646
@petermattis petermattis mentioned this issue Sep 20, 2017
Merged
petermattis added a commit to petermattis/cockroach that referenced this issue Sep 21, 2017
@petermattis
storage/engine: optimize MVCCFindSplitKey
ac78dd5 
Use an explicit iterator instead of Reader.Iterate() in order to reduce
allocations. Specifically, we can avoid 2 allocations per key/value.

Do not create a snapshot when calling MVCCFindSplitKey as internally it
uses a single iterator which has an implicit snapshot.

Remove the unused rangeID parameter to MVCCFindSplitKey. This parameter
was only being used to lookup the range stats to log them under V(2).

name                                     old time/op    new time/op    delta
MVCCFindSplitKey_RocksDB/valueSize=32-8     341ms ±23%     269ms ± 8%   -21.29%  (p=0.000 n=10+10)

name                                     old speed      new speed      delta
MVCCFindSplitKey_RocksDB/valueSize=32-8   198MB/s ±19%   250MB/s ± 8%   +26.34%  (p=0.000 n=10+10)

name                                     old alloc/op   new alloc/op   delta
MVCCFindSplitKey_RocksDB/valueSize=32-8    36.6MB ± 0%     0.0MB ± 0%  -100.00%  (p=0.000 n=9+10)

name                                     old allocs/op  new allocs/op  delta
MVCCFindSplitKey_RocksDB/valueSize=32-8     1.16M ± 0%     0.00M ± 0%  -100.00%  (p=0.000 n=9+10)

See cockroachdb#18646
@petermattis petermattis modified the milestones: 2.0, 2.1 Apr 5, 2018
@tbg tbg added A-kv-distribution Relating to rebalancing and leasing. A-kv-client Relating to the KV client and the KV interface. and removed A-kv-client Relating to the KV client and the KV interface. labels May 15, 2018
@tbg tbg added the C-investigation Further steps needed to qualify. C-label will change. label Jul 22, 2018
@tbg tbg closed this as completed Oct 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@tbg tbg

Labels
A-kv-distribution Relating to rebalancing and leasing. C-investigation Further steps needed to qualify. C-label will change.
Projects
None yet
Milestone
2.1
Development

No branches or pull requests
2 participants
@tbg @petermattis