Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(12)

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

Created:
3 years, 10 months ago by robliao
Modified:
3 years, 10 months 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

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 ...
3 years, 10 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 ...
3 years, 10 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, ...
3 years, 10 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? ...
3 years, 10 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: ...
3 years, 10 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 > ...
3 years, 10 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 ...
3 years, 10 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 ...
3 years, 10 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 ...
3 years, 10 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 ...
3 years, 10 months 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 ...
3 years, 10 months 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 ...
3 years, 10 months 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 ...
3 years, 10 months 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 ...
3 years, 10 months 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. ...
3 years, 10 months 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. ...
3 years, 10 months 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 ...
3 years, 10 months 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: > ...
3 years, 10 months 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 ...
3 years, 10 months 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
3 years, 10 months ago (2017-02-24 00:16:01 UTC) #62
commit-bot: I haz the power
3 years, 10 months 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...

Powered by Google App Engine
This is Rietveld 408576698