raftstore: remove the local reader thread #4558

5kbpers · 2019-04-23T08:45:44Z

What have you changed? (mandatory)

In TiKV, all read requests are collected into batches and executed by a thread called local-reader.
We found that this thread has become a bottleneck of read performance.
This PR removes this thread and moves the execution of read request to the readpool, which improves the read performance and reduces context switches.

What are the type of the changes? (mandatory)

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

How has this PR been tested? (mandatory)

Unit test, integration test, Jepsen test(still running, don't find any problem by now)

Does this PR affect tidb-ansible update? (mandatory)

Yes. see pingcap/tidb-ansible#753

Signed-off-by: 5kbpers <tangminghua@pingcap.com>

5kbpers · 2019-04-24T04:49:54Z

/run-integration-tests

zhangjinpeng87 · 2019-04-24T05:03:48Z

After this pr merged, don't forget to modify tikv's grafana in tidb-ansible repo.

ngaut · 2019-04-24T05:05:00Z

Do we have any benchmark results?

5kbpers · 2019-04-24T05:31:16Z

After this pr merged, don't forget to modify tikv's grafana in tidb-ansible repo.

@zhangjinpeng1987 OK

5kbpers · 2019-04-24T05:38:46Z

Do we have any benchmark results?

@ngaut I wrote a test log on confluence: 2019-04 Remove local reader thread

ngaut · 2019-04-24T06:20:04Z

Could you post a brief result here?

5kbpers · 2019-04-24T06:41:40Z

YCSB Stress Testing

TiKV Configuration

readpool.storage.high-concurrency: 24
readpool.storage.normal-concurrency: 24
readpool.storage.low-concurrency: 24
readpool.coprocessor.high-concurrency: 24
readpool.coprocessor.normal-concurrency: 24
readpool.coprocessor.low-concurrency: 24
server.grpc-concurrency: 12
server.stats-concurrency: 5

Deploy single TiKV instance and single PD instance.

YCSB Configuration

use go-ycsb.
workload:

recordcount=10000000
operationcount=100000000
workload=core
readallfields=true
readproportion=1
updateproportion=0
scanproportion=0
insertproportion=0
requestdistribution=uniform

Two YCSB clients, script:

./bin/go-ycsb run tikv -P workload-point-get -p tikv.type="txn" -p threadcount=2048

Result

Both throughput and latency get pretty improvement, and the frequency of context switch has decrease.

5kbpers · 2019-04-24T06:44:16Z

Could you post a brief result here?

@ngaut OK, I have just posted it, see the previous comment.

ngaut · 2019-04-24T07:28:56Z

Thanks.

Signed-off-by: 5kbpers <tangminghua@pingcap.com>

5kbpers · 2019-04-29T05:32:30Z

/run-all-tests

5kbpers · 2019-05-05T01:36:38Z

/run-all-tests

src/binutil/server.rs

src/raftstore/store/worker/read.rs

Connor1996 · 2019-05-22T07:50:19Z

src/raftstore/store/peer.rs

@@ -370,14 +370,6 @@ impl Peer {
    pub fn activate<T, C>(&self, ctx: &PollContext<T, C>) {


please update the comment, and I prefer writting

let mut meta = ctx.store_meta.lock().unwrap(); meta.readers .insert(self.region_id, ReadDelegate::from_peer(self));

here rather than in post_raft_ready_append

I guess seperating them is better for now, because activate is also called in fsm/peer.rs, where the meta is already locked. It will introduce much more changes.

src/raftstore/store/peer.rs

…into thread-local-reader

Signed-off-by: qupeng <qupeng@pingcap.com>

hicqu · 2019-05-22T12:37:48Z

PTAL @Connor1996 thanks

Connor1996

LGTM

…into thread-local-reader

hicqu · 2019-05-22T14:53:24Z

/run-all-tests

Connor1996

LGTM

overvenus

Rest LGTM

src/raftstore/store/peer.rs

overvenus · 2019-05-22T15:20:36Z

src/raftstore/store/worker/read.rs

+                        return;
+                    } else {
+                        // Remove delegate for updating it by next cmd execution.
+                        self.delegates.borrow_mut().remove(&region_id);


Why not move this line to L339?

components/test_raftstore/src/server.rs

zhangjinpeng87 · 2019-05-23T01:40:23Z

src/raftstore/store/worker/read.rs

            Some(delegate) => {
                fail_point!("localreader_on_find_delegate");
-                delegate
+                match delegate.take() {


Why take away? How about the following read requests for this region?

It's because of lifetime. After handle a request, it will be put into the hashmap back.

This may be a performance issue, please pay attention to it.

Signed-off-by: qupeng <qupeng@pingcap.com>

Connor1996

LGTM

hicqu · 2019-05-23T04:10:21Z

/run-all-tests

Signed-off-by: 5kbpers <tangminghua@pingcap.com>

5kbpers added 8 commits April 18, 2019 16:35

*: remove local reader thread

dca5e33

Signed-off-by: 5kbpers <tangminghua@pingcap.com>

Merge branch 'master' into 'thread-local-reader'

924aebf

Signed-off-by: 5kbpers <tangminghua@pingcap.com>

*: modify tests for removing local-reader thread & fix downcast bug

4536534

Signed-off-by: 5kbpers <tangminghua@pingcap.com>

tikv-server: fix thread name of storage readpool

8848b76

Signed-off-by: 5kbpers <tangminghua@pingcap.com>

*: fix LocalReader test & remove unused import

8257c0a

Signed-off-by: 5kbpers <tangminghua@pingcap.com>

*: move 'LocalReader' member to ServerRaftStoreRouter

53cbbdc

Signed-off-by: 5kbpers <tangminghua@pingcap.com>

*: engine tls may not be initialized

9b94032

Signed-off-by: 5kbpers <tangminghua@pingcap.com>

storage/mod.rs: move execution of snapshot operation into readpool

0dbf9e5

Signed-off-by: 5kbpers <tangminghua@pingcap.com>

5kbpers force-pushed the thread-local-reader branch from 7580914 to 0dbf9e5 Compare April 23, 2019 09:58

5kbpers added 4 commits April 23, 2019 18:00

storage/mod.rs: don't init tls engine in test

0922082

Signed-off-by: 5kbpers <tangminghua@pingcap.com>

tikv-server: fix thread name of storage readpool

120a98e

Signed-off-by: 5kbpers <tangminghua@pingcap.com>

raftstore/worker/read.rs: comment unused code

7fabf05

Signed-off-by: 5kbpers <tangminghua@pingcap.com>

readpool_impl.rs: destroy tls engine before threadpool is destroyed

6dc26c6

Signed-off-by: 5kbpers <tangminghua@pingcap.com>

raftstore/worker/read.rs: collect metrics of LocalReader

686baf9

Signed-off-by: 5kbpers <tangminghua@pingcap.com>

5kbpers force-pushed the thread-local-reader branch from 993242e to 686baf9 Compare April 24, 2019 08:28

siddontang and others added 2 commits April 27, 2019 11:56

Merge branch 'master' into thread-local-reader

e77cd6f

Merge branch 'master' into thread-local-reader

5010315

5kbpers mentioned this pull request May 5, 2019

tikv: remove the local reader thread pingcap/tidb-ansible#753

Merged

5kbpers changed the title ~~[DNM] raftstore: remove local reader thread~~ raftstore: remove local reader thread May 6, 2019

hicqu added 3 commits May 21, 2019 18:59

Merge branch 'master' into scheduler-get-snapshot-improve

3f381bd

Merge branch 'master' into thread-local-reader

d4cda76

Merge branch 'scheduler-get-snapshot-improve' into thread-local-reader

96a3354

hicqu mentioned this pull request May 21, 2019

All pending features #4749

Closed

5 tasks

Merge branch 'master' into thread-local-reader

3f8b7d9

zhangjinpeng87 reviewed May 22, 2019

View reviewed changes

src/binutil/server.rs Outdated Show resolved Hide resolved

Connor1996 reviewed May 22, 2019

View reviewed changes

hicqu added 3 commits May 22, 2019 20:01

Merge branch 'thread-local-reader' of https://github.com/5kbpers/tikv …

974cd7f

…into thread-local-reader

address comments.

984af31

Signed-off-by: qupeng <qupeng@pingcap.com>

Merge branch 'master' into thread-local-reader

4f9163d

Connor1996 previously approved these changes May 22, 2019

View reviewed changes

hicqu added 2 commits May 22, 2019 22:48

Merge branch 'master' into thread-local-reader

2b3afb6

Merge branch 'thread-local-reader' of https://github.com/5kbpers/tikv …

df80b26

…into thread-local-reader

hicqu dismissed Connor1996’s stale review via df80b26 May 22, 2019 14:52

Connor1996 previously approved these changes May 22, 2019

View reviewed changes

overvenus reviewed May 22, 2019

View reviewed changes

zhangjinpeng87 reviewed May 23, 2019

View reviewed changes

components/test_raftstore/src/server.rs Outdated Show resolved Hide resolved

zhangjinpeng87 reviewed May 23, 2019

View reviewed changes

hicqu added 2 commits May 23, 2019 11:17

Merge branch 'master' into thread-local-reader

dbebc38

address comments

32fe2a6

Signed-off-by: qupeng <qupeng@pingcap.com>

hicqu dismissed Connor1996’s stale review via 32fe2a6 May 23, 2019 04:04

overvenus approved these changes May 23, 2019

View reviewed changes

Connor1996 approved these changes May 23, 2019

View reviewed changes

hicqu merged commit 0a9ebfc into tikv:master May 23, 2019

jswh pushed a commit to jswh/tikv that referenced this pull request May 27, 2019

raftstore: remove the local reader thread (tikv#4558)

8c80fb5

Signed-off-by: 5kbpers <tangminghua@pingcap.com>

hicqu mentioned this pull request May 31, 2019

raftstore: remove local reader cache #4802

Closed

sticnarf pushed a commit to sticnarf/tikv that referenced this pull request Oct 27, 2019

raftstore: remove the local reader thread (tikv#4558)

fdbaf9b

Signed-off-by: 5kbpers <tangminghua@pingcap.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

raftstore: remove the local reader thread #4558

raftstore: remove the local reader thread #4558

5kbpers commented Apr 23, 2019 •

edited

Loading

5kbpers commented Apr 24, 2019

zhangjinpeng87 commented Apr 24, 2019 •

edited

Loading

ngaut commented Apr 24, 2019

5kbpers commented Apr 24, 2019

5kbpers commented Apr 24, 2019

ngaut commented Apr 24, 2019

5kbpers commented Apr 24, 2019

5kbpers commented Apr 24, 2019

ngaut commented Apr 24, 2019

5kbpers commented Apr 29, 2019

5kbpers commented May 5, 2019

Connor1996 May 22, 2019

hicqu May 22, 2019

hicqu commented May 22, 2019

Connor1996 left a comment

hicqu commented May 22, 2019

Connor1996 left a comment

overvenus left a comment

overvenus May 22, 2019

zhangjinpeng87 May 23, 2019 •

edited

Loading

hicqu May 23, 2019

zhangjinpeng87 May 23, 2019

hicqu May 23, 2019

Connor1996 left a comment

hicqu commented May 23, 2019

		@@ -370,14 +370,6 @@ impl Peer {
		pub fn activate<T, C>(&self, ctx: &PollContext<T, C>) {

raftstore: remove the local reader thread #4558

raftstore: remove the local reader thread #4558

Conversation

5kbpers commented Apr 23, 2019 • edited Loading

What have you changed? (mandatory)

What are the type of the changes? (mandatory)

How has this PR been tested? (mandatory)

Does this PR affect tidb-ansible update? (mandatory)

5kbpers commented Apr 24, 2019

zhangjinpeng87 commented Apr 24, 2019 • edited Loading

ngaut commented Apr 24, 2019

5kbpers commented Apr 24, 2019

5kbpers commented Apr 24, 2019

ngaut commented Apr 24, 2019

5kbpers commented Apr 24, 2019

YCSB Stress Testing

TiKV Configuration

YCSB Configuration

Result

5kbpers commented Apr 24, 2019

ngaut commented Apr 24, 2019

5kbpers commented Apr 29, 2019

5kbpers commented May 5, 2019

Connor1996 May 22, 2019

Choose a reason for hiding this comment

hicqu May 22, 2019

Choose a reason for hiding this comment

hicqu commented May 22, 2019

Connor1996 left a comment

Choose a reason for hiding this comment

hicqu commented May 22, 2019

Connor1996 left a comment

Choose a reason for hiding this comment

overvenus left a comment

Choose a reason for hiding this comment

overvenus May 22, 2019

Choose a reason for hiding this comment

zhangjinpeng87 May 23, 2019 • edited Loading

Choose a reason for hiding this comment

hicqu May 23, 2019

Choose a reason for hiding this comment

zhangjinpeng87 May 23, 2019

Choose a reason for hiding this comment

hicqu May 23, 2019

Choose a reason for hiding this comment

Connor1996 left a comment

Choose a reason for hiding this comment

hicqu commented May 23, 2019

5kbpers commented Apr 23, 2019 •

edited

Loading

zhangjinpeng87 commented Apr 24, 2019 •

edited

Loading

zhangjinpeng87 May 23, 2019 •

edited

Loading