storage: reduce scheduler msgs #3583

overvenus · 2018-09-06T09:59:23Z

What have you changed? (mandatory)

This PR reduces scheduler msgs, also offloads some functions from scheduler to workers.

What are the type of the changes? (mandatory)

Improvement (non-breaking change which is an improvement to an existing feature)

How has this PR been tested? (mandatory)

Unit tests and integration tests.

Does this PR affect documentation (docs/docs-cn) update? (mandatory)

No.

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

No.

Refer to a related PR or issue link (optional)

#3581 , #3551 and #3582

Benchmark result if necessary (optional)

Combining these changes #3581, #3551 and #3582, QPS is improved from 40672.1 to 49144.5, and avg latnecy is reduced from 100.6ms to 83.5ms.

Connor1996 · 2018-09-11T07:32:18Z

src/storage/txn/scheduler.rs

-            Msg::WriteFinished { cid, .. } => write!(f, "WriteFinished [cid={}]", cid),
+            Msg::ReadFinished { ref task, .. } => write!(f, "ReadFinished [cid={}]", task.cid),
+            Msg::WriteFinished { ref task, .. } => write!(f, "WriteFinished [cid={}]", task.cid),
+            Msg::FinishedWithErr { cid, .. } => write!(f, "WriteFinished [cid={}]", cid),


wrong format message

Connor1996 · 2018-09-11T09:04:00Z

src/storage/txn/process.rs

+            false
+        }
+        Err(e) => {
+            panic!("schedule WriteFinished msg failed, err:{:?}", e);


wrong message

Connor1996 · 2018-09-11T09:20:21Z

src/storage/txn/process.rs

@@ -619,6 +684,31 @@ impl ThreadContext for SchedContext {
    }
 }

+pub struct Channels<E: Engine> {
+    scheduler: Option<worker::Scheduler<Msg>>,


Maybe you should add some comments

Connor1996 · 2018-09-11T09:28:27Z

src/storage/txn/process.rs

+                                result,
+                            },
+                        );
+                        if success {


seems that it can be combined into previous one

zhangjinpeng87 · 2018-09-12T07:59:35Z

src/storage/txn/process.rs

@@ -619,6 +684,31 @@ impl ThreadContext for SchedContext {
    }
 }

+pub struct Channels<E: Engine> {


Maybe Executor is a better name?

zhangjinpeng87 · 2018-09-12T08:12:48Z

src/storage/txn/process.rs


-    Ok(())
+fn handle_schedule(scheduler: worker::Scheduler<Msg>, msg: Msg) -> bool {


Maybe send_msg_to_scheduler is a better name. Does this function can be a member of Channels?

Or notify_scheduler?

zhangjinpeng87 · 2018-09-12T09:49:53Z

src/storage/txn/process.rs


-impl<E: Engine> Scheduler<E> {
+    // Start the task.
+    pub fn start(&mut self) {


s/start/pre_start

overvenus · 2018-09-12T12:33:58Z

/run-all-tests

zhangjinpeng87 · 2018-09-13T01:35:32Z

src/storage/txn/process.rs

-        let tag = cmd.tag();
-        let pool = channels.take_pool();
-        if readcmd {
+        let pool = executor.take_pool();


I think it is more better to move following codes into Executor::exec, so the Executor is more like a executor.

zhangjinpeng87 · 2018-09-13T01:37:56Z

src/storage/txn/process.rs

            }
        }
    }

    /// Delivers a command to a worker thread for processing.
-    pub fn process_by_worker(&mut self, cid: u64, cb_ctx: CbContext, snapshot: E::Snap) {
+    fn process_by_worker<E: Engine>(


Maybe process_by_executor is more better.

zhangjinpeng87 · 2018-09-13T02:57:34Z

src/storage/txn/process.rs

-        let scheduler = self.scheduler.clone();
-        if readcmd {
-            worker_pool.execute(move |ctx: &mut SchedContext| {
+        let pool = executor.take_pool();


It's better to wrap following codes into Executor.

zhangjinpeng87 · 2018-09-13T09:03:12Z

src/storage/txn/scheduler.rs

+        let task = Task::new(cid, cmd);
+
+        // TODO: enqueue_task should return an reference of the entity.
+        self.enqueue_task(task, entity);


It is weird that enqueue_task has a entity arg.

Connor1996 · 2018-09-13T09:15:10Z

src/storage/txn/scheduler.rs

 }

 /// Scheduler which schedules the execution of `storage::Command`s.
 pub struct Scheduler<E: Engine> {
    engine: E,

    // cid -> Task
-    pub cmd_ctxs: HashMap<u64, Task>,
+    pending_task: HashMap<u64, Task>,


pending_task -> pending_tasks

BusyJay · 2018-09-13T09:46:25Z

src/storage/txn/scheduler.rs

        if ok {
-            ctx.latch_timer.take();
-            ctx.slow_timer = Some(SlowTimer::new());
+            entity.on_schedule();


Why call on_schedule in acquire_lock instead of schedule?

BusyJay · 2018-09-13T09:47:51Z

src/storage/txn/scheduler.rs

        } else {
-            execute_callback(cb, pr);
+            execute_callback(entity.cb, pr);


Why not call the callback inside thread pool?

BusyJay · 2018-09-13T09:53:23Z

src/storage/txn/scheduler.rs

-                );
-            }
+// TODO: Moves SchedEntity to Task once we adopt futures in the Engine trait.
+struct SchedEntity {


Entity doesn't seem to be a good name, it makes more sense that task itself is an entity as it contains the actual task data. How about naming it as TaskTracker just like RequestTracker in endpoint.rs?

zhangjinpeng87 · 2018-09-14T03:18:31Z

src/storage/txn/scheduler.rs

    }

-    fn dequeue_schedule_entity(&mut self, cid: u64) -> SchedEntity {
-        let entity = self.schedule_entities.remove(&cid).unwrap();
+    fn find_task_context(&mut self, cid: u64) -> TaskContext {


s/find_task_context/take_task_context

zhangjinpeng87

LGTM

Connor1996

LGTM

Signed-off-by: Neil Shen <overvenus@gmail.com>

zhangjinpeng87

LGTM

overvenus · 2018-09-14T07:08:34Z

/run-all-tests

zhangjinpeng87 · 2018-09-14T11:31:32Z

/run-all-tests

Connor1996

LGTM

Signed-off-by: Neil Shen <overvenus@gmail.com>

overvenus added S: WIP component/storage Component: Storage, Scheduler, etc. component/performance Component: Performance labels Sep 6, 2018

overvenus force-pushed the scheduler/reduce-msg branch from d844298 to 639701c Compare September 10, 2018 11:46

overvenus removed the S: WIP label Sep 11, 2018

overvenus force-pushed the scheduler/process branch from 71ae317 to 4a657df Compare September 11, 2018 05:30

overvenus force-pushed the scheduler/reduce-msg branch from 89d9178 to 77886c5 Compare September 11, 2018 06:15

overvenus force-pushed the scheduler/process branch from 4a657df to e7870a9 Compare September 11, 2018 06:17

overvenus force-pushed the scheduler/reduce-msg branch 2 times, most recently from f7b2d38 to 49cf999 Compare September 11, 2018 07:21

Connor1996 reviewed Sep 11, 2018

View reviewed changes

overvenus changed the title ~~storage: reduce scheduler msgs~~ [DNM]storage: reduce scheduler msgs Sep 11, 2018

Connor1996 reviewed Sep 11, 2018

View reviewed changes

zhangjinpeng87 reviewed Sep 12, 2018

View reviewed changes

zhangjinpeng87 reviewed Sep 13, 2018

View reviewed changes

Connor1996 reviewed Sep 13, 2018

View reviewed changes

BusyJay reviewed Sep 13, 2018

View reviewed changes

zhangjinpeng87 reviewed Sep 14, 2018

View reviewed changes

zhangjinpeng87 previously approved these changes Sep 14, 2018

View reviewed changes

Connor1996 previously approved these changes Sep 14, 2018

View reviewed changes

overvenus force-pushed the scheduler/process branch from e7870a9 to f9eb37f Compare September 14, 2018 05:19

overvenus changed the base branch from scheduler/process to master September 14, 2018 06:35

storage: reduce scheduler msgs

8d3235d

Signed-off-by: Neil Shen <overvenus@gmail.com>

overvenus dismissed stale reviews from Connor1996 and zhangjinpeng87 via 8d3235d September 14, 2018 06:38

overvenus force-pushed the scheduler/reduce-msg branch from 203ea3f to 8d3235d Compare September 14, 2018 06:38

overvenus removed the S: DNM label Sep 14, 2018

overvenus changed the title ~~[DNM]storage: reduce scheduler msgs~~ storage: reduce scheduler msgs Sep 14, 2018

zhangjinpeng87 approved these changes Sep 14, 2018

View reviewed changes

Merge branch 'master' into scheduler/reduce-msg

ce0bdf9

Connor1996 approved these changes Sep 14, 2018

View reviewed changes

overvenus merged commit 597b89c into master Sep 14, 2018

overvenus deleted the scheduler/reduce-msg branch September 14, 2018 12:04

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

storage: reduce scheduler msgs (tikv#3583)

d57eb52

Signed-off-by: Neil Shen <overvenus@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

storage: reduce scheduler msgs #3583

storage: reduce scheduler msgs #3583

overvenus commented Sep 6, 2018

Connor1996 Sep 11, 2018

Connor1996 Sep 11, 2018

Connor1996 Sep 11, 2018

Connor1996 Sep 11, 2018

zhangjinpeng87 Sep 12, 2018

zhangjinpeng87 Sep 12, 2018

zhangjinpeng87 Sep 12, 2018

zhangjinpeng87 Sep 12, 2018

overvenus commented Sep 12, 2018

zhangjinpeng87 Sep 13, 2018

zhangjinpeng87 Sep 13, 2018

zhangjinpeng87 Sep 13, 2018

zhangjinpeng87 Sep 13, 2018

Connor1996 Sep 13, 2018

BusyJay Sep 13, 2018

BusyJay Sep 13, 2018

overvenus Sep 13, 2018

BusyJay Sep 13, 2018

zhangjinpeng87 Sep 14, 2018

zhangjinpeng87 left a comment

Connor1996 left a comment

zhangjinpeng87 left a comment

overvenus commented Sep 14, 2018

zhangjinpeng87 commented Sep 14, 2018

Connor1996 left a comment


		Ok(())
		fn handle_schedule(scheduler: worker::Scheduler<Msg>, msg: Msg) -> bool {

storage: reduce scheduler msgs #3583

storage: reduce scheduler msgs #3583

Conversation

overvenus commented Sep 6, 2018

What have you changed? (mandatory)

What are the type of the changes? (mandatory)

How has this PR been tested? (mandatory)

Does this PR affect documentation (docs/docs-cn) update? (mandatory)

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

Refer to a related PR or issue link (optional)

Benchmark result if necessary (optional)

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

Choose a reason for hiding this comment

overvenus commented Sep 12, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhangjinpeng87 left a comment

Choose a reason for hiding this comment

Connor1996 left a comment

Choose a reason for hiding this comment

zhangjinpeng87 left a comment

Choose a reason for hiding this comment

overvenus commented Sep 14, 2018

zhangjinpeng87 commented Sep 14, 2018

Connor1996 left a comment

Choose a reason for hiding this comment