Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(41)

Issue 2692863012: SchedulerWorker Refcounting for Destruction in Production (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 months ago by robliao
Modified:
5 months, 3 weeks ago
Reviewers:
gab, fdoray
CC:
chromium-reviews, gab+watch_chromium.org, robliao+watch_chromium.org, fdoray+watch_chromium.org, vmpstr+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

SchedulerWorker Refcounting for Destruction in Production The upcoming work for one dedicated thread per SingleTaskTaskRunner requires the ability to clean up SchedulerWorker. Additionally dynamically ramping up and down SchedulerWorkers at runtime may also utilize this work. This change introduces refcounting and SchedulerWorker::RequestThreadTermination() so that a caller may safely release a SchedulerWorker at runtime. The expected usage is scoped_refptr<SchedulerWorker> worker_ = /* Existing Worker */ worker_->Cleanup(); worker_ = nullptr; Done! Reference Change: https://codereview.chromium.org/2686593003/ BUG=684080 Review-Url: https://codereview.chromium.org/2692863012 Cr-Commit-Position: refs/heads/master@{#452686} Committed: https://chromium.googlesource.com/chromium/src/+/bcc569081c8c449f98da7d1130bc2bcf5efecf7d

Patch Set 1 #

Total comments: 12

Patch Set 2 : CR Feedback #

Total comments: 8

Patch Set 3 : AtomicFlag #

Patch Set 4 : Fix Up Comments and Description #

Total comments: 28

Patch Set 5 : CR Feedback #

Patch Set 6 : CR Feedback #

Total comments: 6

Patch Set 7 : CR Feedback #

Patch Set 8 : Remove Last Vestiges of std::unique_ptr SchedulerWorker #

Unified diffs Side-by-side diffs Delta from patch set Stats (+435 lines, -135 lines) Patch
M base/task_scheduler/scheduler_worker.h View 1 2 3 4 5 chunks +39 lines, -14 lines 0 comments Download
M base/task_scheduler/scheduler_worker.cc View 1 2 3 4 5 6 15 chunks +65 lines, -32 lines 0 comments Download
M base/task_scheduler/scheduler_worker_pool_impl.h View 1 chunk +1 line, -1 line 0 comments Download
M base/task_scheduler/scheduler_worker_pool_impl.cc View 2 chunks +6 lines, -7 lines 0 comments Download
M base/task_scheduler/scheduler_worker_stack_unittest.cc View 1 chunk +3 lines, -3 lines 0 comments Download
M base/task_scheduler/scheduler_worker_unittest.cc View 1 2 3 4 5 6 7 8 chunks +321 lines, -78 lines 0 comments Download
Commit queue not available (can’t edit this change).

Dependent Patchsets:

Messages

Total messages: 65 (44 generated)
robliao
Here's what life looks like with refcounting. I decided to keep SchedulerWorker::Thread as is with ...
6 months ago (2017-02-17 06:10:12 UTC) #4
fdoray
https://codereview.chromium.org/2692863012/diff/1/base/task_scheduler/scheduler_worker.cc File base/task_scheduler/scheduler_worker.cc (right): https://codereview.chromium.org/2692863012/diff/1/base/task_scheduler/scheduler_worker.cc#newcode110 base/task_scheduler/scheduler_worker.cc:110: if (!detached_thread) Why is it important to take ownership ...
6 months ago (2017-02-17 16:43:41 UTC) #7
robliao
Thanks for the comments! https://codereview.chromium.org/2692863012/diff/1/base/task_scheduler/scheduler_worker.cc File base/task_scheduler/scheduler_worker.cc (right): https://codereview.chromium.org/2692863012/diff/1/base/task_scheduler/scheduler_worker.cc#newcode110 base/task_scheduler/scheduler_worker.cc:110: if (!detached_thread) On 2017/02/17 16:43:41, ...
6 months ago (2017-02-17 19:24:36 UTC) #9
fdoray
lgtm https://codereview.chromium.org/2692863012/diff/40001/base/task_scheduler/scheduler_worker.h File base/task_scheduler/scheduler_worker.h (right): https://codereview.chromium.org/2692863012/diff/40001/base/task_scheduler/scheduler_worker.h#newcode163 base/task_scheduler/scheduler_worker.h:163: bool should_exit_ = false; Ping change to AtomicFlag? ...
6 months ago (2017-02-17 20:28:04 UTC) #10
robliao
https://codereview.chromium.org/2692863012/diff/40001/base/task_scheduler/scheduler_worker.h File base/task_scheduler/scheduler_worker.h (right): https://codereview.chromium.org/2692863012/diff/40001/base/task_scheduler/scheduler_worker.h#newcode163 base/task_scheduler/scheduler_worker.h:163: bool should_exit_ = false; On 2017/02/17 20:28:04, fdoray wrote: ...
6 months ago (2017-02-17 20:38:58 UTC) #11
fdoray
On 2017/02/17 20:38:58, robliao wrote: > https://codereview.chromium.org/2692863012/diff/40001/base/task_scheduler/scheduler_worker.h > File base/task_scheduler/scheduler_worker.h (right): > > https://codereview.chromium.org/2692863012/diff/40001/base/task_scheduler/scheduler_worker.h#newcode163 > ...
6 months ago (2017-02-17 20:59:38 UTC) #12
robliao
> You have to acquire |thread_lock_| in SchedulerWorker::Cleanup() but not in ShouldExit() if you use ...
6 months ago (2017-02-17 21:33:41 UTC) #15
gab
So in this CL the refcount never goes above one still, right? (i.e. this will ...
6 months ago (2017-02-17 21:38:35 UTC) #18
robliao
> So in this CL the refcount never goes above one still, right? (i.e. this ...
6 months ago (2017-02-17 22:04:31 UTC) #22
gab
Took a fresh peak :). lvg, just a few things with tests https://codereview.chromium.org/2692863012/diff/160001/base/task_scheduler/scheduler_worker.cc File base/task_scheduler/scheduler_worker.cc ...
5 months, 3 weeks ago (2017-02-21 19:01:05 UTC) #29
gab
On 2017/02/21 19:01:05, gab wrote: > Took a fresh peak :). lvg, just a few ...
5 months, 3 weeks ago (2017-02-21 19:01:36 UTC) #30
robliao
Thanks for the comments! https://codereview.chromium.org/2692863012/diff/160001/base/task_scheduler/scheduler_worker.cc File base/task_scheduler/scheduler_worker.cc (left): https://codereview.chromium.org/2692863012/diff/160001/base/task_scheduler/scheduler_worker.cc#oldcode296 base/task_scheduler/scheduler_worker.cc:296: thread_ = Thread::Create(this); On 2017/02/21 ...
5 months, 3 weeks ago (2017-02-21 22:26:57 UTC) #33
gab
https://codereview.chromium.org/2692863012/diff/160001/base/task_scheduler/scheduler_worker.cc File base/task_scheduler/scheduler_worker.cc (right): https://codereview.chromium.org/2692863012/diff/160001/base/task_scheduler/scheduler_worker.cc#newcode111 base/task_scheduler/scheduler_worker.cc:111: // in Join(), we go ahead and grab the ...
5 months, 3 weeks ago (2017-02-22 18:02:24 UTC) #38
robliao
https://codereview.chromium.org/2692863012/diff/160001/base/task_scheduler/scheduler_worker.cc File base/task_scheduler/scheduler_worker.cc (right): https://codereview.chromium.org/2692863012/diff/160001/base/task_scheduler/scheduler_worker.cc#newcode111 base/task_scheduler/scheduler_worker.cc:111: // in Join(), we go ahead and grab the ...
5 months, 3 weeks ago (2017-02-22 20:43:40 UTC) #40
gab
lgtm w/ comments https://codereview.chromium.org/2692863012/diff/220001/base/task_scheduler/scheduler_worker.cc File base/task_scheduler/scheduler_worker.cc (right): https://codereview.chromium.org/2692863012/diff/220001/base/task_scheduler/scheduler_worker.cc#newcode118 base/task_scheduler/scheduler_worker.cc:118: // * Join: DetachThreadObject returns nullptr. ...
5 months, 3 weeks ago (2017-02-23 19:25:24 UTC) #49
robliao
https://codereview.chromium.org/2692863012/diff/220001/base/task_scheduler/scheduler_worker.cc File base/task_scheduler/scheduler_worker.cc (right): https://codereview.chromium.org/2692863012/diff/220001/base/task_scheduler/scheduler_worker.cc#newcode118 base/task_scheduler/scheduler_worker.cc:118: // * Join: DetachThreadObject returns nullptr. Join cleans up. ...
5 months, 3 weeks ago (2017-02-23 21:04:22 UTC) #51
gab
https://codereview.chromium.org/2692863012/diff/220001/base/task_scheduler/scheduler_worker_unittest.cc File base/task_scheduler/scheduler_worker_unittest.cc (right): https://codereview.chromium.org/2692863012/diff/220001/base/task_scheduler/scheduler_worker_unittest.cc#newcode662 base/task_scheduler/scheduler_worker_unittest.cc:662: TEST(TaskSchedulerWorkerTest, WorkerDetachesAndSelfDestroysDuringJoin) { As discussed offline, need to tweak ...
5 months, 3 weeks ago (2017-02-23 21:52:54 UTC) #53
robliao
https://codereview.chromium.org/2692863012/diff/220001/base/task_scheduler/scheduler_worker_unittest.cc File base/task_scheduler/scheduler_worker_unittest.cc (right): https://codereview.chromium.org/2692863012/diff/220001/base/task_scheduler/scheduler_worker_unittest.cc#newcode662 base/task_scheduler/scheduler_worker_unittest.cc:662: TEST(TaskSchedulerWorkerTest, WorkerDetachesAndSelfDestroysDuringJoin) { On 2017/02/23 21:52:54, gab wrote: > ...
5 months, 3 weeks ago (2017-02-23 22:00:03 UTC) #54
gab
lgtm++! On 2017/02/23 22:00:03, robliao wrote: > https://codereview.chromium.org/2692863012/diff/220001/base/task_scheduler/scheduler_worker_unittest.cc > File base/task_scheduler/scheduler_worker_unittest.cc (right): > > https://codereview.chromium.org/2692863012/diff/220001/base/task_scheduler/scheduler_worker_unittest.cc#newcode662 ...
5 months, 3 weeks ago (2017-02-23 22:03:04 UTC) #57
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2692863012/260001
5 months, 3 weeks ago (2017-02-24 00:16:01 UTC) #62
commit-bot: I haz the power
5 months, 3 weeks ago (2017-02-24 00:21:51 UTC) #65
Message was sent while issue was closed.
Committed patchset #8 (id:260001) as
https://chromium.googlesource.com/chromium/src/+/bcc569081c8c449f98da7d1130bc...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld b40b6558b