Batch (raw) get requests to same region across batch commands by tabokie · Pull Request #5598 · tikv/tikv

tabokie · 2019-10-09T06:08:42Z

What have you changed?

Batch get / raw-get requests with same priority to the same region.

To avoid request timeout due to batch too large or short of timeslice, add one thread of interval timer and periodically issues a forced submit.

This change will reduce readpool tasking pressure and in turn cpu usage. The effect is relevent to gRPC connection count (contribute to qps in one stream) and batch wait duration.

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

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

No

Does this PR affect `tidb-ansible`?

Will do

Refer to a related PR or issue link (optional)

This PR is split from #5382

Benchmark result if necessary (optional)

~~improve 17% qps for oltp_point_select bench (grpc-connection-count=3, request-batch-wait-duration="1ms") without compromising other workloads.~~

~~improve 20% qps for oltp_point_select (grpc-connection-count=1 or 2, request-batch-wait-duration="1ms"), with a notable side effect on other workloads.~~

This PR has seen some major performance speed-ups on tikv master since it first started (#5363, #5654 ). Now request batch is only feasible under higher perssure.

Here is a result from sysbench oltp_point_select 1024 thread, on 1 tidb 1 tikv each on seperate server with 40 core. (with tidb point get grpc conn of 4)

before batch, tikv cpu ~ 900%

SQL statistics:
queries performed:
read: 82163942
write: 0
other: 0
total: 82163942
transactions: 82163942 (228121.25 per sec.)
queries: 82163942 (228121.25 per sec.)
ignored errors: 0 (0.00 per sec.)
reconnects: 0 (0.00 per sec.)

General statistics:
total time: 360.1744s
total number of events: 82163942

Latency (ms):
min: 0.23
avg: 4.49
max: 49.41
95th percentile: 7.17
sum: 368591486.94

Threads fairness:
events (avg/stddev): 80238.2246/61.32
execution time (avg/stddev): 359.9526/0.00

after batch, tikv cpu ~ 550%

SQL statistics:
queries performed:
read: 91619058
write: 0
other: 0
total: 91619058
transactions: 91619058 (254370.46 per sec.)
queries: 91619058 (254370.46 per sec.)
ignored errors: 0 (0.00 per sec.)
reconnects: 0 (0.00 per sec.)

General statistics:
total time: 360.1777s
total number of events: 91619058

Latency (ms):
min: 0.33
avg: 4.02
max: 45.34
95th percentile: 7.30
sum: 368587682.66

Threads fairness:
events (avg/stddev): 89471.7363/113.70
execution time (avg/stddev): 359.9489/0.00

Any examples? (optional)

Signed-off-by: tabokie <xy.tao@outlook.com>

tabokie · 2019-10-09T07:15:00Z

/run-all-tests/bench

src/server/service/kv.rs

mahjonp · 2019-10-09T07:20:22Z

/bench tidb=pr/12569

src/server/service/kv.rs

zhangjinpeng87 · 2019-10-09T08:06:59Z

src/storage/mod.rs

+                                Err(e) => return future::err(e),
+                            };
+                            for get in gets {
+                                callback(


Can we only use one callback for these get requests to reduce the cost?

The update roughly achieves -50% gRPC poll CPU and +9% qps in oltp_point_select.

src/storage/mod.rs

src/server/service/kv.rs

zhangjinpeng87 · 2019-10-09T08:24:29Z

src/server/service/kv.rs

+        let minibatcher = MiniBatcher::new(tx.clone(), self.minibatch_wait_millis);
+        let stopped = Arc::new(AtomicBool::new(false));
+        let minibatcher = Arc::new(Mutex::new(minibatcher));
+        if self.minibatch_wait_millis > 0 {


@hicqu @BusyJay PTAL here

src/server/service/kv.rs

BusyJay · 2019-10-09T08:57:07Z

Can you add some comments to help reviewer better understand the code?

src/server/service/kv.rs

Signed-off-by: tabokie <xy.tao@outlook.com>

BusyJay

Why doing batch inside the kv service instead of storage?

src/server/service/kv.rs

zhangjinpeng87 · 2019-10-09T09:54:55Z

/bench tidb=pr/12569

tabokie · 2019-10-09T10:31:53Z

Why doing batch inside the kv service instead of storage?

@BusyJay Currently the batch is collected within the lifetime of one batch_commands stream, and batch response depends on the liveness of this stream. To batch at a lower level (and to batch cross threads), each command must held their own reference to the message channel, and we have to make a thread-safe batcher, which I think is a bit costly. The downside is batch effect is too dependent on grpc connection (high connection leads to scattered batch).

Signed-off-by: tabokie <xy.tao@outlook.com>

sre-bot · 2019-10-09T11:22:21Z

tidb.grpc-connection-count=1, batch.timeout=0ms

@@                               Benchmark Diff                               @@
================================================================================
--- tidb: be6163c823285eb7bd048c8d0377c7b05dfd464e
+++ tidb: a387fbf23e26f8f0dc6b140489d04860bc7ba942
--- tikv: 00b0234fecbe97f53700b125ce1f5793ba88531b
+++ tikv: 4eb0c21f0c91f1eefad69bbcc6fc743e3c3bf7a3
pd: 4acaa8c715d40a07f521dd85ef7dcd4118064289
================================================================================
test-1: < oltp_point_select >
    * QPS : 64453.41 ± 2.2211% (std=921.42) delta: -18.37% (p=0.000)
    * AvgMs : 3.97 ± 2.2670% (std=0.06) delta: 22.46%
    * PercentileMs99 : 8.28 ± 0.0000% (std=0.00) delta: 24.14%
            
test-2: < oltp_read_write >
    * QPS : 25687.45 ± 0.8507% (std=150.98) delta: -32.83% (p=0.000)
    * AvgMs : 200.13 ± 0.8644% (std=1.19) delta: 48.86%
    * PercentileMs99 : 312.21 ± 1.0781% (std=2.75) delta: 21.03%
            
test-3: < oltp_insert >
    * QPS : 9036.30 ± 1.8150% (std=119.24) delta: -59.13% (p=0.000)
    * AvgMs : 28.33 ± 1.8002% (std=0.37) delta: 144.70%
    * PercentileMs99 : 46.91 ± 1.1938% (std=0.40) delta: 97.28%
            
test-4: < oltp_update_index >
    * QPS : 7239.76 ± 2.6838% (std=156.80) delta: -58.45% (p=0.000)
    * AvgMs : 35.32 ± 2.8537% (std=0.72) delta: 143.50%
    * PercentileMs99 : 48.78 ± 0.8919% (std=0.43) delta: 53.81%
            
test-5: < oltp_update_non_index >
    * QPS : 12779.03 ± 1.0768% (std=100.24) delta: -57.16% (p=0.000)
    * AvgMs : 20.03 ± 1.0815% (std=0.16) delta: 133.54%
    * PercentileMs99 : 29.19 ± 0.0000% (std=0.00) delta: 38.28%

https://perf.pingcap.com

tabokie · 2019-10-09T12:21:48Z

/run-all-tests

siddontang · 2019-10-09T12:28:39Z

oh, seem the perf reduced. @tabokie

Signed-off-by: tabokie <xy.tao@outlook.com>

tabokie · 2019-10-09T14:51:26Z

oh, seem the perf reduced. @tabokie

@siddontang Don't sweat, this PR only batch get request, should focus on oltp_point_select. As for oltp_point_select, the batch timeout is set to default 0ms in that run, I don't expect a performance gain considering gRPC conn is set to 1.

tabokie · 2019-10-09T15:18:38Z

/release make dist_release/bench tidb=pr/12569

sre-bot · 2019-10-09T15:31:03Z

download tikv at http://fileserver.pingcap.net/download/builds/pingcap/tikv/pr/9f37474f645f66a9689589169a8426e6e95fa66e/centos7/tikv-server.tar.gz

breezewish

The rest looks fine to me

src/server/service/kv.rs

Signed-off-by: tabokie <xy.tao@outlook.com>

Little-Wallace

LGTM

Signed-off-by: tabokie <xy.tao@outlook.com>

components/tikv_util/src/metrics/metrics_reader.rs

etc/config-template.toml

Signed-off-by: tabokie <xy.tao@outlook.com>

src/server/service/kv.rs

Signed-off-by: tabokie <xy.tao@outlook.com>

zhangjinpeng87

LGTM

Little-Wallace

LGTM

batch (raw) get request to same region

f0c1c6b

Signed-off-by: tabokie <xy.tao@outlook.com>

tabokie requested a review from zhangjinpeng87 October 9, 2019 06:09

tabokie added the component/performance Component: Performance label Oct 9, 2019

add unit test for batch read

200993b

Signed-off-by: tabokie <xy.tao@outlook.com>

tabokie force-pushed the batch-get branch from 38ae3d2 to 200993b Compare October 9, 2019 06:11

cleanup

4eb0c21

Signed-off-by: tabokie <xy.tao@outlook.com>

tabokie force-pushed the batch-get branch from 488332c to 4eb0c21 Compare October 9, 2019 06:58

zhangjinpeng87 reviewed Oct 9, 2019

View reviewed changes

src/server/service/kv.rs Outdated Show resolved Hide resolved

zhangjinpeng87 reviewed Oct 9, 2019

View reviewed changes

src/server/service/kv.rs Outdated Show resolved Hide resolved

zhangjinpeng87 reviewed Oct 9, 2019

View reviewed changes

src/storage/mod.rs Outdated Show resolved Hide resolved

zhangjinpeng87 reviewed Oct 9, 2019

View reviewed changes

src/server/service/kv.rs Outdated Show resolved Hide resolved

zhangjinpeng87 reviewed Oct 9, 2019

View reviewed changes

src/server/service/kv.rs Outdated Show resolved Hide resolved

zhangjinpeng87 reviewed Oct 9, 2019

View reviewed changes

src/server/service/kv.rs Outdated Show resolved Hide resolved

address comments

4ce1edd

Signed-off-by: tabokie <xy.tao@outlook.com>

BusyJay reviewed Oct 9, 2019

View reviewed changes

src/server/service/kv.rs Outdated Show resolved Hide resolved

src/server/service/kv.rs Show resolved Hide resolved

use single callback

a5b1f04

Signed-off-by: tabokie <xy.tao@outlook.com>

timeout set to 1ms

9f37474

Signed-off-by: tabokie <xy.tao@outlook.com>

breezewish reviewed Oct 25, 2019

View reviewed changes

address comments

3265591

Signed-off-by: tabokie <xy.tao@outlook.com>

tabokie dismissed Little-Wallace’s stale review via 3265591 October 25, 2019 09:25

Little-Wallace previously approved these changes Oct 25, 2019

View reviewed changes

zhangjinpeng87 and others added 2 commits October 25, 2019 20:38

Merge branch 'master' into batch-get

13230bf

fix comment

b799609

Signed-off-by: tabokie <xy.tao@outlook.com>

tabokie dismissed Little-Wallace’s stale review via b799609 October 28, 2019 01:52

tabokie requested a review from breezewish October 28, 2019 02:36

tikv deleted a comment from sre-bot Oct 28, 2019

minor fix

ce6ade5

Signed-off-by: tabokie <xy.tao@outlook.com>

zhangjinpeng87 reviewed Oct 29, 2019

View reviewed changes

components/tikv_util/src/metrics/metrics_reader.rs Outdated Show resolved Hide resolved

zhangjinpeng87 reviewed Oct 29, 2019

View reviewed changes

etc/config-template.toml Outdated Show resolved Hide resolved

tabokie added 4 commits October 29, 2019 09:57

address comments

fcc4ea3

Signed-off-by: tabokie <xy.tao@outlook.com>

Merge branch 'master' into batch-get

a804ade

address comment

5b819c1

Signed-off-by: tabokie <xy.tao@outlook.com>

fix context error

5427450

Signed-off-by: tabokie <xy.tao@outlook.com>

zhangjinpeng87 reviewed Oct 29, 2019

View reviewed changes

src/server/service/kv.rs Outdated Show resolved Hide resolved

zhangjinpeng87 and others added 2 commits October 29, 2019 14:06

Merge branch 'master' into batch-get

6d12424

address comment

24009f1

Signed-off-by: tabokie <xy.tao@outlook.com>

zhangjinpeng87 approved these changes Oct 29, 2019

View reviewed changes

Little-Wallace approved these changes Oct 29, 2019

View reviewed changes

Little-Wallace merged commit a9c1081 into tikv:master Oct 29, 2019

tabokie mentioned this pull request Oct 29, 2019

Batch prewrite and commit requests to same region across batch commands #5756

Merged

MyonKeminta mentioned this pull request Jan 8, 2020

Txn: move some commands functionality around. #6305

Merged

breezewish mentioned this pull request Mar 30, 2020

*: reduce sys_getdents syscall #7306

Merged

This was referenced Apr 17, 2020

*: reduce sys_getdents syscall (#7306) #7508

Closed

*: reduce sys_getdents syscall (#7306) #7510

Closed

*: reduce sys_getdents syscall (#7306) #7511

Closed

*: reduce sys_getdents syscall (#7306) #7512

Closed

tabokie deleted the batch-get branch December 23, 2020 03:16

Conversation

tabokie commented Oct 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What have you changed?

What is the type of the changes?

How is the PR tested?

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

Does this PR affect tidb-ansible?

Refer to a related PR or issue link (optional)

Benchmark result if necessary (optional)

Any examples? (optional)

Uh oh!

tabokie commented Oct 9, 2019

Uh oh!

Uh oh!

mahjonp commented Oct 9, 2019

Uh oh!

Uh oh!

zhangjinpeng87 Oct 9, 2019

Choose a reason for hiding this comment

Uh oh!

tabokie Oct 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zhangjinpeng87 Oct 9, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

BusyJay commented Oct 9, 2019

Uh oh!

Uh oh!

BusyJay left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zhangjinpeng87 commented Oct 9, 2019

Uh oh!

tabokie commented Oct 9, 2019

Uh oh!

sre-bot commented Oct 9, 2019 • edited by tabokie Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tabokie commented Oct 9, 2019

Uh oh!

siddontang commented Oct 9, 2019

Uh oh!

tabokie commented Oct 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tabokie commented Oct 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sre-bot commented Oct 9, 2019

Uh oh!

breezewish left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Little-Wallace left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zhangjinpeng87 left a comment

Choose a reason for hiding this comment

Uh oh!

Little-Wallace left a comment

tabokie commented Oct 9, 2019 •

edited

Loading

Does this PR affect `tidb-ansible`?

tabokie Oct 9, 2019 •

edited

Loading

sre-bot commented Oct 9, 2019 •

edited by tabokie

Loading

tabokie commented Oct 9, 2019 •

edited

Loading

tabokie commented Oct 9, 2019 •

edited

Loading