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

txn: Support bypass specified locks for reads #5798

Merged
merged 16 commits into from Nov 12, 2019

Conversation

MyonKeminta
Copy link
Contributor

@MyonKeminta MyonKeminta commented Nov 4, 2019

Signed-off-by: MyonKeminta MyonKeminta@users.noreply.github.com

What have you changed?

This is a part of supporting large transaction.

When an transaction T1 meets another large transaction T2's lock, after invoking check_txn_status on T2, if T2 is not committed and check_txn_status enlarged T2's min_commit_ts to a value greater than T1.start_ts, then T1 can bypass T2's lock to read earlier value. This PR supports this functionality.

This is a critical change. Please be very careful when reviewing this PR.

What is the type of the changes?

  • New feature (a change which adds functionality)

How is the PR tested?

  • Unit test
  • Integration test

Does this PR affect documentation (docs) or should it be mentioned in the release notes?

No

Does this PR affect tidb-ansible?

No

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
@MyonKeminta
Copy link
Contributor Author

/bench

@sre-bot
Copy link
Contributor

sre-bot commented Nov 4, 2019

Benchmark Report

Run Sysbench Performance Test on VMs

@@                               Benchmark Diff                               @@
================================================================================
tidb: 9591a488029619de0ac9c724860e7cf33712093e
--- tikv: 086056fabd7f4763d111856854ed18f6ec9bd9da
+++ tikv: bc29f01784ca73aca61497c80f676657215cae34
pd: ba01076ee005b5545d679dd903f2674416176f09
================================================================================
oltp_update_non_index:
    * QPS: 4729.66 ± 0.14% (std=4.23) delta: -0.01% (p=0.950)
    * Latency p50: 27.06 ± 0.15% (std=0.03) delta: 0.01%
    * Latency p99: 42.05 ± 5.03% (std=1.26) delta: 2.32%
            
oltp_insert:
    * QPS: 4701.13 ± 0.38% (std=10.80) delta: 0.26% (p=0.164)
    * Latency p50: 27.22 ± 0.38% (std=0.06) delta: -0.27%
    * Latency p99: 48.34 ± 0.00% (std=0.00) delta: -2.25%
            
oltp_read_write:
    * QPS: 15678.11 ± 0.35% (std=36.77) delta: 0.69% (p=0.014)
    * Latency p50: 163.60 ± 0.34% (std=0.39) delta: -0.69%
    * Latency p99: 312.58 ± 1.20% (std=2.64) delta: -7.93%
            
oltp_update_index:
    * QPS: 4263.71 ± 0.52% (std=14.89) delta: -0.11% (p=0.640)
    * Latency p50: 30.02 ± 0.53% (std=0.11) delta: 0.11%
    * Latency p99: 54.84 ± 3.64% (std=1.22) delta: 1.84%
            
oltp_point_select:
    * QPS: 45425.58 ± 0.59% (std=230.87) delta: -0.09% (p=0.852)
    * Latency p50: 2.81 ± 0.53% (std=0.02) delta: 0.09%
    * Latency p99: 9.06 ± 0.00% (std=0.00) delta: 0.00%
            

@MyonKeminta MyonKeminta changed the title [DNM] txn: Support skip lock for scanner and point getter [DNM] txn: Support bypass specified locks for reads Nov 4, 2019
@MyonKeminta
Copy link
Contributor Author

/run-integration-common-test

@MyonKeminta
Copy link
Contributor Author

/bench

@MyonKeminta
Copy link
Contributor Author

/release

@MyonKeminta
Copy link
Contributor Author

/bench

@MyonKeminta
Copy link
Contributor Author

/bench

@MyonKeminta
Copy link
Contributor Author

/release

@MyonKeminta
Copy link
Contributor Author

I want to run a benchmark to reveal whether there is significant performance regression caused by this PR but it seems the auto benchmark doesn't work...

@MyonKeminta
Copy link
Contributor Author

/bench

@sre-bot
Copy link
Contributor

sre-bot commented Nov 5, 2019

Benchmark Report

Run Sysbench Performance Test on VMs

@@                               Benchmark Diff                               @@
================================================================================
tidb: 579304039685562d390c0de5b783425efbd2bd8d
--- tikv: da9168bf76e56c60576ab3bb1780afff49d936f3
+++ tikv: cca15e949c32d94a83d89c943fe7f401ffa75fcb
pd: 10a52ee4d92505add7551c6651b1982b705af678
================================================================================
oltp_update_non_index:
    * QPS: 4696.68 ± 0.11% (std=3.75) delta: -0.46% (p=0.327)
    * Latency p50: 27.19 ± 0.64% (std=0.10) delta: 0.27%
    * Latency p99: 43.65 ± 1.19% (std=0.37) delta: 4.93%
            
oltp_insert:
    * QPS: 4771.18 ± 0.05% (std=1.78) delta: -0.01% (p=0.544)
    * Latency p50: 26.82 ± 0.05% (std=0.01) delta: 0.01%
    * Latency p99: 49.89 ± 2.27% (std=0.75) delta: 3.58%
            
oltp_read_write:
    * QPS: 15432.96 ± 0.37% (std=35.16) delta: -0.04% (p=0.407)
    * Latency p50: 166.16 ± 0.37% (std=0.37) delta: 0.14%
    * Latency p99: 315.91 ± 2.24% (std=4.70) delta: 0.00%
            
oltp_update_index:
    * QPS: 4235.23 ± 0.58% (std=15.46) delta: 0.11% (p=0.648)
    * Latency p50: 30.23 ± 0.58% (std=0.11) delta: -0.12%
    * Latency p99: 53.38 ± 2.72% (std=1.08) delta: -0.88%
            
oltp_point_select:
    * QPS: 45167.62 ± 0.41% (std=131.52) delta: -0.67% (p=0.073)
    * Latency p50: 2.83 ± 0.47% (std=0.01) delta: 0.74%
    * Latency p99: 9.39 ± 0.00% (std=0.00) delta: 3.64%
            

@MyonKeminta
Copy link
Contributor Author

It seems it doesn't harm performance much.

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
@MyonKeminta MyonKeminta removed the S: WIP label Nov 7, 2019
@MyonKeminta MyonKeminta changed the title [DNM] txn: Support bypass specified locks for reads txn: Support bypass specified locks for reads Nov 7, 2019
@MyonKeminta MyonKeminta marked this pull request as ready for review November 7, 2019 09:03
Signed-off-by: MyonKeminta <9948422+MyonKeminta@users.noreply.github.com>

Co-Authored-By: Lei Zhao <zlwgx1023@gmail.com>
@MyonKeminta MyonKeminta force-pushed the misono/read-bypass-checked-lock branch from 8904693 to 0a1499c Compare November 7, 2019 09:32
@sticnarf
Copy link
Contributor

sticnarf commented Nov 8, 2019

It seems it doesn't harm performance much.

sysbench is almost oltp. I'd like to try some AP scenarios...

@sticnarf
Copy link
Contributor

sticnarf commented Nov 8, 2019

/bench +tpch

@sre-bot
Copy link
Contributor

sre-bot commented Nov 8, 2019

Benchmark Report

Run TPC-H 10G Performance Test on VMs

@@                               Benchmark Diff                               @@
================================================================================
tidb: ccbdfdc5178cb84ec97577714b15624adc95380f
--- tikv: d019ccecefc260ff760a53b7b8742fb84ffca9b5
+++ tikv: 0a1499cc56f6298a567fe77d3d0041083beef913
pd: d6d08d9b58fa42a29c3b3f517b9a64a562475588
================================================================================
01.sql duration: 20210.15 ms ± 0.53% (std=68.27) delta: 4.49% (p=0.115)
02.sql duration: 7328.49 ms ± 4.57% (std=160.34) delta: 0.66% (p=0.193)
03.sql duration: 18411.80 ms ± 2.82% (std=273.98) delta: 2.39% (p=0.006)
04.sql duration: 7912.53 ms ± 1.36% (std=65.48) delta: 2.53% (p=0.008)
06.sql duration: 11909.05 ms ± 3.58% (std=224.18) delta: 12.03% (p=0.000)
07.sql duration: 17131.81 ms ± 2.68% (std=266.14) delta: 15.97% (p=0.000)
08.sql duration: 12972.24 ms ± 1.81% (std=128.05) delta: 5.07% (p=0.000)
09.sql duration: 32710.91 ms ± 0.86% (std=166.34) delta: 1.22% (p=0.004)
10.sql duration: 10579.74 ms ± 5.89% (std=333.27) delta: 3.37% (p=0.024)
11.sql duration: 3818.45 ms ± 5.12% (std=117.67) delta: -7.58% (p=0.026)
12.sql duration: 13495.40 ms ± 7.40% (std=426.07) delta: 8.12% (p=0.000)
13.sql duration: 9123.29 ms ± 1.20% (std=61.70) delta: 0.57% (p=0.176)
14.sql duration: 13322.05 ms ± 3.99% (std=254.67) delta: 11.36% (p=0.000)
15.sql duration: 24703.90 ms ± 6.57% (std=780.01) delta: 9.90% (p=0.000)
16.sql duration: 3703.83 ms ± 10.66% (std=226.33) delta: 2.31% (p=0.613)
17.sql duration: 37312.87 ms ± 1.47% (std=333.02) delta: 1.07% (p=0.672)
18.sql duration: 55925.76 ms ± 6.31% (std=2186.15) delta: -0.31% (p=0.756)
19.sql duration: 16229.23 ms ± 5.88% (std=500.72) delta: -0.22% (p=0.426)
20.sql duration: 13937.53 ms ± 5.78% (std=521.62) delta: 11.71% (p=0.000)
21.sql duration: 34304.90 ms ± 1.44% (std=327.02) delta: 17.58% (p=0.000)
22.sql duration: 5188.18 ms ± 6.52% (std=183.80) delta: 4.24% (p=0.502)

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
@coocood
Copy link
Contributor

coocood commented Nov 8, 2019

LGTM

@sticnarf
Copy link
Contributor

sticnarf commented Nov 8, 2019

/bench +tpch

@sre-bot
Copy link
Contributor

sre-bot commented Nov 8, 2019

Benchmark Report

Run TPC-H 10G Performance Test on VMs

@@                               Benchmark Diff                               @@
================================================================================
tidb: 87357daba56e5d009653cec4d3996621d436e921
--- tikv: 1999eaf8a6da5d4d7cc8f89a0d1127c516614aef
+++ tikv: aebad246522dfeb05eaf6575dd7449e2f697d38e
pd: d6d08d9b58fa42a29c3b3f517b9a64a562475588
================================================================================
01.sql duration: 18926.16 ms ± 0.22% (std=32.52) delta: -0.49% (p=0.630)
02.sql duration: 7494.42 ms ± 3.27% (std=140.65) delta: -0.46% (p=0.480)
03.sql duration: 17555.67 ms ± 1.57% (std=151.71) delta: -4.00% (p=0.000)
04.sql duration: 7576.70 ms ± 2.65% (std=102.09) delta: 0.56% (p=0.430)
06.sql duration: 10628.32 ms ± 6.69% (std=552.20) delta: 1.90% (p=0.069)
07.sql duration: 14911.16 ms ± 3.17% (std=302.80) delta: 1.50% (p=0.078)
08.sql duration: 12877.82 ms ± 1.66% (std=124.04) delta: 2.70% (p=0.000)
09.sql duration: 33663.63 ms ± 3.03% (std=600.69) delta: 1.34% (p=0.402)
10.sql duration: 10450.32 ms ± 2.79% (std=154.79) delta: 0.04% (p=0.921)
11.sql duration: 3698.14 ms ± 0.98% (std=20.08) delta: -2.10% (p=0.024)
12.sql duration: 12544.98 ms ± 2.93% (std=209.48) delta: 2.64% (p=0.188)
13.sql duration: 9410.92 ms ± 5.63% (std=249.32) delta: 3.43% (p=0.074)
14.sql duration: 12007.14 ms ± 2.70% (std=228.98) delta: 1.91% (p=0.874)
15.sql duration: 23035.28 ms ± 4.37% (std=643.00) delta: 7.49% (p=0.006)
16.sql duration: 3869.06 ms ± 14.43% (std=303.38) delta: 0.70% (p=0.363)
17.sql duration: 38518.79 ms ± 1.42% (std=312.57) delta: 0.15% (p=0.910)
18.sql duration: 57919.08 ms ± 2.02% (std=712.21) delta: -0.15% (p=0.553)
19.sql duration: 15665.32 ms ± 3.25% (std=243.74) delta: -4.25% (p=0.000)
20.sql duration: 12120.41 ms ± 2.66% (std=162.25) delta: 1.45% (p=0.209)
21.sql duration: 30523.10 ms ± 1.10% (std=200.73) delta: 1.59% (p=0.004)
22.sql duration: 5103.51 ms ± 5.77% (std=174.59) delta: 3.80% (p=0.060)

@sticnarf
Copy link
Contributor

Actually I think the Arc is avoidable, but it should introduce some lifetimes in ScannerConfig and PointGetter. Probably not that necessary...

@MyonKeminta
Copy link
Contributor Author

MyonKeminta commented Nov 12, 2019

@sticnarf I think it's only sometimes avoidable in the current structure, since the SnapshotScanner sometimes needs impl Send to move to another thread.

@MyonKeminta
Copy link
Contributor Author

/release

Copy link
Contributor

@sticnarf sticnarf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@youjiali1995 youjiali1995 added the status/can-merge Status: Can merge to base branch label Nov 12, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Nov 12, 2019

Your auto merge job has been accepted, waiting for 5837, 5843, 5829

@sre-bot
Copy link
Contributor

sre-bot commented Nov 12, 2019

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Nov 12, 2019

@MyonKeminta merge failed.

@youjiali1995
Copy link
Contributor

/test

@youjiali1995 youjiali1995 merged commit ed4d332 into tikv:master Nov 12, 2019
@MyonKeminta MyonKeminta deleted the misono/read-bypass-checked-lock branch November 13, 2019 05:27
hawkingrei pushed a commit to hawkingrei/tikv that referenced this pull request Dec 1, 2019
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/transaction SIG: Transaction status/can-merge Status: Can merge to base branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants