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.

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

Cleanup RegionIterator to use RocksDB features directly #6010

Merged
merged 10 commits into from Nov 25, 2019

Conversation

@breeswish
Copy link
Member

breeswish commented Nov 22, 2019

What have you changed?

By utilizing some recent RocksDB features, like seek_for_prev and iterator bound, we can simplify our RegionIterator. Also moved some error cases to standalone functions to reduce the hot-path code size.

What is the type of the changes?

  • Improvement (a change which is an improvement to an existing feature)

How is the PR tested?

  • Unit test
  • Integration test
breeswish added 5 commits Nov 21, 2019
Signed-off-by: Breezewish <breezewish@pingcap.com>
Signed-off-by: Breezewish <breezewish@pingcap.com>
Signed-off-by: Breezewish <breezewish@pingcap.com>
Signed-off-by: Breezewish <breezewish@pingcap.com>
@breeswish breeswish changed the title Cleanup cursor Cleanup RegionIterator to use RocksDB features directly Nov 22, 2019
@breeswish

This comment has been minimized.

Copy link
Member Author

breeswish commented Nov 22, 2019

/release make dist_release

@breeswish

This comment has been minimized.

Copy link
Member Author

breeswish commented Nov 22, 2019

/bench +tpch +sysbench

@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Nov 22, 2019

Benchmark Report

Run Sysbench Performance Test on VMs

@@                               Benchmark Diff                               @@
================================================================================
tidb: 89847e86ce88182b32bd4ecd3aad30013162cb21
--- tikv: b209a546bb290c60cd975a7b5092b2dfa6464416
+++ tikv: 6f2f4ac108a13c56d33936026c149f3a6359d41e
pd: 85c7f39a2842ee77a79b014b0cd77fadaa4086b4
================================================================================
oltp_update_non_index:
    * QPS: 4750.24 ± 0.02% (std=0.80) delta: 0.20% (p=0.376)
    * Latency p50: 26.94 ± 0.02% (std=0.00) delta: -0.26%
    * Latency p99: 42.05 ± 5.03% (std=1.26) delta: -4.22%
            
oltp_insert:
    * QPS: 4775.66 ± 0.09% (std=2.92) delta: 0.01% (p=0.559)
    * Latency p50: 26.80 ± 0.09% (std=0.02) delta: -0.02%
    * Latency p99: 45.42 ± 6.18% (std=1.66) delta: -3.78%
            
oltp_read_write:
    * QPS: 15717.56 ± 0.48% (std=50.71) delta: 0.40% (p=0.222)
    * Latency p50: 163.21 ± 0.44% (std=0.50) delta: -0.38%
    * Latency p99: 305.20 ± 2.39% (std=5.15) delta: -0.29%
            
oltp_update_index:
    * QPS: 4252.34 ± 0.32% (std=8.85) delta: 0.19% (p=0.222)
    * Latency p50: 30.09 ± 0.32% (std=0.07) delta: -0.20%
    * Latency p99: 53.61 ± 2.27% (std=0.80) delta: 0.91%
            
oltp_point_select:
    * QPS: 40456.33 ± 0.30% (std=85.59) delta: 0.40% (p=0.112)
    * Latency p50: 3.16 ± 0.40% (std=0.01) delta: -0.39%
    * Latency p99: 9.56 ± 0.00% (std=0.00) delta: -1.75%
            
@breeswish

This comment has been minimized.

Copy link
Member Author

breeswish commented Nov 22, 2019

/bench +tpch

@mahjonp

This comment has been minimized.

Copy link
Contributor

mahjonp commented Nov 22, 2019

Oops, there was a dns resolve error when upload result, the final result was pasted here:

@@                               Benchmark Diff                               @@
================================================================================
tidb: 518692c14cab1d901f92a773488fc288cea2d8ce
--- tikv: 1d4e1046db90eacce4857c563a535fd8514fa36e
+++ tikv: 6f2f4ac108a13c56d33936026c149f3a6359d41e
pd: d8ea9951ab51d0e0301f794118259cb4941033cb
================================================================================
01.sql duration: 17886.58 ms ± 2.98% (std=227.73) delta: -2.01% (p=0.071)
02.sql duration: 6902.03 ms ± 2.83% (std=124.58) delta: -1.09% (p=0.389)
03.sql duration: 16851.16 ms ± 1.08% (std=104.16) delta: -5.09% (p=0.000)
04.sql duration: 7094.92 ms ± 1.84% (std=83.36) delta: -4.10% (p=0.026)
06.sql duration: 9412.94 ms ± 0.33% (std=15.67) delta: -4.74% (p=0.005)
07.sql duration: 13771.95 ms ± 0.87% (std=82.21) delta: -2.18% (p=0.000)
08.sql duration: 12039.05 ms ± 2.85% (std=168.33) delta: -1.96% (p=0.005)
09.sql duration: 33042.18 ms ± 2.29% (std=436.20) delta: 1.42% (p=0.044)
10.sql duration: 9796.48 ms ± 2.07% (std=108.02) delta: -2.51% (p=0.006)
11.sql duration: 3480.00 ms ± 5.99% (std=124.26) delta: -1.09% (p=0.801)
12.sql duration: 11433.96 ms ± 6.91% (std=344.22) delta: -6.81% (p=0.095)
13.sql duration: 9230.13 ms ± 2.58% (std=132.93) delta: -0.96% (p=0.045)
14.sql duration: 10812.88 ms ± 2.45% (std=149.28) delta: -6.16% (p=0.047)
15.sql duration: 19445.34 ms ± 4.37% (std=406.84) delta: -2.84% (p=0.074)
16.sql duration: 3565.05 ms ± 10.61% (std=274.67) delta: -6.04% (p=0.287)
17.sql duration: 34184.97 ms ± 1.42% (std=269.18) delta: -0.82% (p=0.748)
18.sql duration: 54432.80 ms ± 4.74% (std=1402.07) delta: -3.59% (p=0.014)
19.sql duration: 14777.47 ms ± 1.15% (std=100.45) delta: -5.79% (p=0.002)
20.sql duration: 10926.50 ms ± 3.21% (std=215.21) delta: -0.41% (p=0.674)
21.sql duration: 28281.48 ms…
src/storage/kv/cursor.rs Outdated Show resolved Hide resolved
src/storage/kv/cursor.rs Outdated Show resolved Hide resolved
breeswish added 2 commits Nov 25, 2019
Signed-off-by: Breezewish <breezewish@pingcap.com>
@breeswish

This comment has been minimized.

Copy link
Member Author

breeswish commented Nov 25, 2019

/run-all-tests

Copy link
Contributor

youjiali1995 left a comment

LGTM

@breeswish

This comment has been minimized.

Copy link
Member Author

breeswish commented Nov 25, 2019

/test

@sticnarf

This comment has been minimized.

Copy link
Contributor

sticnarf commented Nov 25, 2019

Seems that avoid inlining has nothing to do with RegionIterator simplification and can be done in another pr :)

Copy link
Contributor

Little-Wallace left a comment

LGTM

@Little-Wallace

This comment has been minimized.

Copy link
Contributor

Little-Wallace commented Nov 25, 2019

/test

Signed-off-by: Breezewish <breezewish@pingcap.com>
@breeswish

This comment has been minimized.

Copy link
Member Author

breeswish commented Nov 25, 2019

/run-all-tests

Copy link
Contributor

Little-Wallace left a comment

LGTM

@breeswish breeswish added the S: LGT2 label Nov 25, 2019
@breeswish breeswish merged commit 37d037f into tikv:master Nov 25, 2019
6 checks passed
6 checks passed
DCO All commits are signed off!
Details
idc-jenkins-ci-tikv/integration-common-test Jenkins job succeeded.
Details
idc-jenkins-ci-tikv/integration-compatibility-test Jenkins job succeeded.
Details
idc-jenkins-ci-tikv/integration-copr-test Jenkins job succeeded.
Details
idc-jenkins-ci-tikv/integration-ddl-test Jenkins job succeeded.
Details
idc-jenkins-ci/test Jenkins job succeeded.
Details
@breeswish breeswish deleted the breeswish:cleanup_cursor branch Nov 25, 2019
hawkingrei added a commit to hawkingrei/tikv that referenced this pull request Dec 1, 2019
Signed-off-by: Breezewish <breezewish@pingcap.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.