Chromium Code Reviews
Help | Chromium Project | Sign in
(16)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 week, 5 days ago by robliao (PST plus 17h)
Modified:
5 days, 17 hours ago
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 (PST plus 17h)
Here's what life looks like with refcounting. I decided to keep SchedulerWorker::Thread as is with ...
1 week, 5 days 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 ...
1 week, 5 days ago (2017-02-17 16:43:41 UTC) #7
robliao (PST plus 17h)
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, ...
1 week, 4 days 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? ...
1 week, 4 days ago (2017-02-17 20:28:04 UTC) #10
robliao (PST plus 17h)
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: ...
1 week, 4 days 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 > ...
1 week, 4 days ago (2017-02-17 20:59:38 UTC) #12
robliao (PST plus 17h)
> You have to acquire |thread_lock_| in SchedulerWorker::Cleanup() but not in ShouldExit() if you use ...
1 week, 4 days ago (2017-02-17 21:33:41 UTC) #15
gab (slow - travel and perf)
So in this CL the refcount never goes above one still, right? (i.e. this will ...
1 week, 4 days ago (2017-02-17 21:38:35 UTC) #18
robliao (PST plus 17h)
> So in this CL the refcount never goes above one still, right? (i.e. this ...
1 week, 4 days ago (2017-02-17 22:04:31 UTC) #22
gab (slow - travel and perf)
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 ...
1 week ago (2017-02-21 19:01:05 UTC) #29
gab (slow - travel and perf)
On 2017/02/21 19:01:05, gab wrote: > Took a fresh peak :). lvg, just a few ...
1 week ago (2017-02-21 19:01:36 UTC) #30
robliao (PST plus 17h)
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 ...
1 week ago (2017-02-21 22:26:57 UTC) #33
gab (slow - travel and perf)
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 ...
6 days, 23 hours ago (2017-02-22 18:02:24 UTC) #38
robliao (PST plus 17h)
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 ...
6 days, 21 hours ago (2017-02-22 20:43:40 UTC) #40
gab (slow - travel and perf)
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 days, 22 hours ago (2017-02-23 19:25:24 UTC) #49
robliao (PST plus 17h)
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 days, 20 hours ago (2017-02-23 21:04:22 UTC) #51
gab (slow - travel and perf)
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 days, 19 hours ago (2017-02-23 21:52:54 UTC) #53
robliao (PST plus 17h)
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 days, 19 hours ago (2017-02-23 22:00:03 UTC) #54
gab (slow - travel and perf)
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 days, 19 hours 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 days, 17 hours ago (2017-02-24 00:16:01 UTC) #62
commit-bot: I haz the power
5 days, 17 hours 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 f8e48bd