Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2044023003:
Virtualize The Existence of a Scheduler Worker Thread (Closed)
|
Created:
4 years, 6 months ago by robliao Modified:
4 years, 6 months ago CC:
chromium-reviews, gab+watch_chromium.org, robliao+watch_chromium.org, fdoray+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@detach Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionVirtualize The Existence of a Scheduler Worker Thread
SchedulerWorkers are expected to always remain alive in
SchedulerWorkerPoolImpl. At the same time, we also want threads to sleep. This
change disconnects the SchedulerWorker's lifetime from the thread's
lifetime and allows the SchedulerWorkerPoolImpl to suggest when a worker can
detach the associated thread.
BUG=553459
Committed: https://crrev.com/86b90282101ddae90c07065d3edb612b9f0a972b
Cr-Commit-Position: refs/heads/master@{#401441}
Patch Set 1 : #
Total comments: 40
Patch Set 2 : CR Feedback #
Total comments: 41
Patch Set 3 : CR Feedback gab@ #
Total comments: 30
Patch Set 4 : CR Feedback gab@ fdoray@ #Patch Set 5 : Apply Renames #
Total comments: 14
Patch Set 6 : CR Feedback fdoray@ #
Depends on Patchset: Messages
Total messages: 42 (20 generated)
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
robliao@chromium.org changed reviewers: + fdoray@chromium.org, gab@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2044023003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2044023003/diff/20001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_thread.cc (right): https://codereview.chromium.org/2044023003/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread.cc:17: class SchedulerWorkerThread::Worker : public PlatformThread::Delegate { s/::Worker/::Thread/ ? https://codereview.chromium.org/2044023003/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread.cc:24: worker->Initialize(thread_priority); Could read priority from |outer|. https://codereview.chromium.org/2044023003/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread.cc:38: wake_up_event_.Wait(); GetSleepTimeout()? Otherwise, a thread that is never woken up will never detach. https://codereview.chromium.org/2044023003/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread.cc:40: while (outer_ && !outer_->task_tracker_->shutdown_completed() && How can |outer_| be nullptr here? You "break;" right after setting is to nullptr. DCHECK(outer_) inside the loop? https://codereview.chromium.org/2044023003/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread.cc:50: outer_ = nullptr; Is there a reason why this has to be set to nullptr? I would change SchedulerWorkerThread* outer_; to SchedulerWorkerThread* const outer_; https://codereview.chromium.org/2044023003/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread.cc:54: TimeDelta sleep_time = outer_->delegate_->GetSleepTimeout(); const TimeDelta sleep_time = https://codereview.chromium.org/2044023003/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread.cc:85: DCHECK(!detached_worker || !wake_up_event_.IsSignaled()) << !IsWakeUpPending() https://codereview.chromium.org/2044023003/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread.cc:93: bool IsWakeUpPending() { return wake_up_event_.IsSignaled(); } IsSignaled() resets the event... https://cs.chromium.org/chromium/src/base/synchronization/waitable_event.h?q=... https://codereview.chromium.org/2044023003/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread.cc:167: return !!worker_; AutoSchedulerLock auto_lock(worker_lock_); https://codereview.chromium.org/2044023003/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread.cc:180: std::unique_ptr<SchedulerWorkerThread::Worker> SchedulerWorkerThread::Detach() { DCHECK(!ShouldExitForTesting()); before acquiring |worker_lock_| https://codereview.chromium.org/2044023003/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread.cc:184: // could have woken us up. It should be the responsibility of the delegate to prevent the thread from detaching if single-threaded task runners are associated with the SchedulerWorkerThread. The real reason why we don't want to detach is to avoid breaking the guarantee that GetWork() is always called after a call to WakeUp(). https://codereview.chromium.org/2044023003/diff/20001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_thread.h (right): https://codereview.chromium.org/2044023003/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread.h:25: // A thread that runs Tasks from Sequences returned by a delegate. s/SchedulerWorkerThread/SchedulerWorker/ ? This is no longer a thread. It's a "worker" that runs tasks asynchronously, one at a time, not necessarily on the same thread. https://codereview.chromium.org/2044023003/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread.h:42: // Called by |worker_thread| when it enters its main function. Comment that this is called every time a "real" thread is created. A call to WakeUp() after the thread was detached (or hibernated) triggers a call to this method. https://codereview.chromium.org/2044023003/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread.h:58: // Called by |worker_thread| to see if it is allowed to detach. s/CanDetach/CanHibernate/ ? I think those who implement this method should ask themselves "can this worker enter a state in which it consumes less resources but takes longer to respond to a WakeUp()?" rather "can the thread handle be released?" (implementation detail). Add comments about the side-effects of returning true: - Future Tasks may run on a different thread than Tasks that already ran. - The next WakeUp() will be more costly. - The worker will free resources. https://codereview.chromium.org/2044023003/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread.h:59: virtual bool CanDetach(SchedulerWorkerThread* worker_thread) = 0; This is called after GetWork() returns nullptr. https://codereview.chromium.org/2044023003/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread.h:62: enum class InitialWorkerState { ALIVE, DETACHED }; ALIVE, HIBERNATING https://codereview.chromium.org/2044023003/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread.h:72: InitialWorkerState worker_state); I would remove this argument and always start HIBERNATING. Advantages: - WakeUp() is the only method from which Worker is instantiated. - Simpler and less confusing interface (If WakeUp() needs to be called before the SchedulerWorkerThread runs tasks, what are the benefits of creating it ALIVE?) https://codereview.chromium.org/2044023003/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread.h:81: // returned by the GetWork() method of its delegate until it returns nullptr. May fail if the thread was hibernating. https://codereview.chromium.org/2044023003/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread.h:90: // Whether is worker is alive. // Returns true if the worker is alive. + const
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
Thanks! https://codereview.chromium.org/2044023003/diff/20001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_thread.cc (right): https://codereview.chromium.org/2044023003/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread.cc:17: class SchedulerWorkerThread::Worker : public PlatformThread::Delegate { On 2016/06/08 17:51:48, fdoray wrote: > s/::Worker/::Thread/ ? I wanted to avoid SchedulerWorkerThread::Thread. If you folks are okay with Thread, I'm happy to rename this. https://codereview.chromium.org/2044023003/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread.cc:24: worker->Initialize(thread_priority); On 2016/06/08 17:51:48, fdoray wrote: > Could read priority from |outer|. Indeed! Done. https://codereview.chromium.org/2044023003/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread.cc:38: wake_up_event_.Wait(); On 2016/06/08 17:51:48, fdoray wrote: > GetSleepTimeout()? Otherwise, a thread that is never woken up will never detach. Done. Huh, interesting. This actually means that GetSleepTimeout() is part of the initial run instead of not idle. Seems okay now, which impacts unit tests. We should probably reconsider putting DCHECKs in delegates if we don't actually care about the call ordering. (SchedulerWorkerThreadStackUnittest had to be updated for this). https://codereview.chromium.org/2044023003/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread.cc:40: while (outer_ && !outer_->task_tracker_->shutdown_completed() && On 2016/06/08 17:51:48, fdoray wrote: > How can |outer_| be nullptr here? You "break;" right after setting is to > nullptr. > DCHECK(outer_) inside the loop? Indeed. I left this over from an earlier revision w/o the break. https://codereview.chromium.org/2044023003/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread.cc:50: outer_ = nullptr; On 2016/06/08 17:51:48, fdoray wrote: > Is there a reason why this has to be set to nullptr? > > I would change > SchedulerWorkerThread* outer_; to > SchedulerWorkerThread* const outer_; This ensures that two workers aren't talking to a SchedulerWorkerThread at the same time. https://codereview.chromium.org/2044023003/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread.cc:54: TimeDelta sleep_time = outer_->delegate_->GetSleepTimeout(); On 2016/06/08 17:51:48, fdoray wrote: > const TimeDelta sleep_time = Done. https://codereview.chromium.org/2044023003/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread.cc:85: DCHECK(!detached_worker || !wake_up_event_.IsSignaled()) << On 2016/06/08 17:51:48, fdoray wrote: > !IsWakeUpPending() Done. https://codereview.chromium.org/2044023003/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread.cc:93: bool IsWakeUpPending() { return wake_up_event_.IsSignaled(); } On 2016/06/08 17:51:48, fdoray wrote: > IsSignaled() resets the event... > https://cs.chromium.org/chromium/src/base/synchronization/waitable_event.h?q=... Nice catch. That certainly seems unexpected. Changed to manual and we reset on wake. https://codereview.chromium.org/2044023003/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread.cc:167: return !!worker_; On 2016/06/08 17:51:48, fdoray wrote: > AutoSchedulerLock auto_lock(worker_lock_); Done. https://codereview.chromium.org/2044023003/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread.cc:180: std::unique_ptr<SchedulerWorkerThread::Worker> SchedulerWorkerThread::Detach() { On 2016/06/08 17:51:48, fdoray wrote: > DCHECK(!ShouldExitForTesting()); before acquiring |worker_lock_| Done. https://codereview.chromium.org/2044023003/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread.cc:184: // could have woken us up. On 2016/06/08 17:51:48, fdoray wrote: > It should be the responsibility of the delegate to prevent the thread from > detaching if single-threaded task runners are associated with the > SchedulerWorkerThread. The real reason why we don't want to detach is to avoid > breaking the guarantee that GetWork() is always called after a call to WakeUp(). Added a note about single-threaded work to CanDetach and updated this comment. This can also happen if the thread is idle and suddenly gets scheduled to run work. https://codereview.chromium.org/2044023003/diff/20001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_thread.h (right): https://codereview.chromium.org/2044023003/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread.h:25: // A thread that runs Tasks from Sequences returned by a delegate. On 2016/06/08 17:51:48, fdoray wrote: > s/SchedulerWorkerThread/SchedulerWorker/ ? > This is no longer a thread. It's a "worker" that runs tasks asynchronously, one > at a time, not necessarily on the same thread. This comes down to if the thread should be virtual to the thread pool or not. I took the stance that as far as the threadpool is concerned, these are threads. How they are managed underneath the hood is up to the implementation. I'll give you that it may not necessarily correspond to the same PlatformThread, and I'll call that out here. https://codereview.chromium.org/2044023003/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread.h:42: // Called by |worker_thread| when it enters its main function. On 2016/06/08 17:51:48, fdoray wrote: > Comment that this is called every time a "real" thread is created. A call to > WakeUp() after the thread was detached (or hibernated) triggers a call to this > method. Updated comment. https://codereview.chromium.org/2044023003/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread.h:58: // Called by |worker_thread| to see if it is allowed to detach. On 2016/06/08 17:51:48, fdoray wrote: > s/CanDetach/CanHibernate/ ? I think those who implement this method should ask > themselves "can this worker enter a state in which it consumes less resources > but takes longer to respond to a WakeUp()?" rather "can the thread handle be > released?" (implementation detail). > > Add comments about the side-effects of returning true: > - Future Tasks may run on a different thread than Tasks that already ran. > - The next WakeUp() will be more costly. > - The worker will free resources. Hibernation in the natural sense doesn't actually release resources. It just shuffles them around or consumes them more slowly. We're actually releasing resources when we let go of the thread. Added the bullet points about detachment. https://codereview.chromium.org/2044023003/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread.h:59: virtual bool CanDetach(SchedulerWorkerThread* worker_thread) = 0; On 2016/06/08 17:51:48, fdoray wrote: > This is called after GetWork() returns nullptr. Updated loosing the ordering constraint. https://codereview.chromium.org/2044023003/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread.h:62: enum class InitialWorkerState { ALIVE, DETACHED }; On 2016/06/08 17:51:49, fdoray wrote: > ALIVE, HIBERNATING See discussion about hibernation vs detachment. https://codereview.chromium.org/2044023003/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread.h:72: InitialWorkerState worker_state); On 2016/06/08 17:51:49, fdoray wrote: > I would remove this argument and always start HIBERNATING. > > Advantages: > - WakeUp() is the only method from which Worker is instantiated. > - Simpler and less confusing interface (If WakeUp() needs to be called before > the SchedulerWorkerThread runs tasks, what are the benefits of creating it > ALIVE?) The main benefit here is that it allows the SchedulerThreadPool to ensure that at least one thread is good to go during create. At the moment, a SchedulerThreadPool does not have the ability to determine if a real thread is backing the SchedulerWorkerThread or not. Updated comment. https://codereview.chromium.org/2044023003/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread.h:81: // returned by the GetWork() method of its delegate until it returns nullptr. On 2016/06/08 17:51:49, fdoray wrote: > May fail if the thread was hibernating. Updated comment. https://codereview.chromium.org/2044023003/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread.h:90: // Whether is worker is alive. On 2016/06/08 17:51:48, fdoray wrote: > // Returns true if the worker is alive. > + const I think I was falling into java style habits with these names. Changed to return true.
lg :-) https://codereview.chromium.org/2044023003/diff/20001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_thread.cc (right): https://codereview.chromium.org/2044023003/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread.cc:17: class SchedulerWorkerThread::Worker : public PlatformThread::Delegate { On 2016/06/08 19:00:08, robliao wrote: > On 2016/06/08 17:51:48, fdoray wrote: > > s/::Worker/::Thread/ ? > > I wanted to avoid SchedulerWorkerThread::Thread. If you folks are okay with > Thread, I'm happy to rename this. Or we could have SchedulerWorker and SchedulerWorker::Thread since the former is no longer a thread per se? https://codereview.chromium.org/2044023003/diff/100001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker_thread.cc (right): https://codereview.chromium.org/2044023003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.cc:65: // andthere are live references to it, it will be enqueued when a Task is "and there" https://codereview.chromium.org/2044023003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.cc:79: "This thread was detached and woken up at the same time."; Is this bad (i.e. someone did something wrong) or is it unexpected (i.e. this component is written in a way that this shouldn't be possible)? Clarify in comment. https://codereview.chromium.org/2044023003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.cc:113: wake_up_event_.Reset(); Why not keep this event AUTOMATIC if you're going to manually Reset() it post wait anyways? https://codereview.chromium.org/2044023003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.cc:134: // Creation is single-threaded, so no synchronization is necessary. I think what you mean is: // Creation happens before any other thread can reference this one, so no synchronization is necessary. ? https://codereview.chromium.org/2044023003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.cc:146: DCHECK(ShouldExitForTesting() || !worker_); So I thought we said the SWT object itself would always remain alive? But I also remember we talked about a case where we might want to delete it. I forget, can you refresh my memory on why this is desired (and perhaps add a comment so it sticks?) https://codereview.chromium.org/2044023003/diff/100001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker_thread.h (right): https://codereview.chromium.org/2044023003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.h:20: rm empty line https://codereview.chromium.org/2044023003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.h:34: // The worker is free to release and reallocate thread suggestions with guidance "thread suggestions"? https://codereview.chromium.org/2044023003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.h:65: // A SchedulerWorkerThread is free to ignore the request to detach. Which "request to detach"? Is a return true here a "request to detach"? https://codereview.chromium.org/2044023003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.h:71: // - Future tasks may run on a different PlatformThread after detachment. I don't think this is worth mentioning (i.e. it's implicit that shared work will be picked up by other threads regardless of what this thread does) https://codereview.chromium.org/2044023003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.h:77: enum class InitialWorkerState { ALIVE, DETACHED }; "InitialState" is good enough IMO, "Worker" is redundant per this being nested in "SchedulerWorkerThread" https://codereview.chromium.org/2044023003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.h:81: // handle shutdown behavior of Tasks. If |worker_state| is DETACHED, the Is the idea that we would create |max_threads| SWTs with most in DETACHED InitialState? https://codereview.chromium.org/2044023003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.h:83: // underlying platform thread fails during Create.. s/.././ https://codereview.chromium.org/2044023003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.h:98: // May fail if the worker is detached. Document what failure means since this is a void return. https://codereview.chromium.org/2044023003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.h:105: void JoinForTesting(); Document that Detach() must always return false while this call is in progress. https://codereview.chromium.org/2044023003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.h:117: // Returns the worker for ownership by the worker thread if the detach was "for ownership by" => I don't understand this sentence https://codereview.chromium.org/2044023003/diff/100001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker_thread_unittest.cc (right): https://codereview.chromium.org/2044023003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread_unittest.cc:311: scoped_refptr<Sequence> GetWork(SchedulerWorkerThread* worker_thread) Document what this does. https://codereview.chromium.org/2044023003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread_unittest.cc:327: "GetWork() never returns a sequence so there's nothing to reenqueue."; That's not true : it does return a sequence but it has only one task and thus doesn't need to be reenqueued either. https://codereview.chromium.org/2044023003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread_unittest.cc:357: bool can_detach_; = false; for 2 lines above instead of in constructor (I'm a fan of C++11 class-member initializers for POD types -- it's so easy otherwise to add a bool and forget to add it to the constructor) https://codereview.chromium.org/2044023003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread_unittest.cc:400: delegate->set_can_detach(false); I'd expected ResetAll() to also reset "detach" to default (i.e. false)
Description was changed from ========== Virtualize The Existence of a SchedulerWorkerThread SchedulerWorkerThreads are expected to always remain alive in SchedulerThreadPoolImpl. At the same time, we also want threads to sleep. This change disconnects the SchedulerWorkerThread lifetime from the worker's lifetime and allows the SchedulerThreadPoolImpl to suggest when a worker can detach the associated worker. BUG=553459 ========== to ========== Virtualize The Existence of a SchedulerWorkerThread SchedulerWorkerThreads are expected to always remain alive in SchedulerThreadPoolImpl. At the same time, we also want threads to sleep. This change disconnects the SchedulerWorkerThread lifetime from the worker's lifetime and allows the SchedulerThreadPoolImpl to suggest when a worker can detach the associated worker. BUG=553459 ==========
Patchset #3 (id:120001) has been deleted
https://codereview.chromium.org/2044023003/diff/20001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_thread.cc (right): https://codereview.chromium.org/2044023003/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread.cc:17: class SchedulerWorkerThread::Worker : public PlatformThread::Delegate { On 2016/06/10 16:15:20, gab wrote: > On 2016/06/08 19:00:08, robliao wrote: > > On 2016/06/08 17:51:48, fdoray wrote: > > > s/::Worker/::Thread/ ? > > > > I wanted to avoid SchedulerWorkerThread::Thread. If you folks are okay with > > Thread, I'm happy to rename this. > > Or we could have SchedulerWorker and SchedulerWorker::Thread since the former is > no longer a thread per se? Happy to perform the rename. I'll do that as the very last thing to keep the comment train preserved. https://codereview.chromium.org/2044023003/diff/100001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker_thread.cc (right): https://codereview.chromium.org/2044023003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.cc:65: // andthere are live references to it, it will be enqueued when a Task is On 2016/06/10 16:15:20, gab wrote: > "and there" Done. https://codereview.chromium.org/2044023003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.cc:79: "This thread was detached and woken up at the same time."; On 2016/06/10 16:15:20, gab wrote: > Is this bad (i.e. someone did something wrong) or is it unexpected (i.e. this > component is written in a way that this shouldn't be possible)? Clarify in > comment. Yep, very bad. If the thread is signaled after the detach succeeds, that means that means SchedulerWorkerThread was able to call WakeUp on on the worker, which should not be possible. This means a WakeUp will not be serviced. Added a comment. https://codereview.chromium.org/2044023003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.cc:113: wake_up_event_.Reset(); On 2016/06/10 16:15:20, gab wrote: > Why not keep this event AUTOMATIC if you're going to manually Reset() it post > wait anyways? IsSignaled apparently resets the WaitableEvent! It was news to me too! https://codereview.chromium.org/2044023003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.cc:134: // Creation is single-threaded, so no synchronization is necessary. On 2016/06/10 16:15:20, gab wrote: > I think what you mean is: > > // Creation happens before any other thread can reference this one, so no > synchronization is necessary. > > ? Yep, exactly what I mean. I was going for the one-liner comment approach. Updated. https://codereview.chromium.org/2044023003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.cc:146: DCHECK(ShouldExitForTesting() || !worker_); On 2016/06/10 16:15:20, gab wrote: > So I thought we said the SWT object itself would always remain alive? But I also > remember we talked about a case where we might want to delete it. I forget, can > you refresh my memory on why this is desired (and perhaps add a comment so it > sticks?) I don't recall a case where we would want the SchedulerWorkerThread to go away. SchedulerThreadPoolImpl has a "const" worker thread list, so it always sticks around _except_ during tests where we tear the SchedulerThreadPoolImpl down. In fact, if we null out worker_ on Join, we can just assert for the thread, which is probably what we want anyways. Added a comment and updated the DCHECK. https://codereview.chromium.org/2044023003/diff/100001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker_thread.h (right): https://codereview.chromium.org/2044023003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.h:20: On 2016/06/10 16:15:21, gab wrote: > rm empty line Done. https://codereview.chromium.org/2044023003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.h:34: // The worker is free to release and reallocate thread suggestions with guidance On 2016/06/10 16:15:20, gab wrote: > "thread suggestions"? Fixed! https://codereview.chromium.org/2044023003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.h:65: // A SchedulerWorkerThread is free to ignore the request to detach. On 2016/06/10 16:15:21, gab wrote: > Which "request to detach"? Is a return true here a "request to detach"? Clarified. Today, it is. https://codereview.chromium.org/2044023003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.h:71: // - Future tasks may run on a different PlatformThread after detachment. On 2016/06/10 16:15:21, gab wrote: > I don't think this is worth mentioning (i.e. it's implicit that shared work will > be picked up by other threads regardless of what this thread does) sgtm, since threads are indeed virtualized now. https://codereview.chromium.org/2044023003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.h:77: enum class InitialWorkerState { ALIVE, DETACHED }; On 2016/06/10 16:15:21, gab wrote: > "InitialState" is good enough IMO, "Worker" is redundant per this being nested > in "SchedulerWorkerThread" sgtm. Done. https://codereview.chromium.org/2044023003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.h:81: // handle shutdown behavior of Tasks. If |worker_state| is DETACHED, the On 2016/06/10 16:15:21, gab wrote: > Is the idea that we would create |max_threads| SWTs with most in DETACHED > InitialState? Yup. The new CL I have only creates one in ALIVE mode, the rest are in detached. https://codereview.chromium.org/2044023003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.h:83: // underlying platform thread fails during Create.. On 2016/06/10 16:15:21, gab wrote: > s/.././ Done. https://codereview.chromium.org/2044023003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.h:98: // May fail if the worker is detached. On 2016/06/10 16:15:21, gab wrote: > Document what failure means since this is a void return. Done. https://codereview.chromium.org/2044023003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.h:105: void JoinForTesting(); On 2016/06/10 16:15:21, gab wrote: > Document that Detach() must always return false while this call is in progress. Doc'ed in Delegate::CanDetach(). https://codereview.chromium.org/2044023003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.h:117: // Returns the worker for ownership by the worker thread if the detach was On 2016/06/10 16:15:21, gab wrote: > "for ownership by" => I don't understand this sentence Clarified. https://codereview.chromium.org/2044023003/diff/100001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker_thread_unittest.cc (right): https://codereview.chromium.org/2044023003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread_unittest.cc:311: scoped_refptr<Sequence> GetWork(SchedulerWorkerThread* worker_thread) On 2016/06/10 16:15:21, gab wrote: > Document what this does. Done. https://codereview.chromium.org/2044023003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread_unittest.cc:327: "GetWork() never returns a sequence so there's nothing to reenqueue."; On 2016/06/10 16:15:21, gab wrote: > That's not true : it does return a sequence but it has only one task and thus > doesn't need to be reenqueued either. Indeed! Updated this copy+paste line. https://codereview.chromium.org/2044023003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread_unittest.cc:357: bool can_detach_; On 2016/06/10 16:15:21, gab wrote: > = false; for 2 lines above instead of in constructor > > (I'm a fan of C++11 class-member initializers for POD types -- it's so easy > otherwise to add a bool and forget to add it to the constructor) Done. It's too bad we can't do the same for non-POD types. https://codereview.chromium.org/2044023003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread_unittest.cc:400: delegate->set_can_detach(false); On 2016/06/10 16:15:21, gab wrote: > I'd expected ResetAll() to also reset "detach" to default (i.e. false) This used to be ResetAllAndDisallowDetach, but that felt wrong and overloaded, so I separated the two since detachment is a characteristic of the object. I changed ResetAll to ResetState to make it more clear.
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2044023003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Mostly lgtm % comments and SchedulerWorker::Thread rename (if Francois is on board). https://codereview.chromium.org/2044023003/diff/100001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker_thread.cc (right): https://codereview.chromium.org/2044023003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.cc:146: DCHECK(ShouldExitForTesting() || !worker_); Feels we want to keep the ShouldExitForTesting() DCHECK as well though? Shouldn't it be DCHECK(ShouldExitForTesting() && !worker_) in fact? https://codereview.chromium.org/2044023003/diff/140001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker_thread.cc (right): https://codereview.chromium.org/2044023003/diff/140001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.cc:78: // If a wake up is pending and we successfully detached, somehow outer_ was |outer_| x2 https://codereview.chromium.org/2044023003/diff/140001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.cc:151: // It is unexpected for worker_ to be alive and for SchedulerWorkerThread to |worker_| https://codereview.chromium.org/2044023003/diff/140001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker_thread.h (right): https://codereview.chromium.org/2044023003/diff/140001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.h:71: // When true is returned... s/.../:/ https://codereview.chromium.org/2044023003/diff/140001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.h:73: // - The worker will free resources if it can. The comment further above initially says "reserves the right to ignore" and here it says "will free if it can" https://codereview.chromium.org/2044023003/diff/140001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.h:74: // False must be returned if SchedulerWorkerThread::JoinForTesting is in s/False must be returned/This *must* return false if SWT::JoinForTesting() is in progress/ https://codereview.chromium.org/2044023003/diff/140001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.h:100: // May fail if the worker is detached. On failure the worker will not be able "the worker will not be able to call" : unclear for a reader IMO whether this means the GetWork() call will fail or that the worker will be incapacitated and not able to call it himself. I still find the last two sentences confusing. https://codereview.chromium.org/2044023003/diff/140001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker_thread_unittest.cc (right): https://codereview.chromium.org/2044023003/diff/140001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread_unittest.cc:311: // Sends one item of work to signal work_processed_. On subsequent calls, |work_processed_| https://codereview.chromium.org/2044023003/diff/140001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread_unittest.cc:326: NOTREACHED() << Actually, in tests, prefer ADD_FAILURE() to NOTREACHED().
https://codereview.chromium.org/2044023003/diff/140001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker_thread.cc (right): https://codereview.chromium.org/2044023003/diff/140001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.cc:101: const size_t kDefaultStackSize = 0; constexpr https://codereview.chromium.org/2044023003/diff/140001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.cc:124: // Event signaled to wake up this SchedulerWorkerThread. this Thread (or whatever the name of this class is) https://codereview.chromium.org/2044023003/diff/140001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker_thread.h (right): https://codereview.chromium.org/2044023003/diff/140001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.h:24: // A worker that manages at most one thread to run Tasks from Sequences returned one thread at a time? https://codereview.chromium.org/2044023003/diff/140001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.h:63: // call to GetWork() returned nullptr. No new line? https://codereview.chromium.org/2044023003/diff/140001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.h:65: // this function. How will the thread pool keep one active (not detached) idle thread at all time? Could we change this comment to "From the time this returns true to the time OnMainEntry() is called, the worker is in a detached state." In addition to the idle stack, the thread pool would have a detached stack. CanDetach() would return false if idle_stack_.size() <= 1. Before returning true, CanDetach() would move the worker from idle_stack_ to detached_stack_. OnMainEntry() would move the worker back to idle_stack_. SchedulerWorkerThread::Detach() would always succeed. In addition to the detached thread, it would return a boolean indicating whether a wake up was pending at detachment time (IsWakeUpPending() read within the scope of |worker_lock_|). If a wake up was pending at detachment time, call outer_->WakeUp() just before setting |outer_| to nullptr. https://codereview.chromium.org/2044023003/diff/140001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.h:110: // Returns ture if the worker is alive. *true https://codereview.chromium.org/2044023003/diff/140001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker_thread_unittest.cc (right): https://codereview.chromium.org/2044023003/diff/140001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread_unittest.cc:405: ASSERT_TRUE(worker_thread->WorkerAliveForTesting()); EXPECT_TRUE() We want JoinForTesting() to run even if the condition isn't met.
https://codereview.chromium.org/2044023003/diff/100001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker_thread.cc (right): https://codereview.chromium.org/2044023003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.cc:146: DCHECK(ShouldExitForTesting() || !worker_); On 2016/06/13 19:23:05, gab wrote: > Feels we want to keep the ShouldExitForTesting() DCHECK as well though? > > Shouldn't it be DCHECK(ShouldExitForTesting() && !worker_) in fact? It seemed redundant to me as the goal of ShouldExitForTesting was to help make sure the associated thread is gone before we destroy SchedulerWorkerThread (leaking resources). If you folks feel strongly about this, I can put it back in. https://codereview.chromium.org/2044023003/diff/140001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker_thread.cc (right): https://codereview.chromium.org/2044023003/diff/140001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.cc:78: // If a wake up is pending and we successfully detached, somehow outer_ was On 2016/06/13 19:23:05, gab wrote: > |outer_| x2 Done. https://codereview.chromium.org/2044023003/diff/140001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.cc:101: const size_t kDefaultStackSize = 0; On 2016/06/13 20:31:17, fdoray wrote: > constexpr Done. https://codereview.chromium.org/2044023003/diff/140001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.cc:124: // Event signaled to wake up this SchedulerWorkerThread. On 2016/06/13 20:31:17, fdoray wrote: > this Thread (or whatever the name of this class is) Adjusted. https://codereview.chromium.org/2044023003/diff/140001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.cc:151: // It is unexpected for worker_ to be alive and for SchedulerWorkerThread to On 2016/06/13 19:23:05, gab wrote: > |worker_| Done. https://codereview.chromium.org/2044023003/diff/140001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker_thread.h (right): https://codereview.chromium.org/2044023003/diff/140001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.h:24: // A worker that manages at most one thread to run Tasks from Sequences returned On 2016/06/13 20:31:17, fdoray wrote: > one thread at a time? Changed to a single thread. https://codereview.chromium.org/2044023003/diff/140001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.h:63: // call to GetWork() returned nullptr. On 2016/06/13 20:31:17, fdoray wrote: > No new line? Done. https://codereview.chromium.org/2044023003/diff/140001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.h:65: // this function. On 2016/06/13 20:31:17, fdoray wrote: > How will the thread pool keep one active (not detached) idle thread at all time? > > Could we change this comment to "From the time this returns true to the time > OnMainEntry() is called, the worker is in a detached state." In addition to the > idle stack, the thread pool would have a detached stack. CanDetach() would > return false if idle_stack_.size() <= 1. Before returning true, CanDetach() > would move the worker from idle_stack_ to detached_stack_. OnMainEntry() would > move the worker back to idle_stack_. > > SchedulerWorkerThread::Detach() would always succeed. In addition to the > detached thread, it would return a boolean indicating whether a wake up was > pending at detachment time (IsWakeUpPending() read within the scope of > |worker_lock_|). If a wake up was pending at detachment time, call > outer_->WakeUp() just before setting |outer_| to nullptr. > How will the thread pool keep one active (not detached) idle thread at all time? The threadpool will simply make sure it returns false for CanDetach for at least one worker. If there are no threads on the idle stack, it means all of them are awake and running. It won't return true until there are some threads on the idle stack. > Could we change this comment to "From the time this returns true to the time OnMainEntry() is called, the worker is in a detached state." Not sure we can. Calling OnMainEntry presumes that a worker is alive. It would be strange to detach immediately after OnMainEntry. > SchedulerWorkerThread::Detach() would always succeed. The existing setup reduces thread churn. I'm a little suspicious if a thread is going away and it itself requests a WakeUp. The natural question is if it's trying to WakeUp a new thread, why not use the existing thread? Having the thread request a WakeUp on a successful detachment would mean shutting down the thread to create a new one. https://codereview.chromium.org/2044023003/diff/140001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.h:71: // When true is returned... On 2016/06/13 19:23:06, gab wrote: > s/.../:/ Done. https://codereview.chromium.org/2044023003/diff/140001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.h:73: // - The worker will free resources if it can. On 2016/06/13 19:23:05, gab wrote: > The comment further above initially says "reserves the right to ignore" and here > it says "will free if it can" Updated this comment and removed the statement above. https://codereview.chromium.org/2044023003/diff/140001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.h:74: // False must be returned if SchedulerWorkerThread::JoinForTesting is in On 2016/06/13 19:23:06, gab wrote: > s/False must be returned/This *must* return false if SWT::JoinForTesting() is in > progress/ Done. https://codereview.chromium.org/2044023003/diff/140001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.h:100: // May fail if the worker is detached. On failure the worker will not be able On 2016/06/13 19:23:06, gab wrote: > "the worker will not be able to call" : unclear for a reader IMO whether this > means the GetWork() call will fail or that the worker will be incapacitated and > not able to call it himself. > > I still find the last two sentences confusing. fdoray wanted to emphasize that WakeUp is generally a signal to call the Delegate's GetWork. Upon failure to create a thread, GetWork will not be called. Changed the wording to make this a little clearer. https://codereview.chromium.org/2044023003/diff/140001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.h:110: // Returns ture if the worker is alive. On 2016/06/13 20:31:17, fdoray wrote: > *true Done. https://codereview.chromium.org/2044023003/diff/140001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker_thread_unittest.cc (right): https://codereview.chromium.org/2044023003/diff/140001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread_unittest.cc:311: // Sends one item of work to signal work_processed_. On subsequent calls, On 2016/06/13 19:23:06, gab wrote: > |work_processed_| Done. https://codereview.chromium.org/2044023003/diff/140001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread_unittest.cc:326: NOTREACHED() << On 2016/06/13 19:23:06, gab wrote: > Actually, in tests, prefer ADD_FAILURE() to NOTREACHED(). Done. https://codereview.chromium.org/2044023003/diff/140001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread_unittest.cc:405: ASSERT_TRUE(worker_thread->WorkerAliveForTesting()); On 2016/06/13 20:31:17, fdoray wrote: > EXPECT_TRUE() > We want JoinForTesting() to run even if the condition isn't met. This needs to be an ASSERT. There's no thread to join if the worker is gone. A call to JoinForTesting will wakeup a new thread and we'll end up joining against that.
lgtm
lgtm w/ question https://codereview.chromium.org/2044023003/diff/100001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker_thread.cc (right): https://codereview.chromium.org/2044023003/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.cc:146: DCHECK(ShouldExitForTesting() || !worker_); On 2016/06/13 23:23:28, robliao wrote: > On 2016/06/13 19:23:05, gab wrote: > > Feels we want to keep the ShouldExitForTesting() DCHECK as well though? > > > > Shouldn't it be DCHECK(ShouldExitForTesting() && !worker_) in fact? > It seemed redundant to me as the goal of ShouldExitForTesting was to help make > sure the associated thread is gone before we destroy SchedulerWorkerThread > (leaking resources). Wasn't it also because the rest of the scheduler assumes the workers to always be around? Or is that an impl detail of the higher-level components of the scheduler? > > If you folks feel strongly about this, I can put it back in.
Patchset #5 (id:180001) has been deleted
Patchset #5 (id:200001) has been deleted
I've applied the renames. PTAL. Thanks! > Wasn't it also because the rest of the scheduler assumes the workers to always > be around? Or is that an impl detail of the higher-level components of the > scheduler? Yup. It was an impl detail of the higher levels that a SchedulerWorker would always be around and fixed in the vector of workers.
still lgtm % comments https://codereview.chromium.org/2044023003/diff/220001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker.cc (right): https://codereview.chromium.org/2044023003/diff/220001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker.cc:201: // If a wakeup is pending, then a WakeUp came in while we were deciding to WakeUp() https://codereview.chromium.org/2044023003/diff/220001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker.cc:203: // guarantee that we call GetWork after a successful wakeup. GetWork() https://codereview.chromium.org/2044023003/diff/220001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker.h (right): https://codereview.chromium.org/2044023003/diff/220001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker.h:82: // shutdown behavior of Tasks. If |worker_state| is DETACHED, the worker will the *thread* will https://codereview.chromium.org/2044023003/diff/220001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker.h:83: // be created upon a WakeUp. Returns nullptr if creating the underlying WakeUp*()* ? () are used in comments in this file: Line 92: JoinForTesting() Line 100: GetWork() https://codereview.chromium.org/2044023003/diff/220001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker.h:84: // platform thread fails during Create. Create*()*? https://codereview.chromium.org/2044023003/diff/220001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker.h:99: // WakeUp may fail if the worker is detached and it fails to allocate a new WakeUp*()*? https://codereview.chromium.org/2044023003/diff/220001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker_unittest.cc (right): https://codereview.chromium.org/2044023003/diff/220001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_unittest.cc:311: // send nullptr to indicate there's no more work to be done. Send/send or Sends/sends
Description was changed from ========== Virtualize The Existence of a SchedulerWorkerThread SchedulerWorkerThreads are expected to always remain alive in SchedulerThreadPoolImpl. At the same time, we also want threads to sleep. This change disconnects the SchedulerWorkerThread lifetime from the worker's lifetime and allows the SchedulerThreadPoolImpl to suggest when a worker can detach the associated worker. BUG=553459 ========== to ========== Virtualize The Existence of a Scheduler Worker Thread SchedulerWorkers are expected to always remain alive in SchedulerWorkerPoolImpl. At the same time, we also want threads to sleep. This change disconnects the SchedulerWorker's lifetime from the thread's lifetime and allows the SchedulerWorkerPoolImpl to suggest when a worker can detach the associated thread. BUG=553459 ==========
Done. Thanks! https://codereview.chromium.org/2044023003/diff/220001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker.cc (right): https://codereview.chromium.org/2044023003/diff/220001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker.cc:201: // If a wakeup is pending, then a WakeUp came in while we were deciding to On 2016/06/22 18:22:43, fdoray wrote: > WakeUp() Done. https://codereview.chromium.org/2044023003/diff/220001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker.cc:203: // guarantee that we call GetWork after a successful wakeup. On 2016/06/22 18:22:43, fdoray wrote: > GetWork() Done. https://codereview.chromium.org/2044023003/diff/220001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker.h (right): https://codereview.chromium.org/2044023003/diff/220001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker.h:82: // shutdown behavior of Tasks. If |worker_state| is DETACHED, the worker will On 2016/06/22 18:22:43, fdoray wrote: > the *thread* will Done. https://codereview.chromium.org/2044023003/diff/220001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker.h:83: // be created upon a WakeUp. Returns nullptr if creating the underlying On 2016/06/22 18:22:43, fdoray wrote: > WakeUp*()* ? > > () are used in comments in this file: > Line 92: JoinForTesting() > Line 100: GetWork() Done. https://codereview.chromium.org/2044023003/diff/220001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker.h:84: // platform thread fails during Create. On 2016/06/22 18:22:43, fdoray wrote: > Create*()*? Done. https://codereview.chromium.org/2044023003/diff/220001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker.h:99: // WakeUp may fail if the worker is detached and it fails to allocate a new On 2016/06/22 18:22:43, fdoray wrote: > WakeUp*()*? Done. https://codereview.chromium.org/2044023003/diff/220001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker_unittest.cc (right): https://codereview.chromium.org/2044023003/diff/220001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_unittest.cc:311: // send nullptr to indicate there's no more work to be done. On 2016/06/22 18:22:43, fdoray wrote: > Send/send or Sends/sends Adjusted to keep the subject consistent for both sentences.
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2044023003/240001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by robliao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org, fdoray@chromium.org Link to the patchset: https://codereview.chromium.org/2044023003/#ps240001 (title: "CR Feedback fdoray@")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2044023003/240001
Message was sent while issue was closed.
Description was changed from ========== Virtualize The Existence of a Scheduler Worker Thread SchedulerWorkers are expected to always remain alive in SchedulerWorkerPoolImpl. At the same time, we also want threads to sleep. This change disconnects the SchedulerWorker's lifetime from the thread's lifetime and allows the SchedulerWorkerPoolImpl to suggest when a worker can detach the associated thread. BUG=553459 ========== to ========== Virtualize The Existence of a Scheduler Worker Thread SchedulerWorkers are expected to always remain alive in SchedulerWorkerPoolImpl. At the same time, we also want threads to sleep. This change disconnects the SchedulerWorker's lifetime from the thread's lifetime and allows the SchedulerWorkerPoolImpl to suggest when a worker can detach the associated thread. BUG=553459 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Virtualize The Existence of a Scheduler Worker Thread SchedulerWorkers are expected to always remain alive in SchedulerWorkerPoolImpl. At the same time, we also want threads to sleep. This change disconnects the SchedulerWorker's lifetime from the thread's lifetime and allows the SchedulerWorkerPoolImpl to suggest when a worker can detach the associated thread. BUG=553459 ========== to ========== Virtualize The Existence of a Scheduler Worker Thread SchedulerWorkers are expected to always remain alive in SchedulerWorkerPoolImpl. At the same time, we also want threads to sleep. This change disconnects the SchedulerWorker's lifetime from the thread's lifetime and allows the SchedulerWorkerPoolImpl to suggest when a worker can detach the associated thread. BUG=553459 Committed: https://crrev.com/86b90282101ddae90c07065d3edb612b9f0a972b Cr-Commit-Position: refs/heads/master@{#401441} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/86b90282101ddae90c07065d3edb612b9f0a972b Cr-Commit-Position: refs/heads/master@{#401441} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
