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

*: not send tso request when point get with max tso #11981

Merged
merged 16 commits into from Sep 3, 2019

Conversation

@crazycs520
Copy link
Contributor

commented Sep 2, 2019

What problem does this PR solve?

Point get without double read will use maxUint64 as transaction tso. Then no need to send tso request to PD.

This PR will delay to get txn future after try fast point, so it may have a little performance regression in the case of need to get tso from PD. Especially in the situation of point get with double read.

BenchMark

Environment: In my local machine: 1 TiDB, 1 PD, 5 TiKV.

Best case: Point get with max tso

Before

  • point get QPS is 25.7k

image

image

This PR

  • point get QPS is 31.4k

image

image

Worst case: Point get with double read can't use max tso

Before

  • point get with double read QPS is 17.8k

In this PR

  • point get with double read QPS is 17.1k

As you can see, since I delay use get txn future, there is a performance regression is this situation.

What is changed and how it works?

Check List

Tests

  • No test.

Code changes

  • Has exported function/method change
crazycs520 added 4 commits Sep 2, 2019
@crazycs520

This comment has been minimized.

Copy link
Contributor Author

commented Sep 2, 2019

/run-all-tests

@crazycs520 crazycs520 requested review from tiancaiamao, coocood and jackysp Sep 2, 2019

@crazycs520

This comment has been minimized.

Copy link
Contributor Author

commented Sep 2, 2019

/bench

@crazycs520

This comment has been minimized.

Copy link
Contributor Author

commented Sep 2, 2019

@jackysp
Copy link
Member

left a comment

I think @cfzjywxk is doing something similar. I have tried it before. I think if you want to change the getting ts, it's best to make the prepare statement stop executing subqueries first.

@codecov

This comment has been minimized.

Copy link

commented Sep 2, 2019

Codecov Report

Merging #11981 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #11981   +/-   ##
===========================================
  Coverage   81.3309%   81.3309%           
===========================================
  Files           445        445           
  Lines         95468      95468           
===========================================
  Hits          77645      77645           
  Misses        12293      12293           
  Partials       5530       5530

@crazycs520 crazycs520 removed the status/WIP label Sep 2, 2019

@mahjonp

This comment has been minimized.

Copy link

commented Sep 2, 2019

@crazycs520 I had adjusted some configurations yesterday afternoon, so there existed an error in the benchmark result.

@pingcap pingcap deleted a comment from sre-bot Sep 2, 2019

@pingcap pingcap deleted a comment from sre-bot Sep 2, 2019

@crazycs520

This comment has been minimized.

Copy link
Contributor Author

commented Sep 2, 2019

/bench

@sre-bot

This comment has been minimized.

Copy link

commented Sep 2, 2019

@@                               Benchmark Diff                               @@
================================================================================
--- tidb: 728ed194c0669cb4effe6398de464c3ffbfb6608
+++ tidb: 46acbe07df9016a0fce3c5562e85ea60601df975
tikv: af70f367a38aad3305e32d8de17f60e09ea87fc3
pd: a1f8d9db4ac3e713c3259ca0979312fe3b61adac
================================================================================
test-1: < oltp_insert >
    * QPS : 21601.36 ± 0.2608% (std=40.10) delta: -0.15%
    * AvgMs : 11.84 ± 0.2744% (std=0.02) delta: 0.14%
    * PercentileMs99 : 42.31 ± 2.5526% (std=0.62) delta: 0.01%
            
test-2: < oltp_update_non_index >
    * QPS : 29636.88 ± 0.4531% (std=97.59) delta: -0.21%
    * AvgMs : 8.63 ± 0.4170% (std=0.03) delta: 0.19%
    * PercentileMs99 : 30.59 ± 1.0788% (std=0.27) delta: 1.09%
            
test-3: < oltp_read_write >
    * QPS : 37093.30 ± 0.2842% (std=71.10) delta: 0.00%
    * AvgMs : 138.57 ± 0.2656% (std=0.26) delta: -0.02%
    * PercentileMs99 : 257.95 ± 0.0000% (std=0.00) delta: 0.00%
            
test-4: < oltp_point_select >
    * QPS : 75141.81 ± 1.3490% (std=583.43) delta: -0.09%
    * AvgMs : 3.41 ± 1.2918% (std=0.03) delta: 0.18%
    * PercentileMs99 : 7.43 ± 0.0000% (std=0.00) delta: 0.00%
            
test-5: < oltp_update_index >
    * QPS : 17104.94 ± 0.8327% (std=86.63) delta: -0.20%
    * AvgMs : 14.96 ± 0.8420% (std=0.08) delta: 0.21%
    * PercentileMs99 : 48.17 ± 2.1675% (std=0.65) delta: 1.47%
            

https://perf.pingcap.com

planner/optimize.go Outdated Show resolved Hide resolved
planner/optimize.go Outdated Show resolved Hide resolved
@tiancaiamao

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2019

LGTM

@coocood

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

LGTM

@coocood coocood added the status/LGT2 label Sep 3, 2019

@crazycs520

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2019

/bench doesn't have performance improvement was caused by sysbench use prepare/execute to test point get, and prepare can't enjoy this PR's benefit.

@crazycs520

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2019

/run-all-tests

// 1. ctx is auto commit tagged.
// 2. plan is point get by pk.
func isPointGetWithoutDoubleRead(ctx sessionctx.Context, p plannercore.Plan) bool {
// check auto commit

This comment has been minimized.

Copy link
@winkyao

winkyao Sep 3, 2019

Member

this comment is meaningless?

@winkyao
Copy link
Member

left a comment

LGTM

@winkyao
winkyao approved these changes Sep 3, 2019
@sre-bot

This comment has been minimized.

Copy link

commented Sep 3, 2019

@@                               Benchmark Diff                               @@
================================================================================
--- tidb: 994e14e2e71ba16085c4e24d09d708a282beac18
+++ tidb: 7a0c170583f159d35143909851de4dee2cfc70d3
tikv: 6c420ba38cc0389c3cf21d4c5b5731ad110e6873
pd: 7baffcba6ae610f79854638aba50cd5feff9761f
================================================================================
test-1: < oltp_insert >
    * QPS : 21513.23 ± 0.1662% (std=20.90) delta: 0.02%
    * AvgMs : 11.89 ± 0.1472% (std=0.01) delta: -0.03%
    * PercentileMs99 : 42.61 ± 0.0000% (std=0.00) delta: 0.72%
            
test-2: < oltp_update_non_index >
    * QPS : 29595.16 ± 0.1788% (std=35.26) delta: -0.07%
    * AvgMs : 8.65 ± 0.1851% (std=0.01) delta: 0.01%
    * PercentileMs99 : 30.70 ± 2.1758% (std=0.41) delta: -0.35%
            
test-3: < oltp_read_write >
    * QPS : 36965.12 ± 0.3901% (std=86.92) delta: -0.18%
    * AvgMs : 139.05 ± 0.3912% (std=0.32) delta: 0.17%
    * PercentileMs99 : 257.95 ± 0.0000% (std=0.00) delta: 0.00%
            
test-4: < oltp_point_select >
    * QPS : 75528.47 ± 1.3784% (std=655.96) delta: 0.12%
    * AvgMs : 3.39 ± 1.5348% (std=0.03) delta: -0.06%
    * PercentileMs99 : 7.43 ± 0.0000% (std=0.00) delta: 0.70%
            
test-5: < oltp_update_index >
    * QPS : 17167.72 ± 0.4366% (std=56.26) delta: 0.52%
    * AvgMs : 14.91 ± 0.4191% (std=0.05) delta: -0.50%
    * PercentileMs99 : 47.99 ± 2.5379% (std=0.70) delta: -0.36%
            

https://perf.pingcap.com

@crazycs520 crazycs520 merged commit dffe293 into pingcap:master Sep 3, 2019

6 checks passed

idc-jenkins-ci-tidb/build Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/build_check_race Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/check_dev Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/check_dev_2 Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/unit-test Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details
jingyugao added a commit to jingyugao/tidb that referenced this pull request Sep 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.