txn: use tokio_threadpool for txn scheduler #4628

fredchenbj · 2019-05-05T12:06:51Z

Signed-off-by: fredchenbj cfworking@163.com

What have you changed? (mandatory)

use tokio-threadpool to replace threadpool in tikv_util used by txn scheduler.

The currently defined types are listed below, please pick one of the types for this PR by removing the others:

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

How has this PR been tested? (mandatory)

manual tests

Signed-off-by: fredchenbj <cfworking@163.com>

sre-bot · 2019-05-05T12:06:55Z

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

siddontang · 2019-05-06T01:14:51Z

Great, PTAL @breeswish @hicqu

siddontang · 2019-05-06T01:15:03Z

@fredchenbj
can you show us the benchmark results?

zhangjinpeng87 · 2019-05-06T01:30:48Z

@fredchenbj Do you test the binary with grafana and check if these metrics work well?

fredchenbj · 2019-05-06T02:49:39Z

I use go-ycsb tool with command: ./bin/go-ycsb load/run tikv -P workloads/workloada -p tikv.pd="10.136.16.2:2379" -p tikv.type="txn" -p tikv.conncount=32 --threads=32 --target=3000. The result of test case is followed:

load
master: Avg(us): 6779, Min(us): 4925, Max(us): 393967, 95th(us): 9000, 99th(us): 13000

tokio:  Avg(us): 6608, Min(us): 4743, Max(us): 363260, 95th(us): 8000, 99th(us): 13000

run
master: Avg(us): 2748, Min(us): 1898, Max(us): 229763, 95th(us): 4000, 99th(us): 5000

tokio:  Avg(us): 2841, Min(us): 1929, Max(us): 210304, 95th(us): 4000, 99th(us): 5000

From above, the latency under certain load is not decreased obviously, and the context switch decreased about 5%.
@siddontang

fredchenbj · 2019-05-06T02:56:11Z

@fredchenbj Do you test the binary with grafana and check if these metrics work well?

I have tested the binary, I would check metrics more carefully and comment here later.

zhangjinpeng87 · 2019-05-06T05:33:21Z

and the context switch decreased about 5%

@fredchenbj I wonder why the context switch decreased, do you have any idea?

ngaut · 2019-05-06T10:46:41Z

src/storage/txn/sched_pool_impl.rs

+        .build()
+}
+
+#[inline]


Please remove all of the inline hints, let the compiler decide to optimize the code.

@ngaut The inline hint still letting the compiler decide inline or not. (note that it is not #[inline(always)])

I know, we don't need to do that, usually the compiler is smart enough to make the decision.

breezewish · 2019-05-06T10:53:25Z

src/storage/txn/sched_pool_impl.rs

@@ -0,0 +1,106 @@
+// Copyright 2019 TiKV Project Authors. Licensed under Apache-2.0.


No need to call it xxx_impl?

breezewish · 2019-05-06T10:56:02Z

src/storage/txn/sched_pool_impl.rs

+        .build()
+}
+
+#[inline]


@ngaut The inline hint still letting the compiler decide inline or not. (note that it is not #[inline(always)])

breezewish · 2019-05-06T10:56:37Z

src/storage/txn/sched_pool_impl.rs

+        m.borrow_mut()
+            .command_keyread_duration
+            .with_label_values(&[cmd])
+            .observe(count);


why this duration is called count?

breezewish · 2019-05-06T10:57:02Z

src/storage/txn/sched_pool_impl.rs

+        m.borrow_mut()
+            .processing_read_duration
+            .with_label_values(&[cmd])
+            .start_coarse_timer();


This timer does not record any time.

Signed-off-by: fredchenbj <cfworking@163.com>

…heduler-and-process

fredchenbj · 2019-05-07T09:22:30Z

@fredchenbj Do you test the binary with grafana and check if these metrics work well?

I have check these metrics work as before.

and the context switch decreased about 5%

@fredchenbj I wonder why the context switch decreased, do you have any idea?

I don't have any ideas about this.

breezewish · 2019-05-07T10:18:56Z

src/storage/txn/sched_pool.rs

+    }
+}
+
+pub fn build_sched_pool(pool_size: usize, name_prefix: &str) -> FuturePool {


The name is quite confusing.. build_sched_pool does not build a SchedPool but a FuturePool instead.

breezewish · 2019-05-07T10:20:27Z

src/storage/metrics.rs

@@ -5,6 +5,9 @@ use prometheus_static_metric::*;

 make_static_metric! {
    pub label_enum CommandKind {
+        get,


Looks like this change is not necessary?

yes, here I tested wrong before, and will remove it.

breezewish · 2019-05-07T10:20:44Z

src/storage/txn/process.rs

        Executor {
            scheduler: Some(scheduler),
-            pool: Some(pool),
+            schedpool: Some(pool),


prefer sched_pool

breezewish · 2019-05-07T10:22:36Z

@zhangjinpeng1987 A work-stealing pool have local task queues, which may help with reducing context switch.

Signed-off-by: fredchenbj <cfworking@163.com>

breezewish · 2019-05-07T12:21:57Z

/run-integration-tests

fredchenbj · 2019-05-08T10:44:24Z

friendly ping @siddontang @breeswish

siddontang · 2019-05-13T00:38:11Z

src/storage/txn/process.rs

-                .processing_read_duration
-                .with_label_values(&[tag])
-                .start_coarse_timer();
+            let read_duration = tikv_util::time::Instant::now_coarse();


can we use Instant::now_coarse() here?

Signed-off-by: fredchenbj <cfworking@163.com>

breezewish

Mostly fine. @zhangjinpeng1987 PTAL

…heduler-and-process

siddontang · 2019-05-17T05:49:09Z

PTAL @hicqu

please fix the confliect @fredchenbj

…heduler-and-process

hicqu · 2019-05-17T06:13:08Z

I merged master and resolved some conflicts. PTAL again @breeswish thanks!

hicqu · 2019-05-17T06:20:00Z

LGTM

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

Signed-off-by: fredchenbj <cfworking@163.com>

txn: use tokio_thread for txn scheduler

fb0d4bb

Signed-off-by: fredchenbj <cfworking@163.com>

fredchenbj changed the title ~~txn: use tokio_thread for txn scheduler~~ txn: use tokio_threadpool for txn scheduler May 6, 2019

breezewish added component/performance Component: Performance contribution This PR is from a community contributor. labels May 6, 2019

ngaut reviewed May 6, 2019

View reviewed changes

breezewish reviewed May 6, 2019

View reviewed changes

fredbjer added 2 commits May 7, 2019 16:46

fix something and add some metric types

f081034

Signed-off-by: fredchenbj <cfworking@163.com>

Merge branch 'master' into fredchenbj/use-tokio-threadpool-for-txn-sc…

6a17a4a

…heduler-and-process

breezewish reviewed May 7, 2019

View reviewed changes

fix something

c6c0f80

Signed-off-by: fredchenbj <cfworking@163.com>

siddontang reviewed May 13, 2019

View reviewed changes

little fix

033d7b3

Signed-off-by: fredchenbj <cfworking@163.com>

breezewish previously approved these changes May 13, 2019

View reviewed changes

Merge branch 'master' into fredchenbj/use-tokio-threadpool-for-txn-sc…

b36d254

…heduler-and-process

hicqu dismissed breezewish’s stale review via e0113ce May 17, 2019 06:07

Merge branch 'master' into fredchenbj/use-tokio-threadpool-for-txn-sc…

9274fd6

…heduler-and-process

hicqu force-pushed the fredchenbj/use-tokio-threadpool-for-txn-scheduler-and-process branch from e0113ce to 3b84fae Compare May 17, 2019 06:12

hicqu force-pushed the fredchenbj/use-tokio-threadpool-for-txn-scheduler-and-process branch from 3b84fae to 9274fd6 Compare May 17, 2019 06:14

hicqu previously approved these changes May 17, 2019

View reviewed changes

fix a bug introduced in the latest merge from master

9c9ac7a

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

hicqu dismissed their stale review via 9c9ac7a May 17, 2019 06:28

hicqu approved these changes May 17, 2019

View reviewed changes

breezewish approved these changes May 17, 2019

View reviewed changes

breezewish merged commit 93146c2 into tikv:master May 17, 2019

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

txn: use tokio_threadpool for txn scheduler (tikv#4628)

a92fc40

Signed-off-by: fredchenbj <cfworking@163.com>

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

txn: use tokio_threadpool for txn scheduler (tikv#4628)

82e7195

Signed-off-by: fredchenbj <cfworking@163.com>

fredchenbj mentioned this pull request Dec 18, 2019

Add @fredchenbj to engine-sig reviewer tikv/community#42

Merged

zhangjinpeng87 mentioned this pull request Mar 26, 2020

MAINTAINERS: add Fu Chen as TiKV maintainer #7259

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

txn: use tokio_threadpool for txn scheduler #4628

txn: use tokio_threadpool for txn scheduler #4628

fredchenbj commented May 5, 2019 •

edited

Loading

sre-bot commented May 5, 2019

siddontang commented May 6, 2019

siddontang commented May 6, 2019

zhangjinpeng87 commented May 6, 2019

fredchenbj commented May 6, 2019 •

edited

Loading

fredchenbj commented May 6, 2019

zhangjinpeng87 commented May 6, 2019

ngaut May 6, 2019

breezewish May 6, 2019

ngaut May 6, 2019

breezewish May 6, 2019

breezewish May 6, 2019

breezewish May 6, 2019

breezewish May 6, 2019

fredchenbj commented May 7, 2019

breezewish May 7, 2019

breezewish May 7, 2019

fredchenbj May 7, 2019

breezewish May 7, 2019

breezewish commented May 7, 2019

breezewish commented May 7, 2019

fredchenbj commented May 8, 2019

siddontang May 13, 2019

fredchenbj May 13, 2019

breezewish left a comment

siddontang commented May 17, 2019

hicqu commented May 17, 2019

hicqu commented May 17, 2019

		@@ -0,0 +1,106 @@
		// Copyright 2019 TiKV Project Authors. Licensed under Apache-2.0.

txn: use tokio_threadpool for txn scheduler #4628

txn: use tokio_threadpool for txn scheduler #4628

Conversation

fredchenbj commented May 5, 2019 • edited Loading

What have you changed? (mandatory)

How has this PR been tested? (mandatory)

sre-bot commented May 5, 2019

siddontang commented May 6, 2019

siddontang commented May 6, 2019

zhangjinpeng87 commented May 6, 2019

fredchenbj commented May 6, 2019 • edited Loading

fredchenbj commented May 6, 2019

zhangjinpeng87 commented May 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredchenbj commented May 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

breezewish commented May 7, 2019

breezewish commented May 7, 2019

fredchenbj commented May 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

breezewish left a comment

Choose a reason for hiding this comment

siddontang commented May 17, 2019

hicqu commented May 17, 2019

hicqu commented May 17, 2019

fredchenbj commented May 5, 2019 •

edited

Loading

fredchenbj commented May 6, 2019 •

edited

Loading