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

coprocesser: refactor thread pool to pass contexts #2152

Merged
merged 28 commits into from Aug 22, 2017

Conversation

Projects
None yet
4 participants
@choleraehyq

choleraehyq commented Aug 10, 2017

@siddontang

This comment has been minimized.

Show comment
Hide comment
@siddontang

siddontang Aug 10, 2017

Contributor

@choleraehyq

You should use two PRs, one to remove the unused queue, the other is for the new thread pool.

Contributor

siddontang commented Aug 10, 2017

@choleraehyq

You should use two PRs, one to remove the unused queue, the other is for the new thread pool.

@choleraehyq choleraehyq changed the title from coprocesser: refactor thread pool to pass contexts to [DNM] coprocesser: refactor thread pool to pass contexts Aug 10, 2017

@choleraehyq choleraehyq changed the title from [DNM] coprocesser: refactor thread pool to pass contexts to coprocesser: refactor thread pool to pass contexts Aug 11, 2017

@zhangjinpeng1987

This comment has been minimized.

Show comment
Hide comment
@zhangjinpeng1987
Member

zhangjinpeng1987 commented Aug 14, 2017

LGTM

@choleraehyq

This comment has been minimized.

Show comment
Hide comment
@choleraehyq

choleraehyq commented Aug 14, 2017

@choleraehyq

This comment has been minimized.

Show comment
Hide comment
@choleraehyq

choleraehyq Aug 15, 2017

We can put whatever we want in Context. And each worker has its own Context, we can update worker's Context in on_task_started and on_task_finished hooks.

@siddontang @zhangjinpeng1987 PTAL

choleraehyq commented Aug 15, 2017

We can put whatever we want in Context. And each worker has its own Context, we can update worker's Context in on_task_started and on_task_finished hooks.

@siddontang @zhangjinpeng1987 PTAL

Show outdated Hide outdated src/coprocessor/endpoint.rs Outdated
Show outdated Hide outdated src/util/threadpool.rs Outdated

Cholerae Hu added some commits Aug 10, 2017

Cholerae Hu
coprocesser: refactor thread pool to pass a context
Signed-off-by: Cholerae Hu <huyingqian@pingcap.com>
Cholerae Hu
threadpool: fix clippy
Signed-off-by: Cholerae Hu <huyingqian@pingcap.com>
Cholerae Hu
threadpool: change task job type
Signed-off-by: Cholerae Hu <huyingqian@pingcap.com>
Cholerae Hu
threadpool: add unittest for context
Signed-off-by: Cholerae Hu <huyingqian@pingcap.com>
Cholerae Hu
threadpool: address comment
Signed-off-by: Cholerae Hu <huyingqian@pingcap.com>
Cholerae Hu
*: address comment
Signed-off-by: Cholerae Hu <huyingqian@pingcap.com>
Cholerae Hu
threadpool: refactor context
Signed-off-by: Cholerae Hu <huyingqian@pingcap.com>
Cholerae Hu
threadpool: fix typo
Signed-off-by: Cholerae Hu <huyingqian@pingcap.com>
Cholerae Hu
threadpool: use mut reference in hooks
Signed-off-by: Cholerae Hu <huyingqian@pingcap.com>
Cholerae Hu
threadpool: add comments about rustc bug
Signed-off-by: Cholerae Hu <huyingqian@pingcap.com>
@zhangjinpeng1987

This comment has been minimized.

Show comment
Hide comment
@zhangjinpeng1987

zhangjinpeng1987 Aug 16, 2017

Member

Have you ever benched the new thread pool?

Member

zhangjinpeng1987 commented Aug 16, 2017

Have you ever benched the new thread pool?

Cholerae Hu added some commits Aug 16, 2017

Cholerae Hu
*: remove gid in task
Signed-off-by: Cholerae Hu <huyingqian@pingcap.com>
Cholerae Hu
threadpool: use crossbeam.MsQueue to refactore
Signed-off-by: Cholerae Hu <huyingqian@pingcap.com>

Cholerae Hu added some commits Aug 17, 2017

Cholerae Hu
threadpool: remove MsQueue and refactor
Signed-off-by: Cholerae Hu <huyingqian@pingcap.com>
Cholerae Hu
threadpool: remove unused var
Signed-off-by: Cholerae Hu <huyingqian@pingcap.com>
@choleraehyq

This comment has been minimized.

Show comment
Hide comment

choleraehyq commented Aug 17, 2017

task = self.get_next_task(Some(&t.gid));
while !self.stop_flag.load(AtomicOrdering::SeqCst) {
if let Some(t) = self.get_task_timeout(time::Duration::from_millis(WORKER_WAIT_TIME)) {
self.ctx.on_task_started();

This comment has been minimized.

@siddontang

siddontang Aug 17, 2017

Contributor

should check timeout even no task here.

@siddontang

siddontang Aug 17, 2017

Contributor

should check timeout even no task here.

This comment has been minimized.

@choleraehyq

choleraehyq Aug 18, 2017

What do you mean? If get_task_timeout return None, it must have been timeout.

@choleraehyq

choleraehyq Aug 18, 2017

What do you mean? If get_task_timeout return None, it must have been timeout.

This comment has been minimized.

@siddontang

siddontang Aug 18, 2017

Contributor

In our case, if we only do one task, when do we flush the local metric to the global metrics regularly? do we should do:

let lastTime = instant::now 
while {
    if task do 
    if now - lastTime > interval {
        ctx.on_interval()
    }
}
@siddontang

siddontang Aug 18, 2017

Contributor

In our case, if we only do one task, when do we flush the local metric to the global metrics regularly? do we should do:

let lastTime = instant::now 
while {
    if task do 
    if now - lastTime > interval {
        ctx.on_interval()
    }
}

This comment has been minimized.

@choleraehyq

choleraehyq Aug 18, 2017

Does it better to provide a on_tick and run it each loop rather than checking time and on_interval? I think time check logic should be implement by user in on_tick.

@choleraehyq

choleraehyq Aug 18, 2017

Does it better to provide a on_tick and run it each loop rather than checking time and on_interval? I think time check logic should be implement by user in on_tick.

This comment has been minimized.

@BusyJay

BusyJay Aug 20, 2017

Contributor

I don't think so. This is a very common usage, context should provide a more handy interface so the check logic doesn't have to be written again and again.

@BusyJay

BusyJay Aug 20, 2017

Contributor

I don't think so. This is a very common usage, context should provide a more handy interface so the check logic doesn't have to be written again and again.

This comment has been minimized.

@choleraehyq

choleraehyq Aug 21, 2017

Then how can user pass interval to the task pool? Should we add an extra interval argument when creating ThreadPool?

@choleraehyq

choleraehyq Aug 21, 2017

Then how can user pass interval to the task pool? Should we add an extra interval argument when creating ThreadPool?

This comment has been minimized.

@siddontang

siddontang Aug 21, 2017

Contributor

yes

@siddontang

siddontang Aug 21, 2017

Contributor

yes

Show outdated Hide outdated Cargo.toml Outdated
Show outdated Hide outdated src/util/threadpool.rs Outdated
Show outdated Hide outdated src/util/threadpool.rs Outdated

Cholerae Hu added some commits Aug 18, 2017

Cholerae Hu
threadpool: address comment
Signed-off-by: Cholerae Hu <huyingqian@pingcap.com>
Cholerae Hu
context: add on_tick
Signed-off-by: Cholerae Hu <huyingqian@pingcap.com>
@siddontang

This comment has been minimized.

Show comment
Hide comment
@siddontang
Contributor

siddontang commented Aug 18, 2017

Show outdated Hide outdated src/util/threadpool.rs Outdated
Show outdated Hide outdated src/util/threadpool.rs Outdated
task = self.get_next_task(Some(&t.gid));
while !self.stop_flag.load(AtomicOrdering::SeqCst) {
if let Some(t) = self.get_task_timeout(time::Duration::from_millis(WORKER_WAIT_TIME)) {
self.ctx.on_task_started();

This comment has been minimized.

@BusyJay

BusyJay Aug 20, 2017

Contributor

I don't think so. This is a very common usage, context should provide a more handy interface so the check logic doesn't have to be written again and again.

@BusyJay

BusyJay Aug 20, 2017

Contributor

I don't think so. This is a very common usage, context should provide a more handy interface so the check logic doesn't have to be written again and again.

Show outdated Hide outdated src/util/threadpool.rs Outdated
@siddontang

This comment has been minimized.

Show comment
Hide comment
@siddontang

siddontang Aug 21, 2017

Contributor

any update? @choleraehyq

Contributor

siddontang commented Aug 21, 2017

any update? @choleraehyq

@choleraehyq

This comment has been minimized.

Show comment
Hide comment
@choleraehyq

choleraehyq Aug 21, 2017

Dealing with expression. Will update later today.

choleraehyq commented Aug 21, 2017

Dealing with expression. Will update later today.

Cholerae Hu
threadpool: address comment
Signed-off-by: Cholerae Hu <huyingqian@pingcap.com>
Show outdated Hide outdated src/util/threadpool.rs Outdated
Show outdated Hide outdated src/util/threadpool.rs Outdated
Show outdated Hide outdated src/util/threadpool.rs Outdated
Show outdated Hide outdated src/util/threadpool.rs Outdated

Cholerae Hu added some commits Aug 21, 2017

Cholerae Hu
threadpool: address comment
Signed-off-by: Cholerae Hu <huyingqian@pingcap.com>
@BusyJay

This comment has been minimized.

Show comment
Hide comment
@BusyJay

BusyJay Aug 21, 2017

Contributor

LGTM

Contributor

BusyJay commented Aug 21, 2017

LGTM

Show outdated Hide outdated src/coprocessor/endpoint.rs Outdated
Cholerae Hu
threadpool: rename batch_size to tasks_per_tick
Signed-off-by: Cholerae Hu <huyingqian@pingcap.com>
@siddontang

LGTM

@siddontang siddontang merged commit 60952b1 into master Aug 22, 2017

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
license/cla Contributor License Agreement is signed.
Details

@siddontang siddontang deleted the hyq/threadpool branch Aug 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment