|
|
DescriptionTaskScheduler [5/9] Task Tracker
This change is a subset of https://codereview.chromium.org/1698183005/
All tasks go through the scheduler's TaskTracker when they are posted and
when they are executed. The TaskTracker enforces shutdown semantics and takes
care of tracing and profiling.
We have an alternate implementation which uses atomic operations to reduce lock
contention. However, we prefer to check this simple implementation in first and
bring the atomic operations in an incremental CL.
BUG=553459
Committed: https://crrev.com/0fc7a6669c12c2ef2d1ccbd496232b7c65f6c81c
Cr-Commit-Position: refs/heads/master@{#383749}
Patch Set 1 #Patch Set 2 : rebase #
Total comments: 25
Patch Set 3 : ShutdownManager -> TaskTracker #Patch Set 4 : rebase #Patch Set 5 : self review #
Total comments: 1
Patch Set 6 : fix PostTask comment #
Total comments: 43
Patch Set 7 : address comments gab+robliao 15-17 #Patch Set 8 : self review #Patch Set 9 : self review #Patch Set 10 : expand tests #
Total comments: 28
Patch Set 11 : CR gab #19 #
Total comments: 25
Patch Set 12 : CR from gab #23 #Patch Set 13 : self review #
Total comments: 16
Patch Set 14 : CR gab/robliao #26-27 #Patch Set 15 : CR gab/robliao #26-27 #Patch Set 16 : self review #
Total comments: 16
Patch Set 17 : self review #Patch Set 18 : self review #
Total comments: 2
Patch Set 19 : CR gab/asvitkine #31-35 #
Total comments: 18
Patch Set 20 : CR gab/danakj #38-39 #Patch Set 21 : CR gab/danakj #38-39 #
Total comments: 2
Patch Set 22 : CR gab #41 (fix a comment) #
Total comments: 3
Patch Set 23 : fix build errors #Patch Set 24 : fix build errors #Patch Set 25 : CR robliao #61 #
Dependent Patchsets: Messages
Total messages: 68 (19 generated)
Description was changed from ========== TaskScheduler [5/8] Shutdown Manager R=gab@chromium.org, robliao@chromium.org ========== to ========== TaskScheduler [5/8] Shutdown Manager This change is a subset of https://codereview.chromium.org/1698183005/ As documented in https://docs.google.com/a/chromium.org/document/d/1S2AAeoo1xa_vsLbDYBsDHCqhrk... BUG=553459 ==========
Description was changed from ========== TaskScheduler [5/8] Shutdown Manager This change is a subset of https://codereview.chromium.org/1698183005/ As documented in https://docs.google.com/a/chromium.org/document/d/1S2AAeoo1xa_vsLbDYBsDHCqhrk... BUG=553459 ========== to ========== TaskScheduler [5/8] Shutdown Manager This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ==========
1st pass, would like you two's input on my mega meta comment about the API of ShutdownManager below. Cheers, Gab https://codereview.chromium.org/1705943002/diff/20001/base/task_scheduler/shu... File base/task_scheduler/shutdown_manager.cc (right): https://codereview.chromium.org/1705943002/diff/20001/base/task_scheduler/shu... base/task_scheduler/shutdown_manager.cc:24: "simultaneously on multiple threads."; How about (as mentioned in header): DCHECK(!shutdown_completed_ || is_shuttting_down) << "Shutdown() should only be invoked once and from one thread only."; https://codereview.chromium.org/1705943002/diff/20001/base/task_scheduler/shu... base/task_scheduler/shutdown_manager.cc:36: AutoSchedulerLock auto_lock(lock_); Please highlight in CL description that you have a plan for a lock free atomic version of ShutdownManager but checking this one in first will make it easier to bring that as an incremental CL. https://codereview.chromium.org/1705943002/diff/20001/base/task_scheduler/shu... base/task_scheduler/shutdown_manager.cc:90: subtle::MemoryBarrier(); Locking seems fine in test code (at least for now). The header for atomicops.h says "If you plan to use these routines, you should have a good reason, such as solid evidence that performance would otherwise suffer, or there being no alternative." and test code clearly doesn't qualify :-). https://codereview.chromium.org/1705943002/diff/20001/base/task_scheduler/shu... base/task_scheduler/shutdown_manager.cc:95: subtle::MemoryBarrier(); I'd say use a lock here for now (until you come up with a lock free version of the entire thing). It doesn't seem critical yet as this is only called on the outside of the WorkerThread's main loop. https://codereview.chromium.org/1705943002/diff/20001/base/task_scheduler/shu... File base/task_scheduler/shutdown_manager.h (right): https://codereview.chromium.org/1705943002/diff/20001/base/task_scheduler/shu... base/task_scheduler/shutdown_manager.h:29: // - There is no pending BLOCK_SHUTDOWN tasks. s/is/are/ https://codereview.chromium.org/1705943002/diff/20001/base/task_scheduler/shu... base/task_scheduler/shutdown_manager.h:30: // This cannot be called simultaneously on multiple threads. // This should only be called on shutdown from one thread only. (the previous form makes it sound like it's still okay to call this multiple times, just not from multiple threads whereas I think we want to articulate this should only be called once, from one thread) https://codereview.chromium.org/1705943002/diff/20001/base/task_scheduler/shu... base/task_scheduler/shutdown_manager.h:34: bool ShouldPostTask(TaskShutdownBehavior shutdown_behavior); This actually does more than return its opinion, it also records the task has having been posted. With the API as-is it would be correct for an algorithm to call this multiple times before posting a single task (not sure why but it wouldn't be outright incorrect). How about changing this to: // Informs ShutdownManager that a task with |shutdown_behavior| is about to be // posted. Returns true if the ShutdownManager approves of this operation (the // task should otherwise not be posted). bool RequestPostTask(TaskShutdownBehavior shutdown_behavior); Another option is (even more explicit): // Forwards |task| to |post_task_continuation| unless the current shutdown state // prevents that in which case |task| is dropped. void PostTask(const base::Callback<scoped_ptr<Task>> post_task_continuation, scoped_ptr<Task> task); WDYT? I like option 2 except for the fact that it adds ShutdownManager in the direct task posting graph instead of a side dependency. Given we also depend on the DidExecuteTask() call I guess sticking to option 1 as a similar "inform me" API is okay. That brings me to 65/35 in favor of option 1 I think.. ;-) EDIT: So I just read the test which made it even more clear to me that this was required and gave me a third idea (now including ShouldScheduleTask()): // Forwards |task| to |post_task_continuation| unless the current shutdown state // prevents that in which case |task| is dropped. void PostTask(const base::Callback<scoped_ptr<Task>> post_task_continuation, scoped_ptr<Task> task); // Returns a ScopedTask which wraps |task| unless the current shutdown state // prevents |task|'s execution in which case |task| is dropped and the returned // pointer is null. ShutdownManager will use the scope of the returned object // when counting live tasks during shutdown. ShutdownManager::ScopedTask RequestExecuteTask(scoped_ptr<Task> task); where: class ShutdownManager { class ScopedTask { public: ScopedTask(ShutdownManager* outer, scoped_ptr<Task> task) : outer_(outer), task_(std::move(task)) {} ScopedTask(ScopedTask&& moved) : outer_(moved.outer), task_(std::move(moved.task_)) { moved.outer_ = nullptr; } ~ScopedTask() { if (outer_) { // TODO: Decrease |outer_|'s accordingly state post execution. } } const Task* task() { return task.get(); } private: ShutdownManager* outer_; scoped_ptr<Task> task_; DISALLOW_COPY_AND_ASSIGN(ScopedTask); }; (...) }; Once again, looking for your two's input of these 3 options. Cheers! https://codereview.chromium.org/1705943002/diff/20001/base/task_scheduler/shu... base/task_scheduler/shutdown_manager.h:46: bool shutdown_completed() const; Inline impl of lower_case_getters_and_setters() above. https://codereview.chromium.org/1705943002/diff/20001/base/task_scheduler/shu... base/task_scheduler/shutdown_manager.h:53: // zero. [gab TODO] Append "while |is_shutting_down_|"? https://codereview.chromium.org/1705943002/diff/20001/base/task_scheduler/shu... File base/task_scheduler/shutdown_manager_unittest.cc (right): https://codereview.chromium.org/1705943002/diff/20001/base/task_scheduler/shu... base/task_scheduler/shutdown_manager_unittest.cc:27: PlatformThread::Sleep(TimeDelta::FromMilliseconds(5)); 5 seems arbitrary. How about PlatformThread::YieldCurrentThread()? https://codereview.chromium.org/1705943002/diff/20001/base/task_scheduler/shu... base/task_scheduler/shutdown_manager_unittest.cc:183: TEST(TaskSchedulerShutdownManagerTest, ShutdownWithQueuedBlockShutdownTasks) { I think ShouldPostAndScheduleTaskDuringShutdown already tests this and more. WDYT?
Just responding to the large comment. https://codereview.chromium.org/1705943002/diff/20001/base/task_scheduler/shu... File base/task_scheduler/shutdown_manager.h (right): https://codereview.chromium.org/1705943002/diff/20001/base/task_scheduler/shu... base/task_scheduler/shutdown_manager.h:34: bool ShouldPostTask(TaskShutdownBehavior shutdown_behavior); On 2016/02/23 22:28:23, gab wrote: > This actually does more than return its opinion, it also records the task has > having been posted. With the API as-is it would be correct for an algorithm to > call this multiple times before posting a single task (not sure why but it > wouldn't be outright incorrect). > > How about changing this to: > > // Informs ShutdownManager that a task with |shutdown_behavior| is about to be > // posted. Returns true if the ShutdownManager approves of this operation (the > // task should otherwise not be posted). > bool RequestPostTask(TaskShutdownBehavior shutdown_behavior); > > Another option is (even more explicit): > > // Forwards |task| to |post_task_continuation| unless the current shutdown state > // prevents that in which case |task| is dropped. > void PostTask(const base::Callback<scoped_ptr<Task>> post_task_continuation, > scoped_ptr<Task> task); > > WDYT? > > I like option 2 except for the fact that it adds ShutdownManager in the direct > task posting graph instead of a side dependency. > > Given we also depend on the DidExecuteTask() call I guess sticking to option 1 > as a similar "inform me" API is okay. > > That brings me to 65/35 in favor of option 1 I think.. ;-) > > > EDIT: > > So I just read the test which made it even more clear to me that this was > required and gave me a third idea (now including ShouldScheduleTask()): > > // Forwards |task| to |post_task_continuation| unless the current shutdown state > // prevents that in which case |task| is dropped. > void PostTask(const base::Callback<scoped_ptr<Task>> post_task_continuation, > scoped_ptr<Task> task); > > // Returns a ScopedTask which wraps |task| unless the current shutdown state > // prevents |task|'s execution in which case |task| is dropped and the returned > // pointer is null. ShutdownManager will use the scope of the returned object > // when counting live tasks during shutdown. > ShutdownManager::ScopedTask RequestExecuteTask(scoped_ptr<Task> task); > > > where: > > class ShutdownManager { > class ScopedTask { > public: > ScopedTask(ShutdownManager* outer, > scoped_ptr<Task> task) : outer_(outer), task_(std::move(task)) {} > > ScopedTask(ScopedTask&& moved) : outer_(moved.outer), > task_(std::move(moved.task_)) { > moved.outer_ = nullptr; > } > > ~ScopedTask() { > if (outer_) { > // TODO: Decrease |outer_|'s accordingly state post execution. > } > } > > const Task* task() { return task.get(); } > > private: > ShutdownManager* outer_; > > scoped_ptr<Task> task_; > > DISALLOW_COPY_AND_ASSIGN(ScopedTask); > }; > > (...) > }; > > > Once again, looking for your two's input of these 3 options. > > Cheers! What I like about the ShutdownManager right now is that it doesn't have to hold on to tasks, so we should definitely keep that as a goal. All options currently do that. I also agree that Should* series of calls should have const marked on the method. These should not have an effect on the ShutdownManager. So now it comes down to how we track the executions. Option 3 means that the scheduler infrastructure would need to hold on to a ScopedTask factoried from the ShutdownManager. While this gets the job done, the relationship of who controls the tasks seems a little different in that the ShutdownManager always has to be involved in all tasks. How about Option 4: The ShutdownManager answers questions about tasks, captured by bool ShouldPostTask(Task) const; bool ShouldScheduleTask(Task) const; The ShutdownManger also implements a Scheduler observer (whatever name makes sense) void OnTaskPosted(Task); void OnTaskScheduled(Task); We can either have one listener for now or have a set if we decide other components need to know about when tasks are posted or scheduled (or more).
One nit and waiting for the resolution of the API. https://codereview.chromium.org/1705943002/diff/20001/base/task_scheduler/shu... File base/task_scheduler/shutdown_manager.cc (right): https://codereview.chromium.org/1705943002/diff/20001/base/task_scheduler/shu... base/task_scheduler/shutdown_manager.cc:57: case TaskShutdownBehavior::BLOCK_SHUTDOWN: { Nit: We generally don't use braces for switch statements unless we need additional local variables.
I just replied to the big comment. https://codereview.chromium.org/1705943002/diff/20001/base/task_scheduler/shu... File base/task_scheduler/shutdown_manager.h (right): https://codereview.chromium.org/1705943002/diff/20001/base/task_scheduler/shu... base/task_scheduler/shutdown_manager.h:34: bool ShouldPostTask(TaskShutdownBehavior shutdown_behavior); On 2016/02/23 22:47:02, robliao wrote: > On 2016/02/23 22:28:23, gab wrote: > > This actually does more than return its opinion, it also records the task has > > having been posted. With the API as-is it would be correct for an algorithm to > > call this multiple times before posting a single task (not sure why but it > > wouldn't be outright incorrect). > > > > How about changing this to: > > > > // Informs ShutdownManager that a task with |shutdown_behavior| is about to be > > // posted. Returns true if the ShutdownManager approves of this operation (the > > // task should otherwise not be posted). > > bool RequestPostTask(TaskShutdownBehavior shutdown_behavior); > > > > Another option is (even more explicit): > > > > // Forwards |task| to |post_task_continuation| unless the current shutdown > state > > // prevents that in which case |task| is dropped. > > void PostTask(const base::Callback<scoped_ptr<Task>> post_task_continuation, > > scoped_ptr<Task> task); > > > > WDYT? > > > > I like option 2 except for the fact that it adds ShutdownManager in the direct > > task posting graph instead of a side dependency. > > > > Given we also depend on the DidExecuteTask() call I guess sticking to option 1 > > as a similar "inform me" API is okay. > > > > That brings me to 65/35 in favor of option 1 I think.. ;-) > > > > > > EDIT: > > > > So I just read the test which made it even more clear to me that this was > > required and gave me a third idea (now including ShouldScheduleTask()): > > > > // Forwards |task| to |post_task_continuation| unless the current shutdown > state > > // prevents that in which case |task| is dropped. > > void PostTask(const base::Callback<scoped_ptr<Task>> post_task_continuation, > > scoped_ptr<Task> task); > > > > // Returns a ScopedTask which wraps |task| unless the current shutdown state > > // prevents |task|'s execution in which case |task| is dropped and the > returned > > // pointer is null. ShutdownManager will use the scope of the returned object > > // when counting live tasks during shutdown. > > ShutdownManager::ScopedTask RequestExecuteTask(scoped_ptr<Task> task); > > > > > > where: > > > > class ShutdownManager { > > class ScopedTask { > > public: > > ScopedTask(ShutdownManager* outer, > > scoped_ptr<Task> task) : outer_(outer), task_(std::move(task)) > {} > > > > ScopedTask(ScopedTask&& moved) : outer_(moved.outer), > > task_(std::move(moved.task_)) { > > moved.outer_ = nullptr; > > } > > > > ~ScopedTask() { > > if (outer_) { > > // TODO: Decrease |outer_|'s accordingly state post execution. > > } > > } > > > > const Task* task() { return task.get(); } > > > > private: > > ShutdownManager* outer_; > > > > scoped_ptr<Task> task_; > > > > DISALLOW_COPY_AND_ASSIGN(ScopedTask); > > }; > > > > (...) > > }; > > > > > > Once again, looking for your two's input of these 3 options. > > > > Cheers! > > What I like about the ShutdownManager right now is that it doesn't have to hold > on to tasks, so we should definitely keep that as a goal. All options currently > do that. > > I also agree that Should* series of calls should have const marked on the > method. These should not have an effect on the ShutdownManager. > > So now it comes down to how we track the executions. > > Option 3 means that the scheduler infrastructure would need to hold on to a > ScopedTask factoried from the ShutdownManager. While this gets the job done, the > relationship of who controls the tasks seems a little different in that the > ShutdownManager always has to be involved in all tasks. > > How about Option 4: > > The ShutdownManager answers questions about tasks, captured by > bool ShouldPostTask(Task) const; > bool ShouldScheduleTask(Task) const; > > The ShutdownManger also implements a Scheduler observer (whatever name makes > sense) > void OnTaskPosted(Task); > void OnTaskScheduled(Task); > > We can either have one listener for now or have a set if we decide other > components need to know about when tasks are posted or scheduled (or more). OPTION 1: Pro: ShutdownManager doesn't have to know about Task (only TaskShutdownBehavior). Con: Hard to guarantee that RequestPostTask/RequestScheduleTask aren't called multiple times for the same Task. OPTION 2: Pro: Guarantees that RequestPostTask is called only once per Task. Con: It's weird that PostTask is done by ShutdownManager (even if it's via a callback). We will have to release the ShutdownManager lock before invoking the callback. OPTION 3: The code to run a task would look like that: { auto scoped_task = shutdown_manager->RequestExecuteTask(std::move(task)); if (scoped_task.get()) debug::TaskAnnotator().RunTask("", *scoped_task.get()); } I think that ScopedTask introduces complexity for no reason. IMO, option 5 is simpler to understand. OPTION 4: Checking if a task can be posted/scheduled and incrementing the number of tasks blocking shutdown has to be done atomically. With option 4, this problematic scenario can happen: . Thread 1: Checks if it can schedule a SKIP_ON_SHUTDOWN task. ShouldScheduleTask returns true because shutdown hasn't started. . Thread 2: Calls Shutdown(), which returns immediately because there is no tasks blocking shutdown. . Thread 1: Starts running its SKIP_ON_SHUTDOWN task :( If the number of tasks blocking shutdown had been incremented BY ShouldScheduleTask, thread 2 wouldn't have returned from Shutdown() before the SKIP_ON_SHUTDOWN task had completed its execution. **OPTION 5**: class ShutdownManager { public: // Forwards |task| to |post_task_continuation| unless the current shutdown // state prevents that in which case |task| is dropped. void PostTask(const base::Callback<scoped_ptr<Task>> post_task_continuation, scoped_ptr<Task> task); // Runs |task| unless the current shutdown state prevents that. void RunTask(scoped_ptr<Task> task); }; The code to run a task would look like that (simpler than option 3): shutdown_manager->RunTask(std::move(task));
On 2016/02/24 17:01:06, fdoray wrote: > I just replied to the big comment. > > https://codereview.chromium.org/1705943002/diff/20001/base/task_scheduler/shu... > File base/task_scheduler/shutdown_manager.h (right): > > https://codereview.chromium.org/1705943002/diff/20001/base/task_scheduler/shu... > base/task_scheduler/shutdown_manager.h:34: bool > ShouldPostTask(TaskShutdownBehavior shutdown_behavior); > On 2016/02/23 22:47:02, robliao wrote: > > On 2016/02/23 22:28:23, gab wrote: > > > This actually does more than return its opinion, it also records the task > has > > > having been posted. With the API as-is it would be correct for an algorithm > to > > > call this multiple times before posting a single task (not sure why but it > > > wouldn't be outright incorrect). > > > > > > How about changing this to: > > > > > > // Informs ShutdownManager that a task with |shutdown_behavior| is about to > be > > > // posted. Returns true if the ShutdownManager approves of this operation > (the > > > // task should otherwise not be posted). > > > bool RequestPostTask(TaskShutdownBehavior shutdown_behavior); > > > > > > Another option is (even more explicit): > > > > > > // Forwards |task| to |post_task_continuation| unless the current shutdown > > state > > > // prevents that in which case |task| is dropped. > > > void PostTask(const base::Callback<scoped_ptr<Task>> post_task_continuation, > > > scoped_ptr<Task> task); > > > > > > WDYT? > > > > > > I like option 2 except for the fact that it adds ShutdownManager in the > direct > > > task posting graph instead of a side dependency. > > > > > > Given we also depend on the DidExecuteTask() call I guess sticking to option > 1 > > > as a similar "inform me" API is okay. > > > > > > That brings me to 65/35 in favor of option 1 I think.. ;-) > > > > > > > > > EDIT: > > > > > > So I just read the test which made it even more clear to me that this was > > > required and gave me a third idea (now including ShouldScheduleTask()): > > > > > > // Forwards |task| to |post_task_continuation| unless the current shutdown > > state > > > // prevents that in which case |task| is dropped. > > > void PostTask(const base::Callback<scoped_ptr<Task>> post_task_continuation, > > > scoped_ptr<Task> task); > > > > > > // Returns a ScopedTask which wraps |task| unless the current shutdown state > > > // prevents |task|'s execution in which case |task| is dropped and the > > returned > > > // pointer is null. ShutdownManager will use the scope of the returned > object > > > // when counting live tasks during shutdown. > > > ShutdownManager::ScopedTask RequestExecuteTask(scoped_ptr<Task> task); > > > > > > > > > where: > > > > > > class ShutdownManager { > > > class ScopedTask { > > > public: > > > ScopedTask(ShutdownManager* outer, > > > scoped_ptr<Task> task) : outer_(outer), > task_(std::move(task)) > > {} > > > > > > ScopedTask(ScopedTask&& moved) : outer_(moved.outer), > > > task_(std::move(moved.task_)) { > > > moved.outer_ = nullptr; > > > } > > > > > > ~ScopedTask() { > > > if (outer_) { > > > // TODO: Decrease |outer_|'s accordingly state post execution. > > > } > > > } > > > > > > const Task* task() { return task.get(); } > > > > > > private: > > > ShutdownManager* outer_; > > > > > > scoped_ptr<Task> task_; > > > > > > DISALLOW_COPY_AND_ASSIGN(ScopedTask); > > > }; > > > > > > (...) > > > }; > > > > > > > > > Once again, looking for your two's input of these 3 options. > > > > > > Cheers! > > > > What I like about the ShutdownManager right now is that it doesn't have to > hold > > on to tasks, so we should definitely keep that as a goal. All options > currently > > do that. > > > > I also agree that Should* series of calls should have const marked on the > > method. These should not have an effect on the ShutdownManager. > > > > So now it comes down to how we track the executions. > > > > Option 3 means that the scheduler infrastructure would need to hold on to a > > ScopedTask factoried from the ShutdownManager. While this gets the job done, > the > > relationship of who controls the tasks seems a little different in that the > > ShutdownManager always has to be involved in all tasks. > > > > How about Option 4: > > > > The ShutdownManager answers questions about tasks, captured by > > bool ShouldPostTask(Task) const; > > bool ShouldScheduleTask(Task) const; > > > > The ShutdownManger also implements a Scheduler observer (whatever name makes > > sense) > > void OnTaskPosted(Task); > > void OnTaskScheduled(Task); > > > > We can either have one listener for now or have a set if we decide other > > components need to know about when tasks are posted or scheduled (or more). > > OPTION 1: > Pro: ShutdownManager doesn't have to know about Task (only > TaskShutdownBehavior). > Con: Hard to guarantee that RequestPostTask/RequestScheduleTask aren't called > multiple times for the same Task. > > OPTION 2: > Pro: Guarantees that RequestPostTask is called only once per Task. > Con: It's weird that PostTask is done by ShutdownManager (even if it's via a > callback). We will have to release the ShutdownManager lock before invoking the > callback. > > OPTION 3: The code to run a task would look like that: > > { > auto scoped_task = shutdown_manager->RequestExecuteTask(std::move(task)); > if (scoped_task.get()) > debug::TaskAnnotator().RunTask("", *scoped_task.get()); > } > > I think that ScopedTask introduces complexity for no reason. IMO, option 5 is > simpler to understand. > > OPTION 4: Checking if a task can be posted/scheduled and incrementing the number > of tasks blocking shutdown has to be done atomically. With option 4, this > problematic scenario can happen: > . Thread 1: Checks if it can schedule a SKIP_ON_SHUTDOWN task. > ShouldScheduleTask returns true because shutdown hasn't started. > . Thread 2: Calls Shutdown(), which returns immediately because there is no > tasks blocking shutdown. > . Thread 1: Starts running its SKIP_ON_SHUTDOWN task :( If the number of tasks > blocking shutdown had been incremented BY ShouldScheduleTask, thread 2 wouldn't > have returned from Shutdown() before the SKIP_ON_SHUTDOWN task had completed its > execution. > > **OPTION 5**: > > class ShutdownManager { > public: > // Forwards |task| to |post_task_continuation| unless the current shutdown > // state prevents that in which case |task| is dropped. > void PostTask(const base::Callback<scoped_ptr<Task>> post_task_continuation, > scoped_ptr<Task> task); > > // Runs |task| unless the current shutdown state prevents that. > void RunTask(scoped_ptr<Task> task); > }; > > The code to run a task would look like that (simpler than option 3): > shutdown_manager->RunTask(std::move(task)); Summarizing offline discussion for reference. We all prefer option #5, so long as this class is renamed from "ShutdownManager" to "TaskTracker" as it now does more than just shutdown specific things (tracing, profiling, shutdown management) and is the eventual perfect place to add more task tracking hooks (another plus of option #5 actually :-)). Thanks all! Gab
Description was changed from ========== TaskScheduler [5/8] Shutdown Manager This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ========== to ========== TaskScheduler [5/8] Task Tracker This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ==========
Description was changed from ========== TaskScheduler [5/8] Task Tracker This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ========== to ========== TaskScheduler [5/8] Task Tracker This change is a subset of https://codereview.chromium.org/1698183005/ We have an alternate implementation which uses atomic operations to reduce lock contention. However, we prefer to check this simple implementation in first and bring the atomic operations in an incremental CL. BUG=553459 ==========
Can you review this CL again? ShutdownManager is now TaskTracker. https://codereview.chromium.org/1705943002/diff/20001/base/task_scheduler/shu... File base/task_scheduler/shutdown_manager.cc (right): https://codereview.chromium.org/1705943002/diff/20001/base/task_scheduler/shu... base/task_scheduler/shutdown_manager.cc:24: "simultaneously on multiple threads."; On 2016/02/23 22:28:22, gab wrote: > How about (as mentioned in header): > > DCHECK(!shutdown_completed_ || is_shuttting_down) > << "Shutdown() should only be invoked once and from one thread only."; Done. https://codereview.chromium.org/1705943002/diff/20001/base/task_scheduler/shu... base/task_scheduler/shutdown_manager.cc:36: AutoSchedulerLock auto_lock(lock_); On 2016/02/23 22:28:23, gab wrote: > Please highlight in CL description that you have a plan for a lock free atomic > version of ShutdownManager but checking this one in first will make it easier to > bring that as an incremental CL. Done. https://codereview.chromium.org/1705943002/diff/20001/base/task_scheduler/shu... base/task_scheduler/shutdown_manager.cc:57: case TaskShutdownBehavior::BLOCK_SHUTDOWN: { On 2016/02/24 01:22:44, robliao wrote: > Nit: We generally don't use braces for switch statements unless we need > additional local variables. Done. https://codereview.chromium.org/1705943002/diff/20001/base/task_scheduler/shu... base/task_scheduler/shutdown_manager.cc:90: subtle::MemoryBarrier(); On 2016/02/23 22:28:23, gab wrote: > Locking seems fine in test code (at least for now). > > The header for atomicops.h says "If you plan to use these routines, you should > have a good reason, such as solid evidence that performance would otherwise > suffer, or there being no alternative." > > and test code clearly doesn't qualify :-). Done. https://codereview.chromium.org/1705943002/diff/20001/base/task_scheduler/shu... base/task_scheduler/shutdown_manager.cc:95: subtle::MemoryBarrier(); On 2016/02/23 22:28:22, gab wrote: > I'd say use a lock here for now (until you come up with a lock free version of > the entire thing). > > It doesn't seem critical yet as this is only called on the outside of the > WorkerThread's main loop. Done. https://codereview.chromium.org/1705943002/diff/20001/base/task_scheduler/shu... File base/task_scheduler/shutdown_manager.h (right): https://codereview.chromium.org/1705943002/diff/20001/base/task_scheduler/shu... base/task_scheduler/shutdown_manager.h:29: // - There is no pending BLOCK_SHUTDOWN tasks. On 2016/02/23 22:28:23, gab wrote: > s/is/are/ Done. https://codereview.chromium.org/1705943002/diff/20001/base/task_scheduler/shu... base/task_scheduler/shutdown_manager.h:30: // This cannot be called simultaneously on multiple threads. On 2016/02/23 22:28:23, gab wrote: > // This should only be called on shutdown from one thread only. > > (the previous form makes it sound like it's still okay to call this multiple > times, just not from multiple threads whereas I think we want to articulate this > should only be called once, from one thread) Done. "Must only be called once" implies "from one thread only". https://codereview.chromium.org/1705943002/diff/20001/base/task_scheduler/shu... base/task_scheduler/shutdown_manager.h:46: bool shutdown_completed() const; On 2016/02/23 22:28:23, gab wrote: > Inline impl of lower_case_getters_and_setters() above. Done. https://codereview.chromium.org/1705943002/diff/20001/base/task_scheduler/shu... base/task_scheduler/shutdown_manager.h:53: // zero. On 2016/02/23 22:28:23, gab wrote: > [gab TODO] > > Append "while |is_shutting_down_|"? Done. https://codereview.chromium.org/1705943002/diff/20001/base/task_scheduler/shu... File base/task_scheduler/shutdown_manager_unittest.cc (right): https://codereview.chromium.org/1705943002/diff/20001/base/task_scheduler/shu... base/task_scheduler/shutdown_manager_unittest.cc:27: PlatformThread::Sleep(TimeDelta::FromMilliseconds(5)); On 2016/02/23 22:28:23, gab wrote: > 5 seems arbitrary. How about PlatformThread::YieldCurrentThread()? Done. https://codereview.chromium.org/1705943002/diff/20001/base/task_scheduler/shu... base/task_scheduler/shutdown_manager_unittest.cc:183: TEST(TaskSchedulerShutdownManagerTest, ShutdownWithQueuedBlockShutdownTasks) { On 2016/02/23 22:28:23, gab wrote: > I think ShouldPostAndScheduleTaskDuringShutdown already tests this and more. > WDYT? I completely refactored the unit tests :)
https://codereview.chromium.org/1705943002/diff/80001/base/task_scheduler/tas... File base/task_scheduler/task_tracker.h (right): https://codereview.chromium.org/1705943002/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_tracker.h:38: void PostTask(const Callback<void(scoped_ptr<Task>)>& post_task_continuation, A part of the previous architecture was certainly simpler. The flow would be from the scheduler's point of view: if (task_tracker->ShouldPostTask(task)) { Post The Task task_tracker->OnTaskPosted(task); } This decouples the relationship between the task tracker and the consumer into a one way relationship. The scheduler requests info from the TaskTracker and informs the TaskTracker on actions taken. In this setup, the caller is now coupled to a callback from TaskTracker that could reenter depending on the callback.
I liked the TaskTracker { PostTask(), RunTask() } because it could have been a good place to add instrumentation in the future. However, now that I try to use it in WorkerThread, I find it too complex (callback makes it harder to reason about locks, can't transfer ownership of Task to TaskTracker if we want to keep it in its Sequence while it is executed...). What do you think of this interface: class ShutdownManager { public: bool RequestPostTask(TaskShutdownBehavior); bool RequestScheduleTask(TaskShutdownBehavior); void DidExecuteTask(TaskShutdownBehavior); }; robliao@: I don't want 2 separate methods ShouldPostTask/OnTaskPosted because of this scenario: 1. ShouldPostTask(BLOCK_SHUTDOWN) returns true. 2. Shutdown() is called and returns immediately because the number of tasks blocking shutdown is 0. 3. The task is posted... but it shouldn't because shutdown has now completed. We wouldn't have this problem with a RequestPostTask which checks if the task can run and updates the number of tasks blocking shutdown in one atomic step.
On 2016/02/29 23:08:24, fdoray wrote: > I liked the TaskTracker { PostTask(), RunTask() } because it could have been a > good place to add instrumentation in the future. However, now that I try to use > it in WorkerThread, I find it too complex (callback makes it harder to reason > about locks, can't transfer ownership of Task to TaskTracker if we want to keep > it in its Sequence while it is executed...). > > What do you think of this interface: > > class ShutdownManager { > public: > bool RequestPostTask(TaskShutdownBehavior); > bool RequestScheduleTask(TaskShutdownBehavior); > void DidExecuteTask(TaskShutdownBehavior); > }; > > robliao@: I don't want 2 separate methods ShouldPostTask/OnTaskPosted because of > this scenario: > 1. ShouldPostTask(BLOCK_SHUTDOWN) returns true. > 2. Shutdown() is called and returns immediately because the number of tasks > blocking shutdown is 0. > 3. The task is posted... but it shouldn't because shutdown has now completed. > We wouldn't have this problem with a RequestPostTask which checks if the task > can run and updates the number of tasks blocking shutdown in one atomic step. I wonder if we should make TaskTracker transactional. In a light scan, I don't see any lock violations yet. It's not my favorite of alternatives, but it's one way to keep atomicity. With the single method for atomicity solution, we should make sure that the names make it clear that mutation can occur. RequestPostTask doesn't sound strong enough. Something like OnTaskPosting might work, return true if posting should continue, false otherwise, but I'm not a big fan of the name.
https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/ta... File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/ta... base/task_scheduler/task_tracker.cc:50: task_annotator.DidQueueTask(kQueueFunctionName, *task.get()); Not your fault but noticing that "DidQueueTask" is a weird name as it intuitively feels it should be called *before* queueing the task (or it may run before it's called) and this is in fact what existing impls do... so it should really be "WillPostTask"... https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/ta... base/task_scheduler/task_tracker.cc:83: bool TaskTracker::RequestScheduleTask(TaskShutdownBehavior shutdown_behavior) { "RequestRunTask" to match nomenclature in rest of this class? https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/ta... File base/task_scheduler/task_tracker.h (right): https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/ta... base/task_scheduler/task_tracker.h:38: // tracing event and invoking |post_task_callback|. |post_task_callback| must ',' before "and" https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/ta... base/task_scheduler/task_tracker.h:40: // shutdown_completed() returns true. s/are run until shutdown_completed() returns true/are handed off to this class' RunTask() when they are to be executed/ It's important that those tasks make it back through RunTask() -- and it's not otherwise relevant to this API when the pool exits. https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/ta... base/task_scheduler/task_tracker.h:45: // have been successfully posted via PostTask(). s/./ first./ https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/ta... File base/task_scheduler/task_tracker_unittest.cc (right): https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:62: return posted_task; This feels very weird... how about instead have a fake queue here, e.g. std::dequeue task_queue_; accompanied by an AddToQueue() method used in the Bind here? And tests can test the top of the queue instead of doing the raw pointer to returned pointer comparison. https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:92: size_t num_run_tasks_; num_tasks_executed_ ? https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:120: EXPECT_EQ(1U, num_run_tasks_); EXPECT_EQ(0U, ...); as well before RunTask()? https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:279: // pending BLOCK_SHUTDOWN tasks. I think your code actually handles this (+ a DCHECK) so it should be test this with a DEBUG_DEATH test, no? https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:376: } To avoid the triad duplication for every test, how about making TaskSchedulerTaskTrackerTest a parametrized test with testing::WithParamInterface<TaskShutdownBehavior>. Then you can do INSTANTIATE_TEST_CASE_P to get each test to run once in each mode and use GetParam() to figure out which mode you're in right now to drive the little difference in behavior and expectations for each mode? https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/ta... File base/task_scheduler/task_traits.h (right): https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/ta... base/task_scheduler/task_traits.h:139: BASE_EXPORT std::ostream& operator<<( Doesn't PrintTo() also work for this type as above for TaskPriority? Feels weird to have this implemented two different ways for these two very similar types. https://codereview.chromium.org/1705943002/diff/100001/base/test/task_runner_... File base/test/task_runner_test_template.h (right): https://codereview.chromium.org/1705943002/diff/100001/base/test/task_runner_... base/test/task_runner_test_template.h:75: namespace testing { Let's do this change in another CL -- feel free to point to this one for motivation.
https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/ta... File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/ta... base/task_scheduler/task_tracker.cc:69: bool TaskTracker::RequestPostTask(TaskShutdownBehavior shutdown_behavior) { Now that these are private, we can give them names that reflect when they're called. RequestPostTask -> BeforeTaskPosted RequestScheduleTask -> BeforeTaskExecuted DidExecuteTask -> AfterTaskExecuted https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/ta... base/task_scheduler/task_tracker.cc:98: ++num_tasks_blocking_shutdown_; It's worth commented that we want Shutdown to block on these tasks during execution and we increment here since we don't want to include them in the overall blocking count unless they're currently executing. https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/ta... base/task_scheduler/task_tracker.cc:105: return false; Might as well add a NOTREACHED() here. https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/ta... File base/task_scheduler/task_tracker.h (right): https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/ta... base/task_scheduler/task_tracker.h:28: // Shuts down the task tracker. Once this is called, only tasks posted with The blocking behavior is important enough to reemphasize. Let's add Synchronously shutdown down the task tracker. https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/ta... base/task_scheduler/task_tracker.h:36: // Posts |task| unless the current shutdown state prevents that. Posting a Calls |post_task_callback| unless the current shutdown state prevents that. https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/ta... File base/task_scheduler/task_tracker_unittest.cc (right): https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:16: namespace base { I think you need a test case where we block shutdown while a CONTINUE_ON_SHUTDOWN task is running. https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:80: thread_calling_shutdown_->Join(); It would be unexpected to join with the duality between ExpectShutdownAsyncHas[Not]Returned. Maybe JoinAndExpectShutdownAsyncHasReturned? https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:276: PostBeforeShutdownRunAfterShutdown_BlockShutdown) { I'm not sure this test should be here as it's not really testing anything. I can see why it would be here though. Maybe a comment without the test annotation is sufficient. https://codereview.chromium.org/1705943002/diff/100001/base/test/task_runner_... File base/test/task_runner_test_template.h (right): https://codereview.chromium.org/1705943002/diff/100001/base/test/task_runner_... base/test/task_runner_test_template.h:75: namespace testing { Since this is base/test, this should probably be namespace test. https://code.google.com/p/chromium/codesearch#search/&q=namespace%5C%20test%2...
https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/ta... File base/task_scheduler/task_tracker_unittest.cc (right): https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:376: } On 2016/03/01 22:18:47, gab wrote: > To avoid the triad duplication for every test, how about making > TaskSchedulerTaskTrackerTest a parametrized test with > testing::WithParamInterface<TaskShutdownBehavior>. > > Then you can do INSTANTIATE_TEST_CASE_P to get each test to run once in each > mode and use GetParam() to figure out which mode you're in right now to drive > the little difference in behavior and expectations for each mode? Duplication for tests should be okay. Given that we're only going to be writing these once, it's easier to see bugs in the expanded form.
https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/ta... File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/ta... base/task_scheduler/task_tracker.cc:50: task_annotator.DidQueueTask(kQueueFunctionName, *task.get()); On 2016/03/01 22:18:47, gab wrote: > Not your fault but noticing that "DidQueueTask" is a weird name as it > intuitively feels it should be called *before* queueing the task (or it may run > before it's called) and this is in fact what existing impls do... so it should > really be "WillPostTask"... Acknowledged. https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/ta... base/task_scheduler/task_tracker.cc:69: bool TaskTracker::RequestPostTask(TaskShutdownBehavior shutdown_behavior) { On 2016/03/01 22:29:41, robliao wrote: > Now that these are private, we can give them names that reflect when they're > called. > > RequestPostTask -> BeforeTaskPosted > RequestScheduleTask -> BeforeTaskExecuted > DidExecuteTask -> AfterTaskExecuted > Done. https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/ta... base/task_scheduler/task_tracker.cc:83: bool TaskTracker::RequestScheduleTask(TaskShutdownBehavior shutdown_behavior) { On 2016/03/01 22:18:47, gab wrote: > "RequestRunTask" to match nomenclature in rest of this class? robliao@ suggested different method names. RequestPostTask -> BeforeTaskPosted RequestScheduleTask -> BeforeTaskExecuted DidExecuteTask -> AfterTaskExecuted https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/ta... base/task_scheduler/task_tracker.cc:98: ++num_tasks_blocking_shutdown_; On 2016/03/01 22:29:41, robliao wrote: > It's worth commented that we want Shutdown to block on these tasks during > execution and we increment here since we don't want to include them in the > overall blocking count unless they're currently executing. Done. https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/ta... base/task_scheduler/task_tracker.cc:105: return false; On 2016/03/01 22:29:41, robliao wrote: > Might as well add a NOTREACHED() here. Done. https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/ta... File base/task_scheduler/task_tracker.h (right): https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/ta... base/task_scheduler/task_tracker.h:28: // Shuts down the task tracker. Once this is called, only tasks posted with On 2016/03/01 22:29:41, robliao wrote: > The blocking behavior is important enough to reemphasize. Let's add > Synchronously shutdown down the task tracker. Done. https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/ta... base/task_scheduler/task_tracker.h:36: // Posts |task| unless the current shutdown state prevents that. Posting a On 2016/03/01 22:29:41, robliao wrote: > Calls |post_task_callback| unless the current shutdown state prevents that. Done. https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/ta... base/task_scheduler/task_tracker.h:38: // tracing event and invoking |post_task_callback|. |post_task_callback| must On 2016/03/01 22:18:47, gab wrote: > ',' before "and" Done. (Conflicts with robliao@'s comment) https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/ta... base/task_scheduler/task_tracker.h:40: // shutdown_completed() returns true. On 2016/03/01 22:18:47, gab wrote: > s/are run until shutdown_completed() returns true/are handed off to this class' > RunTask() when they are to be executed/ > > It's important that those tasks make it back through RunTask() -- and it's not > otherwise relevant to this API when the pool exits. Done. https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/ta... base/task_scheduler/task_tracker.h:45: // have been successfully posted via PostTask(). On 2016/03/01 22:18:47, gab wrote: > s/./ first./ Done. https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/ta... File base/task_scheduler/task_tracker_unittest.cc (right): https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:16: namespace base { On 2016/03/01 22:29:42, robliao wrote: > I think you need a test case where we block shutdown while a > CONTINUE_ON_SHUTDOWN task is running. Done. PostAndRunBlockedContinueOnShutdownTaskBeforeShutdown https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:62: return posted_task; On 2016/03/01 22:18:47, gab wrote: > This feels very weird... how about instead have a fake queue here, e.g. > > std::dequeue task_queue_; accompanied by an AddToQueue() method used in the Bind > here? > > And tests can test the top of the queue instead of doing the raw pointer to > returned pointer comparison. Done. I now use an std::queue. https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:80: thread_calling_shutdown_->Join(); On 2016/03/01 22:29:42, robliao wrote: > It would be unexpected to join with the duality between > ExpectShutdownAsyncHas[Not]Returned. > Maybe JoinAndExpectShutdownAsyncHasReturned? Done. https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:92: size_t num_run_tasks_; On 2016/03/01 22:18:47, gab wrote: > num_tasks_executed_ ? Done. https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:120: EXPECT_EQ(1U, num_run_tasks_); On 2016/03/01 22:18:47, gab wrote: > EXPECT_EQ(0U, ...); as well before RunTask()? Done. https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:276: PostBeforeShutdownRunAfterShutdown_BlockShutdown) { On 2016/03/01 22:29:42, robliao wrote: > I'm not sure this test should be here as it's not really testing anything. I can > see why it would be here though. Maybe a comment without the test annotation is > sufficient. Now I test that we get the expected failure with EXPECT_DEBUG_DEATH. https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:279: // pending BLOCK_SHUTDOWN tasks. On 2016/03/01 22:18:47, gab wrote: > I think your code actually handles this (+ a DCHECK) so it should be test this > with a DEBUG_DEATH test, no? Done. https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:376: } On 2016/03/01 22:36:41, robliao wrote: > On 2016/03/01 22:18:47, gab wrote: > > To avoid the triad duplication for every test, how about making > > TaskSchedulerTaskTrackerTest a parametrized test with > > testing::WithParamInterface<TaskShutdownBehavior>. > > > > Then you can do INSTANTIATE_TEST_CASE_P to get each test to run once in each > > mode and use GetParam() to figure out which mode you're in right now to drive > > the little difference in behavior and expectations for each mode? > > Duplication for tests should be okay. Given that we're only going to be writing > these once, it's easier to see bugs in the expanded form. I think it's a good idea to use a typed test to reduce code duplication. It requires a lot of effort to try to find the differences between the tests for different shutdown behaviors. With a typed test, the differences become obvious. https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/ta... File base/task_scheduler/task_traits.h (right): https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/ta... base/task_scheduler/task_traits.h:139: BASE_EXPORT std::ostream& operator<<( On 2016/03/01 22:18:47, gab wrote: > Doesn't PrintTo() also work for this type as above for TaskPriority? Feels weird > to have this implemented two different ways for these two very similar types. Unfortunately no: doesn't compile if I implement PrintTo instead of the stream operator. https://codereview.chromium.org/1705943002/diff/100001/base/test/task_runner_... File base/test/task_runner_test_template.h (right): https://codereview.chromium.org/1705943002/diff/100001/base/test/task_runner_... base/test/task_runner_test_template.h:75: namespace testing { On 2016/03/01 22:29:42, robliao wrote: > Since this is base/test, this should probably be namespace test. > > https://code.google.com/p/chromium/codesearch#search/&q=namespace%5C%20test%2... TODO(fdoray): Do this in another CL before sending to reviewers.
Looked mostly at diff since last round. Will make a full pass after this. Thanks! Gab https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/ta... File base/task_scheduler/task_tracker_unittest.cc (right): https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:376: } On 2016/03/02 00:38:41, fdoray wrote: > On 2016/03/01 22:36:41, robliao wrote: > > On 2016/03/01 22:18:47, gab wrote: > > > To avoid the triad duplication for every test, how about making > > > TaskSchedulerTaskTrackerTest a parametrized test with > > > testing::WithParamInterface<TaskShutdownBehavior>. > > > > > > Then you can do INSTANTIATE_TEST_CASE_P to get each test to run once in each > > > mode and use GetParam() to figure out which mode you're in right now to > drive > > > the little difference in behavior and expectations for each mode? > > > > Duplication for tests should be okay. Given that we're only going to be > writing > > these once, it's easier to see bugs in the expanded form. > > I think it's a good idea to use a typed test to reduce code duplication. It > requires a lot of effort to try to find the differences between the tests for > different shutdown behaviors. With a typed test, the differences become obvious. +1 to Francois' argument, readability is improved when the difference is highlighted + less code duplication == win/win :-) https://codereview.chromium.org/1705943002/diff/180001/base/task_scheduler/ta... File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/1705943002/diff/180001/base/task_scheduler/ta... base/task_scheduler/task_tracker.cc:101: // SKIP_ON_SHUTDOWN tasks block shutdown when they are running. s/when/while/ https://codereview.chromium.org/1705943002/diff/180001/base/task_scheduler/ta... File base/task_scheduler/task_tracker.h (right): https://codereview.chromium.org/1705943002/diff/180001/base/task_scheduler/ta... base/task_scheduler/task_tracker.h:38: // handed off to this class' RunTask() when it is to be executed. s/handed off/handed back/ https://codereview.chromium.org/1705943002/diff/180001/base/task_scheduler/ta... File base/task_scheduler/task_tracker_unittest.cc (right): https://codereview.chromium.org/1705943002/diff/180001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:48: public ::testing::WithParamInterface<TaskShutdownBehavior> { You can use testing::TestWithParam<TaskShutdownBehavior> instead of both ::Test and ::WithParamInterface. https://codereview.chromium.org/1705943002/diff/180001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:50: TaskSchedulerTaskTrackerTest() : num_tasks_executed_(0) {} Can initialize member inline now with C++11 :-) (same for |has_returned_| above) I tend to prefer that as it's clearer in a single read that it's initialized and what it's initialized to. https://codereview.chromium.org/1705943002/diff/180001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:68: void RunNextPostedTaskViaTracker() { All methods but this one can move to "protected" I think (i.e. can only use in this class and the test bodies). https://codereview.chromium.org/1705943002/diff/180001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:87: void JoinAndExpectShutdownAsyncHasReturned() { JoinAndExpectAsyncShutdownCompleted() ? https://codereview.chromium.org/1705943002/diff/180001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:94: void ExpectShutdownAsyncHasNotReturned() { VerifyAsyncShutdownInProgress()? https://codereview.chromium.org/1705943002/diff/180001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:96: EXPECT_FALSE(thread_calling_shutdown_->has_returned()); Add EXPECT_TRUE(tracker_.is_shutting_down_for_testing()) ? https://codereview.chromium.org/1705943002/diff/180001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:163: ASSERT_FALSE(posted_tasks_.empty()); Even better than verifying !empty() IMO here and elsewhere would be verifying exact size(). https://codereview.chromium.org/1705943002/diff/180001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:229: } I'm surprised mixing TEST_F with TEST_P even works. I still prefer merging the above 3 tests into 1 as: TEST_P(TaskSchedulerTaskTrackerTest, PostAndRunLongTaskBeforeShutdown) { // Post a CONTINUE_ON_SHUTDOWN task that will block until |event| is signaled. EXPECT_TRUE(posted_tasks_.empty()); WaitableEvent event(false, false); PostTaskViaTracker(make_scoped_ptr( new Task(FROM_HERE, Bind(&WaitableEvent::Wait, Unretained(&event)), TaskTraits().WithShutdownBehavior(GetParam())))); ASSERT_FALSE(posted_tasks_.empty()); // Run the task asynchronouly. ThreadRunningNextPostedTask thread(this); thread.Start(); if (GetParam() == TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN) { // Shutdown() shouldn't block with CONTINUE_ON_SHUTDOWN tasks in progress. tracker_.Shutdown(); } else { // Shutdown() should block with any non CONTINUE_ON_SHUTDOWN task in // progress. CallShutdownAsync(); ExpectShutdownAsyncHasNotReturned(); } // Unblock the task. event.Signal(); thread.Join(); if (GetParam() != TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN) { // Shutdown() should now return. JoinAndExpectShutdownAsyncHasReturned(); } } and even maybe for the sake of uniformizing, always call shutdown async and have ThreadCallingShutdown fire an event when shutdown completes then this can become: TEST_P(TaskSchedulerTaskTrackerTest, PostAndRunLongTaskBeforeShutdown) { // Post a CONTINUE_ON_SHUTDOWN task that will block until |event| is signaled. EXPECT_TRUE(posted_tasks_.empty()); WaitableEvent event(false, false); PostTaskViaTracker(make_scoped_ptr( new Task(FROM_HERE, Bind(&WaitableEvent::Wait, Unretained(&event)), TaskTraits().WithShutdownBehavior(GetParam())))); ASSERT_FALSE(posted_tasks_.empty()); // Run the task asynchronouly. ThreadRunningNextPostedTask thread(this); thread.Start(); // Initiate shutdown while the task is running. CallShutdownAsync(); if (GetParam() == TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN) { // Shutdown should complete even with a CONTINUE_ON_SHUTDOWN in progress. WaitForShutdownCompleted(); } else { // Shutdown should block with any non CONTINUE_ON_SHUTDOWN task in progress. ExpectShutdownAsyncHasNotReturned(); } // Unblock the task. event.Signal(); thread.Join(); // Shutdown should complete in all cases now. JoinAndExpectShutdownAsyncHasReturned(); } where WaitForShutdownCompleted() waits on a WaitableEvent(false, false) fired right after Shutdown() returns -- and actually that event can replace the bool that Shutdown() has returned via its IsSignaled() method :-) https://codereview.chromium.org/1705943002/diff/180001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:231: TEST_F(TaskSchedulerTaskTrackerTest, Same thing as above for all remaining TEST_F's, try to fold all of these into single parametrized tests, per your own argument : readability is improved when the difference between each tests is clear and I find parametrized tests help with this :-). https://codereview.chromium.org/1705943002/diff/180001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:468: ::testing::Values(TaskShutdownBehavior::BLOCK_SHUTDOWN)); Oh nice, first time I see someone do it this way (3 instances, one per value -- I typically only use one test instance and have a list of values instead, e.g.: https://code.google.com/p/chromium/codesearch#chromium/src/components/user_pr...), but I find your approach cleaner so W00T! https://codereview.chromium.org/1705943002/diff/180001/base/test/task_runner_... File base/test/task_runner_test_template.h (right): https://codereview.chromium.org/1705943002/diff/180001/base/test/task_runner_... base/test/task_runner_test_template.h:75: namespace testing { Missing rebase? Thought this change was made upstream already?
https://codereview.chromium.org/1705943002/diff/180001/base/task_scheduler/ta... File base/task_scheduler/task_tracker_unittest.cc (right): https://codereview.chromium.org/1705943002/diff/180001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:135: TEST_P(TaskSchedulerTaskTrackerTest, PostAndRunBeforeShutdown) { If the parametrized tests are like the ones here (no branches), I'm okay with using them here. Once they start having branches, we'll need to consider them on a case-by-case basis.
Description was changed from ========== TaskScheduler [5/8] Task Tracker This change is a subset of https://codereview.chromium.org/1698183005/ We have an alternate implementation which uses atomic operations to reduce lock contention. However, we prefer to check this simple implementation in first and bring the atomic operations in an incremental CL. BUG=553459 ========== to ========== TaskScheduler [5/9] Task Tracker This change is a subset of https://codereview.chromium.org/1698183005/ We have an alternate implementation which uses atomic operations to reduce lock contention. However, we prefer to check this simple implementation in first and bring the atomic operations in an incremental CL. BUG=553459 ==========
https://codereview.chromium.org/1705943002/diff/180001/base/task_scheduler/ta... File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/1705943002/diff/180001/base/task_scheduler/ta... base/task_scheduler/task_tracker.cc:101: // SKIP_ON_SHUTDOWN tasks block shutdown when they are running. On 2016/03/09 21:53:25, gab wrote: > s/when/while/ Done. https://codereview.chromium.org/1705943002/diff/180001/base/task_scheduler/ta... File base/task_scheduler/task_tracker.h (right): https://codereview.chromium.org/1705943002/diff/180001/base/task_scheduler/ta... base/task_scheduler/task_tracker.h:38: // handed off to this class' RunTask() when it is to be executed. On 2016/03/09 21:53:25, gab wrote: > s/handed off/handed back/ Done. https://codereview.chromium.org/1705943002/diff/180001/base/task_scheduler/ta... File base/task_scheduler/task_tracker_unittest.cc (right): https://codereview.chromium.org/1705943002/diff/180001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:48: public ::testing::WithParamInterface<TaskShutdownBehavior> { On 2016/03/09 21:53:25, gab wrote: > You can use testing::TestWithParam<TaskShutdownBehavior> instead of both ::Test > and ::WithParamInterface. Done. https://codereview.chromium.org/1705943002/diff/180001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:50: TaskSchedulerTaskTrackerTest() : num_tasks_executed_(0) {} On 2016/03/09 21:53:25, gab wrote: > Can initialize member inline now with C++11 :-) > > (same for |has_returned_| above) > > I tend to prefer that as it's clearer in a single read that it's initialized and > what it's initialized to. Done. https://codereview.chromium.org/1705943002/diff/180001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:68: void RunNextPostedTaskViaTracker() { On 2016/03/09 21:53:25, gab wrote: > All methods but this one can move to "protected" I think (i.e. can only use in > this class and the test bodies). Done. https://codereview.chromium.org/1705943002/diff/180001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:87: void JoinAndExpectShutdownAsyncHasReturned() { On 2016/03/09 21:53:25, gab wrote: > JoinAndExpectAsyncShutdownCompleted() ? Done. Called it WaitForAsyncShutdownCompleted as you suggested in another of your comments. https://codereview.chromium.org/1705943002/diff/180001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:94: void ExpectShutdownAsyncHasNotReturned() { On 2016/03/09 21:53:25, gab wrote: > VerifyAsyncShutdownInProgress()? Done. https://codereview.chromium.org/1705943002/diff/180001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:96: EXPECT_FALSE(thread_calling_shutdown_->has_returned()); On 2016/03/09 21:53:25, gab wrote: > Add EXPECT_TRUE(tracker_.is_shutting_down_for_testing()) ? Done. https://codereview.chromium.org/1705943002/diff/180001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:135: TEST_P(TaskSchedulerTaskTrackerTest, PostAndRunBeforeShutdown) { On 2016/03/09 22:31:46, robliao wrote: > If the parametrized tests are like the ones here (no branches), I'm okay with > using them here. > > Once they start having branches, we'll need to consider them on a case-by-case > basis. Every test is parameterized now. https://codereview.chromium.org/1705943002/diff/180001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:163: ASSERT_FALSE(posted_tasks_.empty()); On 2016/03/09 21:53:25, gab wrote: > Even better than verifying !empty() IMO here and elsewhere would be verifying > exact size(). Done. https://codereview.chromium.org/1705943002/diff/180001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:229: } On 2016/03/09 21:53:25, gab wrote: > I'm surprised mixing TEST_F with TEST_P even works. > > I still prefer merging the above 3 tests into 1 as: > > TEST_P(TaskSchedulerTaskTrackerTest, PostAndRunLongTaskBeforeShutdown) { > // Post a CONTINUE_ON_SHUTDOWN task that will block until |event| is signaled. > EXPECT_TRUE(posted_tasks_.empty()); > WaitableEvent event(false, false); > PostTaskViaTracker(make_scoped_ptr( > new Task(FROM_HERE, Bind(&WaitableEvent::Wait, Unretained(&event)), > TaskTraits().WithShutdownBehavior(GetParam())))); > ASSERT_FALSE(posted_tasks_.empty()); > > // Run the task asynchronouly. > ThreadRunningNextPostedTask thread(this); > thread.Start(); > > if (GetParam() == TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN) { > // Shutdown() shouldn't block with CONTINUE_ON_SHUTDOWN tasks in progress. > tracker_.Shutdown(); > } else { > // Shutdown() should block with any non CONTINUE_ON_SHUTDOWN task in > // progress. > CallShutdownAsync(); > ExpectShutdownAsyncHasNotReturned(); > } > > // Unblock the task. > event.Signal(); > thread.Join(); > > if (GetParam() != TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN) { > // Shutdown() should now return. > JoinAndExpectShutdownAsyncHasReturned(); > } > } > > > and even maybe for the sake of uniformizing, always call shutdown async and have > ThreadCallingShutdown fire an event when shutdown completes then this can > become: > > TEST_P(TaskSchedulerTaskTrackerTest, PostAndRunLongTaskBeforeShutdown) { > // Post a CONTINUE_ON_SHUTDOWN task that will block until |event| is signaled. > EXPECT_TRUE(posted_tasks_.empty()); > WaitableEvent event(false, false); > PostTaskViaTracker(make_scoped_ptr( > new Task(FROM_HERE, Bind(&WaitableEvent::Wait, Unretained(&event)), > TaskTraits().WithShutdownBehavior(GetParam())))); > ASSERT_FALSE(posted_tasks_.empty()); > > // Run the task asynchronouly. > ThreadRunningNextPostedTask thread(this); > thread.Start(); > > // Initiate shutdown while the task is running. > CallShutdownAsync(); > > if (GetParam() == TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN) { > // Shutdown should complete even with a CONTINUE_ON_SHUTDOWN in progress. > WaitForShutdownCompleted(); > } else { > // Shutdown should block with any non CONTINUE_ON_SHUTDOWN task in progress. > ExpectShutdownAsyncHasNotReturned(); > } > > // Unblock the task. > event.Signal(); > thread.Join(); > > // Shutdown should complete in all cases now. > JoinAndExpectShutdownAsyncHasReturned(); > } > > > where WaitForShutdownCompleted() waits on a WaitableEvent(false, false) fired > right after Shutdown() returns -- and actually that event can replace the bool > that Shutdown() has returned via its IsSignaled() method :-) Done. https://codereview.chromium.org/1705943002/diff/180001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:231: TEST_F(TaskSchedulerTaskTrackerTest, On 2016/03/09 21:53:25, gab wrote: > Same thing as above for all remaining TEST_F's, try to fold all of these into > single parametrized tests, per your own argument : readability is improved when > the difference between each tests is clear and I find parametrized tests help > with this :-). Done. https://codereview.chromium.org/1705943002/diff/180001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:468: ::testing::Values(TaskShutdownBehavior::BLOCK_SHUTDOWN)); On 2016/03/09 21:53:26, gab wrote: > Oh nice, first time I see someone do it this way (3 instances, one per value -- > I typically only use one test instance and have a list of values instead, e.g.: > https://code.google.com/p/chromium/codesearch#chromium/src/components/user_pr...), > but I find your approach cleaner so W00T! Acknowledged. https://codereview.chromium.org/1705943002/diff/180001/base/test/task_runner_... File base/test/task_runner_test_template.h (right): https://codereview.chromium.org/1705943002/diff/180001/base/test/task_runner_... base/test/task_runner_test_template.h:75: namespace testing { On 2016/03/09 21:53:26, gab wrote: > Missing rebase? Thought this change was made upstream already? Yes, missing rebase. I will rebase everything after my Task/Sequence CL gets reviewed.
lvgtm :-), mostly nits below https://codereview.chromium.org/1705943002/diff/200001/base/task_scheduler/ta... File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/1705943002/diff/200001/base/task_scheduler/ta... base/task_scheduler/task_tracker.cc:18: : cv_(lock_.CreateConditionVariable()), Since we only need |cv_| during shutdown. How about we only set it then and then use |cv_|'s existence as an indicator of |is_shutting_down| (i.e. getting rid of that boolean). And probably rename |cv_| to |shutdown_cv_| now that this class is about more than just shutdown (especially if using it to replace |is_shutting_down|). https://codereview.chromium.org/1705943002/diff/200001/base/task_scheduler/ta... base/task_scheduler/task_tracker.cc:44: DCHECK(task.get()); I don't think .get() is necessary here, scoped_ptr implements operator(bool) IIRC. https://codereview.chromium.org/1705943002/diff/200001/base/task_scheduler/ta... base/task_scheduler/task_tracker.cc:89: DCHECK_NE(TaskShutdownBehavior::BLOCK_SHUTDOWN, shutdown_behavior); Add a comment like: // A BLOCK_SHUTDOWN task posted after shutdown has completed is an ordering bug. This DCHECK aims to catch those early. https://codereview.chromium.org/1705943002/diff/200001/base/task_scheduler/ta... File base/task_scheduler/task_tracker.h (right): https://codereview.chromium.org/1705943002/diff/200001/base/task_scheduler/ta... base/task_scheduler/task_tracker.h:20: // All tasks of the scheduler go through a TaskTracker when they are posted and s/a TaskTracker/the TaskTracker/ https://codereview.chromium.org/1705943002/diff/200001/base/task_scheduler/ta... base/task_scheduler/task_tracker.h:32: // - All posted BLOCK_SHUTDOWN tasks have completed their execution. Should we add a hard cap on the number of BLOCK_SHUTDOWN tasks we'll execute? SequencedWorkerPool does this, I don't know if it's ever hit in practice. https://codereview.chromium.org/1705943002/diff/200001/base/task_scheduler/ta... base/task_scheduler/task_tracker.h:44: void RunTask(const Task* task); I was under the impression this would be a scoped_ptr as it essentially now owns running the Task after which the Task should no longer be really useful to anyone (or is it?). If this is kept as a raw pointer then s/handed back/passed back/ in PostTask()'s comment. https://codereview.chromium.org/1705943002/diff/200001/base/task_scheduler/ta... base/task_scheduler/task_tracker.h:58: // |post_task_callback| by PostTask(). Updates |num_tasks_blocking_shutdown_| s/Updates |num_tasks_blocking_shutdown_|/Updates |num_tasks_blocking_shutdown_| if necessary/ (same for other 2 instances below) https://codereview.chromium.org/1705943002/diff/200001/base/task_scheduler/ta... File base/task_scheduler/task_tracker_unittest.cc (right): https://codereview.chromium.org/1705943002/diff/200001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:140: ASSERT_EQ(1, posted_tasks_.size()); 1U (prefer explicitly unsigned constants to compare sizes because that's what they are anyways) https://codereview.chromium.org/1705943002/diff/200001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:202: // succeed for a BLOCK_SHUTDOWN task. s/It should only succeed for a BLOCK_SHUTDOWN task./Only BLOCK_SHUTDOWN tasks should run, others should be discarded./ (i.e. "success" is not what's tested but rather whether it ran or not) https://codereview.chromium.org/1705943002/diff/200001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:240: EXPECT_DEATH( Use EXPECT_DCHECK_DEATH macro from Rob's SchedulerLock unittests, probably worth having our own little test utils, or maybe this is even a macro that makes sense in base's overall test_utils. https://codereview.chromium.org/1705943002/diff/200001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:243: CreateTask(TaskShutdownBehavior::BLOCK_SHUTDOWN).get()); Since that task was never posted to the tracker, it's not clear whether this should crash because the task needs to be posted before its ran or because running tasks after shutdown isn't allowed. https://codereview.chromium.org/1705943002/diff/200001/base/task_scheduler/ta... File base/task_scheduler/task_traits.h (right): https://codereview.chromium.org/1705943002/diff/200001/base/task_scheduler/ta... base/task_scheduler/task_traits.h:139: // Stream operator so TaskPriority can be used in assertion statements. s/TaskPriority/TaskShutdownBehavior/ (I still don't understand why this is operator<< and TaskPriority is PrintTo()? Are they not asserted the same way?)
Description was changed from ========== TaskScheduler [5/9] Task Tracker This change is a subset of https://codereview.chromium.org/1698183005/ We have an alternate implementation which uses atomic operations to reduce lock contention. However, we prefer to check this simple implementation in first and bring the atomic operations in an incremental CL. BUG=553459 ========== to ========== TaskScheduler [5/9] Task Tracker This change is a subset of https://codereview.chromium.org/1698183005/ All tasks of the scheduler go through a TaskTracker when they are posted and when they are executed. The TaskTracker enforces shutdown semantics and takes care of tracing and profiling. We have an alternate implementation which uses atomic operations to reduce lock contention. However, we prefer to check this simple implementation in first and bring the atomic operations in an incremental CL. BUG=553459 ==========
gab@: All done. https://codereview.chromium.org/1705943002/diff/200001/base/task_scheduler/ta... File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/1705943002/diff/200001/base/task_scheduler/ta... base/task_scheduler/task_tracker.cc:18: : cv_(lock_.CreateConditionVariable()), On 2016/03/17 01:42:12, gab wrote: > Since we only need |cv_| during shutdown. How about we only set it then and then > use |cv_|'s existence as an indicator of |is_shutting_down| (i.e. getting rid of > that boolean). > > And probably rename |cv_| to |shutdown_cv_| now that this class is about more > than just shutdown (especially if using it to replace |is_shutting_down|). Done. https://codereview.chromium.org/1705943002/diff/200001/base/task_scheduler/ta... base/task_scheduler/task_tracker.cc:44: DCHECK(task.get()); On 2016/03/17 01:42:12, gab wrote: > I don't think .get() is necessary here, scoped_ptr implements operator(bool) > IIRC. Done. https://codereview.chromium.org/1705943002/diff/200001/base/task_scheduler/ta... base/task_scheduler/task_tracker.cc:89: DCHECK_NE(TaskShutdownBehavior::BLOCK_SHUTDOWN, shutdown_behavior); On 2016/03/17 01:42:12, gab wrote: > Add a comment like: > > // A BLOCK_SHUTDOWN task posted after shutdown has completed is an ordering bug. > This DCHECK aims to catch those early. Done. https://codereview.chromium.org/1705943002/diff/200001/base/task_scheduler/ta... File base/task_scheduler/task_tracker.h (right): https://codereview.chromium.org/1705943002/diff/200001/base/task_scheduler/ta... base/task_scheduler/task_tracker.h:20: // All tasks of the scheduler go through a TaskTracker when they are posted and On 2016/03/17 01:42:12, gab wrote: > s/a TaskTracker/the TaskTracker/ Done. https://codereview.chromium.org/1705943002/diff/200001/base/task_scheduler/ta... base/task_scheduler/task_tracker.h:32: // - All posted BLOCK_SHUTDOWN tasks have completed their execution. On 2016/03/17 01:42:12, gab wrote: > Should we add a hard cap on the number of BLOCK_SHUTDOWN tasks we'll execute? > SequencedWorkerPool does this, I don't know if it's ever hit in practice. I think that we should start by adding a UMA metric to evaluate the number of BLOCK_SHUTDOWN tasks that are typically posted during shutdown. Then, we can add a hard a cap + log UMA metric when it is hit to be warned if ever this number increases. WDYT? I added a TODO for the UMA metric in the .cc file. From SequencedWorkerPool: // The goal is to make it impossible for chrome to 'infinite loop' during // shutdown, but to reasonably expect that all BLOCKING_SHUTDOWN tasks queued // during shutdown get run. There's nothing particularly scientific about the // number chosen. https://codereview.chromium.org/1705943002/diff/200001/base/task_scheduler/ta... base/task_scheduler/task_tracker.h:44: void RunTask(const Task* task); On 2016/03/17 01:42:12, gab wrote: > I was under the impression this would be a scoped_ptr as it essentially now owns > running the Task after which the Task should no longer be really useful to > anyone (or is it?). > > If this is kept as a raw pointer then s/handed back/passed back/ in PostTask()'s > comment. Unfortunately, our task scheduling algorithm requires that the task remain in its sequence while it is run. We can't use a scoped_ptr here because the task is owned by the sequence. https://codereview.chromium.org/1705943002/diff/200001/base/task_scheduler/ta... base/task_scheduler/task_tracker.h:58: // |post_task_callback| by PostTask(). Updates |num_tasks_blocking_shutdown_| On 2016/03/17 01:42:12, gab wrote: > s/Updates |num_tasks_blocking_shutdown_|/Updates |num_tasks_blocking_shutdown_| > if necessary/ > > (same for other 2 instances below) Done. https://codereview.chromium.org/1705943002/diff/200001/base/task_scheduler/ta... File base/task_scheduler/task_tracker_unittest.cc (right): https://codereview.chromium.org/1705943002/diff/200001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:140: ASSERT_EQ(1, posted_tasks_.size()); On 2016/03/17 01:42:12, gab wrote: > 1U (prefer explicitly unsigned constants to compare sizes because that's what > they are anyways) Done. https://codereview.chromium.org/1705943002/diff/200001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:202: // succeed for a BLOCK_SHUTDOWN task. On 2016/03/17 01:42:13, gab wrote: > s/It should only succeed for a BLOCK_SHUTDOWN task./Only BLOCK_SHUTDOWN tasks > should run, others should be discarded./ > > (i.e. "success" is not what's tested but rather whether it ran or not) Done. https://codereview.chromium.org/1705943002/diff/200001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:240: EXPECT_DEATH( On 2016/03/17 01:42:13, gab wrote: > Use EXPECT_DCHECK_DEATH macro from Rob's SchedulerLock unittests, probably worth > having our own little test utils, or maybe this is even a macro that makes sense > in base's overall test_utils. Done. https://codereview.chromium.org/1705943002/diff/200001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:243: CreateTask(TaskShutdownBehavior::BLOCK_SHUTDOWN).get()); On 2016/03/17 01:42:12, gab wrote: > Since that task was never posted to the tracker, it's not clear whether this > should crash because the task needs to be posted before its ran or because > running tasks after shutdown isn't allowed. There is no way to test running after shutdown a BLOCK_SHUTDOWN task that has been posted before shutdown because Shutdown() won't return if there are pending BLOCK_SHUTDOWN tasks. You recommended to test this failure here https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/ta... https://codereview.chromium.org/1705943002/diff/200001/base/task_scheduler/ta... File base/task_scheduler/task_traits.h (right): https://codereview.chromium.org/1705943002/diff/200001/base/task_scheduler/ta... base/task_scheduler/task_traits.h:139: // Stream operator so TaskPriority can be used in assertion statements. On 2016/03/17 01:42:13, gab wrote: > s/TaskPriority/TaskShutdownBehavior/ > > (I still don't understand why this is operator<< and TaskPriority is PrintTo()? > Are they not asserted the same way?) TaskPriority is used in Google Test (EXPECT_EQ) while TaskShutdownBehavior is used in a DCHECK_NE. Google Test supports both PrintTo and operator<< while DCHECK supports operator<< only. I changed the code to use operator<< all the time.
https://codereview.chromium.org/1705943002/diff/240001/base/task_scheduler/ta... File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/1705943002/diff/240001/base/task_scheduler/ta... base/task_scheduler/task_tracker.cc:81: // TODO(fdoray): Add a UMA histogram with the number of BLOCK_SHUTDOWN tasks Should we go ahead and do this now? https://codereview.chromium.org/1705943002/diff/240001/base/task_scheduler/ta... File base/task_scheduler/task_tracker.h (right): https://codereview.chromium.org/1705943002/diff/240001/base/task_scheduler/ta... base/task_scheduler/task_tracker.h:28: // Synchronously shuts down the task tracker. Once this is called, only tasks I would go further and say this shuts down the scheduler. https://codereview.chromium.org/1705943002/diff/240001/base/task_scheduler/ta... base/task_scheduler/task_tracker.h:38: // handed back to this class' RunTask() when it is to be executed. Nit: class' -> instance's https://codereview.chromium.org/1705943002/diff/240001/base/task_scheduler/ta... File base/task_scheduler/task_tracker_unittest.cc (right): https://codereview.chromium.org/1705943002/diff/240001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:139: EXPECT_TRUE(posted_tasks_.empty()); This should probably be an ASSERT_TRUE and the next ASSERT_EQ( should be an EXPECT_EQ. http://googletesting.blogspot.com/2008/07/tott-expect-vs-assert.html https://codereview.chromium.org/1705943002/diff/240001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:225: ASSERT_EQ(task_to_post_raw, posted_tasks_.front().get()); Scrub the usages of ASSERT vs EXPECT as well. This specific check is sometimes EXPECT above.
lgtm :-) w/ nits https://codereview.chromium.org/1705943002/diff/200001/base/task_scheduler/ta... File base/task_scheduler/task_traits.h (right): https://codereview.chromium.org/1705943002/diff/200001/base/task_scheduler/ta... base/task_scheduler/task_traits.h:139: // Stream operator so TaskPriority can be used in assertion statements. On 2016/03/17 20:12:16, fdoray wrote: > On 2016/03/17 01:42:13, gab wrote: > > s/TaskPriority/TaskShutdownBehavior/ > > > > (I still don't understand why this is operator<< and TaskPriority is > PrintTo()? > > Are they not asserted the same way?) > > TaskPriority is used in Google Test (EXPECT_EQ) while TaskShutdownBehavior is > used in a DCHECK_NE. Google Test supports both PrintTo and operator<< while > DCHECK supports operator<< only. I changed the code to use operator<< all the > time. Got it thanks, the now more consistent approach is less confusing :-). https://codereview.chromium.org/1705943002/diff/240001/base/task_scheduler/ta... File base/task_scheduler/task_tracker.h (right): https://codereview.chromium.org/1705943002/diff/240001/base/task_scheduler/ta... base/task_scheduler/task_tracker.h:76: // zero while shutdown is in progress. Add: "null if shutdown isn't in progress" as the impl depends on that. https://codereview.chromium.org/1705943002/diff/240001/base/task_scheduler/ta... File base/task_scheduler/task_tracker_unittest.cc (right): https://codereview.chromium.org/1705943002/diff/240001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:239: // It is not possible to test running after shutdown a BLOCK_SHUTDOWN task I think posting a BLOCK_SHUTDOWN task after shutdown should also DCHECK though. Given the intent is to catch anybody posting shutdown blocking work after shutdown we need to catch it on post more than execution. So we should still keep the DCHECK in execution because that's *really* unexpected but should also DCHECK in Post and this is what we should test here IMO.
https://codereview.chromium.org/1705943002/diff/240001/base/task_scheduler/ta... File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/1705943002/diff/240001/base/task_scheduler/ta... base/task_scheduler/task_tracker.cc:81: // TODO(fdoray): Add a UMA histogram with the number of BLOCK_SHUTDOWN tasks On 2016/03/17 23:34:21, robliao wrote: > Should we go ahead and do this now? Done. https://codereview.chromium.org/1705943002/diff/240001/base/task_scheduler/ta... File base/task_scheduler/task_tracker.h (right): https://codereview.chromium.org/1705943002/diff/240001/base/task_scheduler/ta... base/task_scheduler/task_tracker.h:28: // Synchronously shuts down the task tracker. Once this is called, only tasks On 2016/03/17 23:34:21, robliao wrote: > I would go further and say this shuts down the scheduler. Done. https://codereview.chromium.org/1705943002/diff/240001/base/task_scheduler/ta... base/task_scheduler/task_tracker.h:38: // handed back to this class' RunTask() when it is to be executed. On 2016/03/17 23:34:21, robliao wrote: > Nit: class' -> instance's Done. https://codereview.chromium.org/1705943002/diff/240001/base/task_scheduler/ta... base/task_scheduler/task_tracker.h:76: // zero while shutdown is in progress. On 2016/03/18 18:48:17, gab wrote: > Add: "null if shutdown isn't in progress" as the impl depends on that. Done. https://codereview.chromium.org/1705943002/diff/240001/base/task_scheduler/ta... File base/task_scheduler/task_tracker_unittest.cc (right): https://codereview.chromium.org/1705943002/diff/240001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:139: EXPECT_TRUE(posted_tasks_.empty()); On 2016/03/17 23:34:21, robliao wrote: > This should probably be an ASSERT_TRUE and the next ASSERT_EQ( should be an > EXPECT_EQ. > > http://googletesting.blogspot.com/2008/07/tott-expect-vs-assert.html It is useful to continue running the test even if |posted_tasks_| is non-empty. The checks at lines 145-147 should succeed. Therefore, I think that EXPECT is more appropriate than ASSERT for line 139. I agree with you that EXPECT is more appropriate at line 141. https://codereview.chromium.org/1705943002/diff/240001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:225: ASSERT_EQ(task_to_post_raw, posted_tasks_.front().get()); On 2016/03/17 23:34:21, robliao wrote: > Scrub the usages of ASSERT vs EXPECT as well. This specific check is sometimes > EXPECT above. Done. https://codereview.chromium.org/1705943002/diff/240001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:239: // It is not possible to test running after shutdown a BLOCK_SHUTDOWN task On 2016/03/18 18:48:17, gab wrote: > I think posting a BLOCK_SHUTDOWN task after shutdown should also DCHECK though. > Given the intent is to catch anybody posting shutdown blocking work after > shutdown we need to catch it on post more than execution. > > So we should still keep the DCHECK in execution because that's *really* > unexpected but should also DCHECK in Post and this is what we should test here > IMO. Failure when a BLOCK_SHUTDOWN task is posted after shutdown is tested in TaskSchedulerTaskTrackerTest.PostAfterShutdown. Should I keep the EXPECT_DCHECK_DEATH here to check failure when a task is *run* after shutdown?
Description was changed from ========== TaskScheduler [5/9] Task Tracker This change is a subset of https://codereview.chromium.org/1698183005/ All tasks of the scheduler go through a TaskTracker when they are posted and when they are executed. The TaskTracker enforces shutdown semantics and takes care of tracing and profiling. We have an alternate implementation which uses atomic operations to reduce lock contention. However, we prefer to check this simple implementation in first and bring the atomic operations in an incremental CL. BUG=553459 ========== to ========== TaskScheduler [5/9] Task Tracker This change is a subset of https://codereview.chromium.org/1698183005/ All tasks go through the scheduler's TaskTracker when they are posted and when they are executed. The TaskTracker enforces shutdown semantics and takes care of tracing and profiling. This class is thread-safe. We have an alternate implementation which uses atomic operations to reduce lock contention. However, we prefer to check this simple implementation in first and bring the atomic operations in an incremental CL. BUG=553459 ==========
Description was changed from ========== TaskScheduler [5/9] Task Tracker This change is a subset of https://codereview.chromium.org/1698183005/ All tasks go through the scheduler's TaskTracker when they are posted and when they are executed. The TaskTracker enforces shutdown semantics and takes care of tracing and profiling. This class is thread-safe. We have an alternate implementation which uses atomic operations to reduce lock contention. However, we prefer to check this simple implementation in first and bring the atomic operations in an incremental CL. BUG=553459 ========== to ========== TaskScheduler [5/9] Task Tracker This change is a subset of https://codereview.chromium.org/1698183005/ All tasks go through the scheduler's TaskTracker when they are posted and when they are executed. The TaskTracker enforces shutdown semantics and takes care of tracing and profiling. We have an alternate implementation which uses atomic operations to reduce lock contention. However, we prefer to check this simple implementation in first and bring the atomic operations in an incremental CL. BUG=553459 ==========
https://codereview.chromium.org/1705943002/diff/240001/base/task_scheduler/ta... File base/task_scheduler/task_tracker_unittest.cc (right): https://codereview.chromium.org/1705943002/diff/240001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:239: // It is not possible to test running after shutdown a BLOCK_SHUTDOWN task On 2016/03/18 20:35:39, fdoray wrote: > On 2016/03/18 18:48:17, gab wrote: > > I think posting a BLOCK_SHUTDOWN task after shutdown should also DCHECK > though. > > Given the intent is to catch anybody posting shutdown blocking work after > > shutdown we need to catch it on post more than execution. > > > > So we should still keep the DCHECK in execution because that's *really* > > unexpected but should also DCHECK in Post and this is what we should test here > > IMO. > > Failure when a BLOCK_SHUTDOWN task is posted after shutdown is tested in > TaskSchedulerTaskTrackerTest.PostAfterShutdown. Should I keep the > EXPECT_DCHECK_DEATH here to check failure when a task is *run* after shutdown? Ok thanks, see comment in product, not sure this is BLOCK_SHUTDOWN only. But yes, do test whatever the product code ends up implementing (with updated test comment). https://codereview.chromium.org/1705943002/diff/300001/base/task_scheduler/ta... File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/1705943002/diff/300001/base/task_scheduler/ta... base/task_scheduler/task_tracker.cc:39: num_tasks_blocking_shutdown_copy = num_tasks_blocking_shutdown_; UMA is extremely optimized, I don't think it's worth making a copy just to release the lock barely earlier. https://codereview.chromium.org/1705943002/diff/300001/base/task_scheduler/ta... base/task_scheduler/task_tracker.cc:42: UMA_HISTOGRAM_COUNTS_100( UMA_HISTOGRAM_COUNTS_100 sounds low given current 10000 restriction on BlockingPool. Also, this will not catch an infinite shutdown loop. How about using UMA_HISTOGRAM_COUNTS_CUSTOM with a self-defined max and also logging a metric if the max is busted during shutdown? https://codereview.chromium.org/1705943002/diff/300001/base/task_scheduler/ta... base/task_scheduler/task_tracker.cc:112: // bug. This DCHECK aims to catch those early. Posting is an ordering bug, running is just unexpected as it either shouldn't have been posted if shutdown completed or should be blocking shutdown if was posted before it did. Actually, shouldn't this just be NOTREACHED()? No task should ever request to run after shutdown completed, right? https://codereview.chromium.org/1705943002/diff/300001/base/task_scheduler/te... File base/task_scheduler/test_utils.h (right): https://codereview.chromium.org/1705943002/diff/300001/base/task_scheduler/te... base/task_scheduler/test_utils.h:16: #define EXPECT_DCHECK_DEATH(statement, regex) statement Thought you'd convinced me not to do this? What changed? https://codereview.chromium.org/1705943002/diff/300001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1705943002/diff/300001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:53009: + units="Tasks"> I don't see "Tasks" as a defined unit type in histograms.xml? Or do integer types not need to be explicitly defined?
fdoray@chromium.org changed reviewers: + asvitkine@chromium.org, danakj@chromium.org
Dana: Can you review this CL? Alexei: Can you review histograms.xml + lines 43-45 of task_tracker.cc (where the histogram is logged). Thanks!
Dana: I just received comments from Gab. You can wait until they are addressed to review the CL. Thanks.
lgtm https://codereview.chromium.org/1705943002/diff/340001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1705943002/diff/340001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:53146: + units="Tasks"> Nit: No need to capitalize units.
gab@: Can you take another look? https://codereview.chromium.org/1705943002/diff/240001/base/task_scheduler/ta... File base/task_scheduler/task_tracker_unittest.cc (right): https://codereview.chromium.org/1705943002/diff/240001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:239: // It is not possible to test running after shutdown a BLOCK_SHUTDOWN task On 2016/03/21 17:42:30, gab wrote: > On 2016/03/18 20:35:39, fdoray wrote: > > On 2016/03/18 18:48:17, gab wrote: > > > I think posting a BLOCK_SHUTDOWN task after shutdown should also DCHECK > > though. > > > Given the intent is to catch anybody posting shutdown blocking work after > > > shutdown we need to catch it on post more than execution. > > > > > > So we should still keep the DCHECK in execution because that's *really* > > > unexpected but should also DCHECK in Post and this is what we should test > here > > > IMO. > > > > Failure when a BLOCK_SHUTDOWN task is posted after shutdown is tested in > > TaskSchedulerTaskTrackerTest.PostAfterShutdown. Should I keep the > > EXPECT_DCHECK_DEATH here to check failure when a task is *run* after shutdown? > > Ok thanks, see comment in product, not sure this is BLOCK_SHUTDOWN only. But > yes, do test whatever the product code ends up implementing (with updated test > comment). I think it's BLOCK_SHUTDOWN only (see product). I kept the test as-is. https://codereview.chromium.org/1705943002/diff/300001/base/task_scheduler/ta... File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/1705943002/diff/300001/base/task_scheduler/ta... base/task_scheduler/task_tracker.cc:39: num_tasks_blocking_shutdown_copy = num_tasks_blocking_shutdown_; On 2016/03/21 17:42:30, gab wrote: > UMA is extremely optimized, I don't think it's worth making a copy just to > release the lock barely earlier. Done because it makes the code simpler. However, Dana asked me to release the Sequence's lock before constructing+returning a SequenceSortKey here https://codereview.chromium.org/1705253002/diff/360001/base/task_scheduler/se.... Maybe she'll ask me to do a similar optimization here. https://codereview.chromium.org/1705943002/diff/300001/base/task_scheduler/ta... base/task_scheduler/task_tracker.cc:42: UMA_HISTOGRAM_COUNTS_100( On 2016/03/21 17:42:30, gab wrote: > UMA_HISTOGRAM_COUNTS_100 sounds low given current 10000 restriction on > BlockingPool. > > Also, this will not catch an infinite shutdown loop. > > How about using UMA_HISTOGRAM_COUNTS_CUSTOM with a self-defined max and also > logging a metric if the max is busted during shutdown? Done. The current restriction for BlockingPool is 1000 I believe (https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/br...). https://codereview.chromium.org/1705943002/diff/300001/base/task_scheduler/ta... base/task_scheduler/task_tracker.cc:112: // bug. This DCHECK aims to catch those early. On 2016/03/21 17:42:30, gab wrote: > Posting is an ordering bug, running is just unexpected as it either shouldn't > have been posted if shutdown completed or should be blocking shutdown if was > posted before it did. > > Actually, shouldn't this just be NOTREACHED()? No task should ever request to > run after shutdown completed, right? It is not an error if a WorkerThread tries to run a non BLOCK_SHUTDOWN task after shutdown. For example: Thread 1: WorkerThread extracts a SKIP_SHUTDOWN task from a PriorityQueue. Thread 2: TaskTracker::Shutdown() returns because no task is blocking shutdown. Thread 1: WorkerThread tries to run its task via the TaskTracker. The TaskTracker should prevent that, but it shouldn't crash. https://codereview.chromium.org/1705943002/diff/300001/base/task_scheduler/te... File base/task_scheduler/test_utils.h (right): https://codereview.chromium.org/1705943002/diff/300001/base/task_scheduler/te... base/task_scheduler/test_utils.h:16: #define EXPECT_DCHECK_DEATH(statement, regex) statement On 2016/03/21 17:42:30, gab wrote: > Thought you'd convinced me not to do this? What changed? I explained why we sometimes put multiple statements inside the EXPECT_DCHECK_DEATH macro [1], but I never said that we shouldn't execute |statement| when !DCHECK_IS_ON(). Pros/cons: We should always run |statement|: - To be consistent with EXPECT_DEBUG_DEATH() [2]. - To be able to verify the state after a DCHECK failure. For example, I would like to verify that BLOCK_SHUTDOWN tasks posted after shutdown don't run in a !DCHECK_IS_ON() build. We should only run |statement| when DCHECK_IS_ON(): - There is no need to verify what happens after a DCHECK because the style guide says that a DCHECK verifies a condition that "must always be true". The state after a DCHECK might be invalid. Note that existing SchedulerLock tests pass regardless of whether |statement| is executed. [1] https://codereview.chromium.org/1706123002/diff/400001/base/task_scheduler/sc... [2] https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/incl... https://codereview.chromium.org/1705943002/diff/300001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1705943002/diff/300001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:53009: + units="Tasks"> On 2016/03/21 17:42:30, gab wrote: > I don't see "Tasks" as a defined unit type in histograms.xml? Or do integer > types not need to be explicitly defined? "Tabs" is not defined as a unit either. asvitkine@ seems to be ok with this change. https://codereview.chromium.org/1705943002/diff/340001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1705943002/diff/340001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:53146: + units="Tasks"> On 2016/03/21 18:14:35, Alexei Svitkine (very slow) wrote: > Nit: No need to capitalize units. Done.
https://codereview.chromium.org/1705943002/diff/300001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1705943002/diff/300001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:53009: + units="Tasks"> On 2016/03/21 19:08:08, fdoray wrote: > On 2016/03/21 17:42:30, gab wrote: > > I don't see "Tasks" as a defined unit type in histograms.xml? Or do integer > > types not need to be explicitly defined? > > "Tabs" is not defined as a unit either. asvitkine@ seems to be ok with this > change. units= don't need an entry - it's a label for the dashboard graphs. enum= does need an entry, however.
https://codereview.chromium.org/1705943002/diff/300001/base/task_scheduler/ta... File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/1705943002/diff/300001/base/task_scheduler/ta... base/task_scheduler/task_tracker.cc:39: num_tasks_blocking_shutdown_copy = num_tasks_blocking_shutdown_; On 2016/03/21 19:08:07, fdoray wrote: > On 2016/03/21 17:42:30, gab wrote: > > UMA is extremely optimized, I don't think it's worth making a copy just to > > release the lock barely earlier. > > Done because it makes the code simpler. > > However, Dana asked me to release the Sequence's lock before > constructing+returning a SequenceSortKey here > https://codereview.chromium.org/1705253002/diff/360001/base/task_scheduler/se.... > Maybe she'll ask me to do a similar optimization here. Right, I saw that, and I agree that made sense there because of the construction of an object. But logging UMA is really just a ++ in a static memory region IIRC. https://codereview.chromium.org/1705943002/diff/300001/base/task_scheduler/ta... base/task_scheduler/task_tracker.cc:42: UMA_HISTOGRAM_COUNTS_100( On 2016/03/21 19:08:07, fdoray wrote: > On 2016/03/21 17:42:30, gab wrote: > > UMA_HISTOGRAM_COUNTS_100 sounds low given current 10000 restriction on > > BlockingPool. > > > > Also, this will not catch an infinite shutdown loop. > > > > How about using UMA_HISTOGRAM_COUNTS_CUSTOM with a self-defined max and also > > logging a metric if the max is busted during shutdown? > > Done. > > The current restriction for BlockingPool is 1000 I believe > (https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/br...). Ah, yes :-), then let's use that. https://codereview.chromium.org/1705943002/diff/300001/base/task_scheduler/ta... base/task_scheduler/task_tracker.cc:112: // bug. This DCHECK aims to catch those early. On 2016/03/21 19:08:07, fdoray wrote: > On 2016/03/21 17:42:30, gab wrote: > > Posting is an ordering bug, running is just unexpected as it either shouldn't > > have been posted if shutdown completed or should be blocking shutdown if was > > posted before it did. > > > > Actually, shouldn't this just be NOTREACHED()? No task should ever request to > > run after shutdown completed, right? > > It is not an error if a WorkerThread tries to run a non BLOCK_SHUTDOWN task > after shutdown. For example: > > Thread 1: WorkerThread extracts a SKIP_SHUTDOWN task from a PriorityQueue. > Thread 2: TaskTracker::Shutdown() returns because no task is blocking shutdown. > Thread 1: WorkerThread tries to run its task via the TaskTracker. The > TaskTracker should prevent that, but it shouldn't crash. Ah right, okay. https://codereview.chromium.org/1705943002/diff/300001/base/task_scheduler/te... File base/task_scheduler/test_utils.h (right): https://codereview.chromium.org/1705943002/diff/300001/base/task_scheduler/te... base/task_scheduler/test_utils.h:16: #define EXPECT_DCHECK_DEATH(statement, regex) statement On 2016/03/21 19:08:07, fdoray wrote: > On 2016/03/21 17:42:30, gab wrote: > > Thought you'd convinced me not to do this? What changed? > > I explained why we sometimes put multiple statements inside the > EXPECT_DCHECK_DEATH macro [1], but I never said that we shouldn't execute > |statement| when !DCHECK_IS_ON(). Pros/cons: > > We should always run |statement|: > - To be consistent with EXPECT_DEBUG_DEATH() [2]. > - To be able to verify the state after a DCHECK failure. For example, I would > like to verify that BLOCK_SHUTDOWN tasks posted after shutdown don't run in a > !DCHECK_IS_ON() build. > > We should only run |statement| when DCHECK_IS_ON(): > - There is no need to verify what happens after a DCHECK because the style guide > says that a DCHECK verifies a condition that "must always be true". The state > after a DCHECK might be invalid. > > Note that existing SchedulerLock tests pass regardless of whether |statement| is > executed. Hmm, this is surprising, that probably at the very least makes them flaky. The DCHECK are catching potential for race conditions, by running this code we run racy code which is probably undeterministically flaky :-(. I'd say testing what happens after a DCHECK doesn't make sense (i.e. +1 to your second point). > > [1] > https://codereview.chromium.org/1706123002/diff/400001/base/task_scheduler/sc... > [2] > https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/incl... https://codereview.chromium.org/1705943002/diff/300001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1705943002/diff/300001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:53009: + units="Tasks"> On 2016/03/21 19:35:35, Alexei Svitkine (very slow) wrote: > On 2016/03/21 19:08:08, fdoray wrote: > > On 2016/03/21 17:42:30, gab wrote: > > > I don't see "Tasks" as a defined unit type in histograms.xml? Or do integer > > > types not need to be explicitly defined? > > > > "Tabs" is not defined as a unit either. asvitkine@ seems to be ok with this > > change. > > units= don't need an entry - it's a label for the dashboard graphs. enum= does > need an entry, however. Got it, thanks! https://codereview.chromium.org/1705943002/diff/360001/base/task_scheduler/ta... File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/1705943002/diff/360001/base/task_scheduler/ta... base/task_scheduler/task_tracker.cc:81: num_tasks_blocking_shutdown_ > 0U); Need to hold lock for |num_tasks_blocking_shutdown_|, this check probably belongs in BeforeRunTask() anyways. https://codereview.chromium.org/1705943002/diff/360001/base/task_scheduler/ta... base/task_scheduler/task_tracker.cc:143: // unexpected as it either shouldn't been posted if shutdown completed or s/shouldn't been posted/shouldn't have been posted/ https://codereview.chromium.org/1705943002/diff/360001/base/task_scheduler/ta... File base/task_scheduler/task_tracker_unittest.cc (right): https://codereview.chromium.org/1705943002/diff/360001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:143: SCOPED_TRACE(""); \ What's the purpose of having this in a macro? https://codereview.chromium.org/1705943002/diff/360001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:259: // Test that a BLOCK_SHUTDOWN task cannot run after shutdown (the task has Actually looking at this again, it seems wrong to even test this... Essentially it's testing that we can't run a task that was never posted which means the API is being used the wrong way. I'm all for the other DEATH tests that verify proper reaction to various API usage. But here it just verifies that we DCHECK on clearly improper API usage. It's like having a DEATH test that passes a nullptr to a method that has a DCHECK(!null)... So I'm now strongly in favor of keeping the DCHECK in the product (to highlight that this condition doesn't make sense), but I'd remove the test here as it's confusing.
https://codereview.chromium.org/1705943002/diff/360001/base/task_scheduler/ta... File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/1705943002/diff/360001/base/task_scheduler/ta... base/task_scheduler/task_tracker.cc:36: DCHECK(!shutdown_completed_ && !shutdown_cv_) Don't use && in DCHECK. Write 2 DCHECKs instead so you can tell which failed. https://codereview.chromium.org/1705943002/diff/360001/base/task_scheduler/ta... base/task_scheduler/task_tracker.cc:81: num_tasks_blocking_shutdown_ > 0U); On 2016/03/21 19:53:25, gab wrote: > Need to hold lock for |num_tasks_blocking_shutdown_|, this check probably > belongs in BeforeRunTask() anyways. +1 Also it's easier to read as: if (shutdown_behavior == BLOCK_SHUTDOWN) DCHECK_GT(num_tasks_blocking_shutdown_, 0u); The if statement will get optimized out when DCHECKs are off since it's not calling any methods or anything. https://codereview.chromium.org/1705943002/diff/360001/base/task_scheduler/ta... base/task_scheduler/task_tracker.cc:103: DCHECK_NE(TaskShutdownBehavior::BLOCK_SHUTDOWN, shutdown_behavior); nit: write this in the order you would write a != b. iow we usually put the variable first and the constant second. https://codereview.chromium.org/1705943002/diff/360001/base/task_scheduler/ta... File base/task_scheduler/task_tracker.h (right): https://codereview.chromium.org/1705943002/diff/360001/base/task_scheduler/ta... base/task_scheduler/task_tracker.h:39: void PostTask(const Callback<void(scoped_ptr<Task>)>& post_task_callback, Can you explain the rationale for using a Callback for this? Do many different places end up calling PostTask and it needs to call back to all of them? If it's always one object, we could use a TaskTrackerClient that you set on create, and it can just call a method on the client? That's cheaper than constructing Callbacks each time.
danakj@: Can you take another look? Thanks. https://codereview.chromium.org/1705943002/diff/360001/base/task_scheduler/ta... File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/1705943002/diff/360001/base/task_scheduler/ta... base/task_scheduler/task_tracker.cc:36: DCHECK(!shutdown_completed_ && !shutdown_cv_) On 2016/03/22 18:56:56, danakj wrote: > Don't use && in DCHECK. Write 2 DCHECKs instead so you can tell which failed. Done. https://codereview.chromium.org/1705943002/diff/360001/base/task_scheduler/ta... base/task_scheduler/task_tracker.cc:81: num_tasks_blocking_shutdown_ > 0U); On 2016/03/22 18:56:56, danakj wrote: > On 2016/03/21 19:53:25, gab wrote: > > Need to hold lock for |num_tasks_blocking_shutdown_|, this check probably > > belongs in BeforeRunTask() anyways. > > +1 > > Also it's easier to read as: > > if (shutdown_behavior == BLOCK_SHUTDOWN) > DCHECK_GT(num_tasks_blocking_shutdown_, 0u); > > The if statement will get optimized out when DCHECKs are off since it's not > calling any methods or anything. This check was already in BeforeRunTask(). I just removed it from here. https://codereview.chromium.org/1705943002/diff/360001/base/task_scheduler/ta... base/task_scheduler/task_tracker.cc:103: DCHECK_NE(TaskShutdownBehavior::BLOCK_SHUTDOWN, shutdown_behavior); On 2016/03/22 18:56:56, danakj wrote: > nit: write this in the order you would write a != b. iow we usually put the > variable first and the constant second. Done. https://codereview.chromium.org/1705943002/diff/360001/base/task_scheduler/ta... base/task_scheduler/task_tracker.cc:143: // unexpected as it either shouldn't been posted if shutdown completed or On 2016/03/21 19:53:25, gab wrote: > s/shouldn't been posted/shouldn't have been posted/ Done. https://codereview.chromium.org/1705943002/diff/360001/base/task_scheduler/ta... File base/task_scheduler/task_tracker.h (right): https://codereview.chromium.org/1705943002/diff/360001/base/task_scheduler/ta... base/task_scheduler/task_tracker.h:39: void PostTask(const Callback<void(scoped_ptr<Task>)>& post_task_callback, On 2016/03/22 18:56:56, danakj wrote: > Can you explain the rationale for using a Callback for this? Do many different > places end up calling PostTask and it needs to call back to all of them? > > If it's always one object, we could use a TaskTrackerClient that you set on > create, and it can just call a method on the client? That's cheaper than > constructing Callbacks each time. The callback will always be bound to the same function, but with different arguments depending on the pq and sequence in which we want to insert the task. E.g. Bind(&PostTaskHelper, Unretained(priority_queue), sequence) I'm in favor of changing TaskTracker::PostTask to take pq+sequence as arguments instead of a callback. The "post task" algorithm that we planned to put in PostTaskHelper() would be in TaskTracker instead. It would be cheaper + it would allow us to provide info about the sequence and pq to tracing. I think that the reason why we decided to have a callback was to decouple TaskTracker from PriorityQueue and Sequence. WDYT? https://codereview.chromium.org/1705943002/diff/360001/base/task_scheduler/ta... File base/task_scheduler/task_tracker_unittest.cc (right): https://codereview.chromium.org/1705943002/diff/360001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:143: SCOPED_TRACE(""); \ On 2016/03/21 19:53:25, gab wrote: > What's the purpose of having this in a macro? With this macro, the line number of the call to VERIFY_ASYNC_SHUTDOWN_IN_PROGRESS() is reported in case of failure inside VerifyAsyncShutdownInProgress(). A line number inside VerifyAsyncShutdownInProgress() is usually less helpful than a line number in the test body to understand the cause of a failure. SCOPED_TRACE: // Causes a trace (including the source file path, the current line // number, and the given message) to be included in every test failure // message generated by code in the current scope. https://codereview.chromium.org/1705943002/diff/360001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:259: // Test that a BLOCK_SHUTDOWN task cannot run after shutdown (the task has On 2016/03/21 19:53:25, gab wrote: > Actually looking at this again, it seems wrong to even test this... Essentially > it's testing that we can't run a task that was never posted which means the API > is being used the wrong way. > > I'm all for the other DEATH tests that verify proper reaction to various API > usage. But here it just verifies that we DCHECK on clearly improper API usage. > It's like having a DEATH test that passes a nullptr to a method that has a > DCHECK(!null)... > > So I'm now strongly in favor of keeping the DCHECK in the product (to highlight > that this condition doesn't make sense), but I'd remove the test here as it's > confusing. Done.
lgtm++ for PS21, thanks! https://codereview.chromium.org/1705943002/diff/360001/base/task_scheduler/ta... File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/1705943002/diff/360001/base/task_scheduler/ta... base/task_scheduler/task_tracker.cc:103: DCHECK_NE(TaskShutdownBehavior::BLOCK_SHUTDOWN, shutdown_behavior); On 2016/03/22 20:19:43, fdoray wrote: > On 2016/03/22 18:56:56, danakj wrote: > > nit: write this in the order you would write a != b. iow we usually put the > > variable first and the constant second. > > Done. I agree that I prefer to write DCHECK_NE(actual, expected) per it being akin to "actual != expected", but the reason I've often heard to do it the other way around is to be consistent with gtest whose assertions are (expected, actual). I don't feel strongly either way, just want to make we decide on one way and remain consistent throughout base/task_scheduler. https://codereview.chromium.org/1705943002/diff/400001/base/task_scheduler/ta... File base/task_scheduler/task_tracker_unittest.cc (right): https://codereview.chromium.org/1705943002/diff/400001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:257: // pending BLOCK_SHUTDOWN tasks. So much "shutdown".. hehe! I think this reads better by putting "after shutdown" after "a BLOCK_SHUTDOWN task posted before shutdown"
gab@: Done. danakj@: Could you take another look? Pending question: Should we change TaskTracker::PostTask(Closure&, scoped_ptr<Task>) -> TaskTracker::PostTask(scoped_ptr<Task>, scoped_refptr<Sequence>, PriorityQueue*) Discussion link: https://codereview.chromium.org/1705943002/diff/360001/base/task_scheduler/ta... https://codereview.chromium.org/1705943002/diff/400001/base/task_scheduler/ta... File base/task_scheduler/task_tracker_unittest.cc (right): https://codereview.chromium.org/1705943002/diff/400001/base/task_scheduler/ta... base/task_scheduler/task_tracker_unittest.cc:257: // pending BLOCK_SHUTDOWN tasks. On 2016/03/24 13:00:39, gab (OOO until Apr. 5) wrote: > So much "shutdown".. hehe! > > I think this reads better by putting "after shutdown" after "a BLOCK_SHUTDOWN > task posted before shutdown" Done.
https://codereview.chromium.org/1705943002/diff/360001/base/task_scheduler/ta... File base/task_scheduler/task_tracker.h (right): https://codereview.chromium.org/1705943002/diff/360001/base/task_scheduler/ta... base/task_scheduler/task_tracker.h:39: void PostTask(const Callback<void(scoped_ptr<Task>)>& post_task_callback, On 2016/03/22 20:19:43, fdoray wrote: > On 2016/03/22 18:56:56, danakj wrote: > > Can you explain the rationale for using a Callback for this? Do many different > > places end up calling PostTask and it needs to call back to all of them? > > > > If it's always one object, we could use a TaskTrackerClient that you set on > > create, and it can just call a method on the client? That's cheaper than > > constructing Callbacks each time. > > The callback will always be bound to the same function, but with different > arguments depending on the pq and sequence in which we want to insert the task. > E.g. Bind(&PostTaskHelper, Unretained(priority_queue), sequence) > > I'm in favor of changing TaskTracker::PostTask to take pq+sequence as arguments > instead of a callback. The "post task" algorithm that we planned to put in > PostTaskHelper() would be in TaskTracker instead. It would be cheaper + it would > allow us to provide info about the sequence and pq to tracing. > > I think that the reason why we decided to have a callback was to decouple > TaskTracker from PriorityQueue and Sequence. > > WDYT? For the sake of encapsulation, I'd like to avoid making TaskTracker aware of PriorityQueue and Sequence. |post_task_callback| is merely a continuation (in fact I'd prefer |post_task_continuation| as a name now that I think about it) This can't be a raw function pointer or a method on a constant delegate because it binds PQ and Sequence :-(. This isn't done in a loop and is only done on PostTask, i.e. in situations where someone already called Bind on the real task. So is the overhead of an extra Bind really a concern? We could make this method aware of the logic in PostTaskHelper, but it feels out of place to me...
https://codereview.chromium.org/1705943002/diff/360001/base/task_scheduler/ta... File base/task_scheduler/task_tracker.h (right): https://codereview.chromium.org/1705943002/diff/360001/base/task_scheduler/ta... base/task_scheduler/task_tracker.h:39: void PostTask(const Callback<void(scoped_ptr<Task>)>& post_task_callback, On 2016/03/24 14:50:12, gab (OOO until Apr. 5) wrote: > On 2016/03/22 20:19:43, fdoray wrote: > > On 2016/03/22 18:56:56, danakj wrote: > > > Can you explain the rationale for using a Callback for this? Do many > different > > > places end up calling PostTask and it needs to call back to all of them? > > > > > > If it's always one object, we could use a TaskTrackerClient that you set on > > > create, and it can just call a method on the client? That's cheaper than > > > constructing Callbacks each time. > > > > The callback will always be bound to the same function, but with different > > arguments depending on the pq and sequence in which we want to insert the > task. > > E.g. Bind(&PostTaskHelper, Unretained(priority_queue), sequence) > > > > I'm in favor of changing TaskTracker::PostTask to take pq+sequence as > arguments > > instead of a callback. The "post task" algorithm that we planned to put in > > PostTaskHelper() would be in TaskTracker instead. It would be cheaper + it > would > > allow us to provide info about the sequence and pq to tracing. > > > > I think that the reason why we decided to have a callback was to decouple > > TaskTracker from PriorityQueue and Sequence. > > > > WDYT? > > For the sake of encapsulation, I'd like to avoid making TaskTracker aware of > PriorityQueue and Sequence. > > |post_task_callback| is merely a continuation (in fact I'd prefer > |post_task_continuation| as a name now that I think about it) > > This can't be a raw function pointer or a method on a constant delegate because > it binds PQ and Sequence :-(. > > This isn't done in a loop and is only done on PostTask, i.e. in situations where > someone already called Bind on the real task. So is the overhead of an extra > Bind really a concern? > We could make this method aware of the logic in PostTaskHelper, but it feels out > of place to me... The root of the issue stems from a necessity to synchronize between if we should post the task and actually posting the task, particularly during shutdown scenarios. The code is roughly: if (CanPostTask(task)) { // We're good to post the task if we aren't yet shutting down UpdateAccounting(task); // Track if this task blocks shutdown PostTask(task); } If we shutdown between CanPostTask and UpdateAccounting on a BLOCK_ON_SHUTDOWN task, shutdown will immediately happen (since we haven't yet accounted for the task), and then we'll post the task. Now we have a BLOCK_ON_SHUTDOWN task after we've shut down. Very bad. To address this, the CanPostTask and UpdateAccounting are synchronized in BeforeRunTask. The callback simply allows us to abstract away the details of posting the task from the TaskTracker as the TaskTracker's job is to simply perform accounting. The second approach would be to create a transactional object (similar to PriorityQueue) and expose things like CanPostTask (The check) and OnPostTask (The accounting). We felt that having this would unnecessarily complicate the design for this one-off need for synchronicity. Having said that, the TaskTracker should be unaware of the existence of a PriorityQueue and a Sequence as they are implementation details of posting a task.
On 2016/03/24 14:50:12, gab (OOO until Apr. 5) wrote: > https://codereview.chromium.org/1705943002/diff/360001/base/task_scheduler/ta... > File base/task_scheduler/task_tracker.h (right): > > https://codereview.chromium.org/1705943002/diff/360001/base/task_scheduler/ta... > base/task_scheduler/task_tracker.h:39: void PostTask(const > Callback<void(scoped_ptr<Task>)>& post_task_callback, > On 2016/03/22 20:19:43, fdoray wrote: > > On 2016/03/22 18:56:56, danakj wrote: > > > Can you explain the rationale for using a Callback for this? Do many > different > > > places end up calling PostTask and it needs to call back to all of them? > > > > > > If it's always one object, we could use a TaskTrackerClient that you set on > > > create, and it can just call a method on the client? That's cheaper than > > > constructing Callbacks each time. > > > > The callback will always be bound to the same function, but with different > > arguments depending on the pq and sequence in which we want to insert the > task. > > E.g. Bind(&PostTaskHelper, Unretained(priority_queue), sequence) > > > > I'm in favor of changing TaskTracker::PostTask to take pq+sequence as > arguments > > instead of a callback. The "post task" algorithm that we planned to put in > > PostTaskHelper() would be in TaskTracker instead. It would be cheaper + it > would > > allow us to provide info about the sequence and pq to tracing. > > > > I think that the reason why we decided to have a callback was to decouple > > TaskTracker from PriorityQueue and Sequence. > > > > WDYT? > > For the sake of encapsulation, I'd like to avoid making TaskTracker aware of > PriorityQueue and Sequence. > > |post_task_callback| is merely a continuation (in fact I'd prefer > |post_task_continuation| as a name now that I think about it) > > This can't be a raw function pointer or a method on a constant delegate because > it binds PQ and Sequence :-(. > > This isn't done in a loop and is only done on PostTask, i.e. in situations where > someone already called Bind on the real task. So is the overhead of an extra > Bind really a concern? I think it'll be okay. It also makes it harder to see where this code goes, as it gets a callback and you can't just click through that in Code Search. So I usually wanna think if a callback is really appropriate when I see it. Sounds like y'all have some good reasons for this here, so that's cool, thanks for the explaining. > We could make this method aware of the logic in PostTaskHelper, but it feels out > of place to me...
LGTM
https://codereview.chromium.org/1705943002/diff/420001/base/task_scheduler/te... File base/task_scheduler/test_utils.h (right): https://codereview.chromium.org/1705943002/diff/420001/base/task_scheduler/te... base/task_scheduler/test_utils.h:16: #define EXPECT_DCHECK_DEATH(statement, regex) statement Intentional change? Didn't see the comment triggering this. (Delta Added in Patchset 14) Original discussion regarding executing the statement: https://codereview.chromium.org/1706123002/diff/400001/base/task_scheduler/sc...
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/1705943002/420001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1705943002/420001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
https://codereview.chromium.org/1705943002/diff/420001/base/task_scheduler/te... File base/task_scheduler/test_utils.h (right): https://codereview.chromium.org/1705943002/diff/420001/base/task_scheduler/te... base/task_scheduler/test_utils.h:16: #define EXPECT_DCHECK_DEATH(statement, regex) statement On 2016/03/29 00:42:03, robliao wrote: > Intentional change? Didn't see the comment triggering this. (Delta Added in > Patchset 14) > > Original discussion regarding executing the statement: > https://codereview.chromium.org/1706123002/diff/400001/base/task_scheduler/sc... Initially it was intentional. But then gab@ said that he preferred not to execute the statement, so I removed it in the latest patch set. Discussion: https://codereview.chromium.org/1705943002/diff/300001/base/task_scheduler/te...
The CQ bit was checked by fdoray@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/1705943002/440001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1705943002/440001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by fdoray@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/1705943002/460001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1705943002/460001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm + 1 nit. https://codereview.chromium.org/1705943002/diff/420001/base/task_scheduler/te... File base/task_scheduler/test_utils.h (right): https://codereview.chromium.org/1705943002/diff/420001/base/task_scheduler/te... base/task_scheduler/test_utils.h:16: #define EXPECT_DCHECK_DEATH(statement, regex) statement On 2016/03/29 11:31:49, fdoray wrote: > On 2016/03/29 00:42:03, robliao wrote: > > Intentional change? Didn't see the comment triggering this. (Delta Added in > > Patchset 14) > > > > Original discussion regarding executing the statement: > > > https://codereview.chromium.org/1706123002/diff/400001/base/task_scheduler/sc... > > Initially it was intentional. But then gab@ said that he preferred not to > execute the statement, so I removed it in the latest patch set. > > Discussion: > https://codereview.chromium.org/1705943002/diff/300001/base/task_scheduler/te... sgtm. Nit: Keeping this empty should work.
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, gab@chromium.org, danakj@chromium.org, robliao@chromium.org Link to the patchset: https://codereview.chromium.org/1705943002/#ps480001 (title: "CR robliao #61")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1705943002/480001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1705943002/480001
Message was sent while issue was closed.
Description was changed from ========== TaskScheduler [5/9] Task Tracker This change is a subset of https://codereview.chromium.org/1698183005/ All tasks go through the scheduler's TaskTracker when they are posted and when they are executed. The TaskTracker enforces shutdown semantics and takes care of tracing and profiling. We have an alternate implementation which uses atomic operations to reduce lock contention. However, we prefer to check this simple implementation in first and bring the atomic operations in an incremental CL. BUG=553459 ========== to ========== TaskScheduler [5/9] Task Tracker This change is a subset of https://codereview.chromium.org/1698183005/ All tasks go through the scheduler's TaskTracker when they are posted and when they are executed. The TaskTracker enforces shutdown semantics and takes care of tracing and profiling. We have an alternate implementation which uses atomic operations to reduce lock contention. However, we prefer to check this simple implementation in first and bring the atomic operations in an incremental CL. BUG=553459 ==========
Message was sent while issue was closed.
Committed patchset #25 (id:480001)
Message was sent while issue was closed.
Description was changed from ========== TaskScheduler [5/9] Task Tracker This change is a subset of https://codereview.chromium.org/1698183005/ All tasks go through the scheduler's TaskTracker when they are posted and when they are executed. The TaskTracker enforces shutdown semantics and takes care of tracing and profiling. We have an alternate implementation which uses atomic operations to reduce lock contention. However, we prefer to check this simple implementation in first and bring the atomic operations in an incremental CL. BUG=553459 ========== to ========== TaskScheduler [5/9] Task Tracker This change is a subset of https://codereview.chromium.org/1698183005/ All tasks go through the scheduler's TaskTracker when they are posted and when they are executed. The TaskTracker enforces shutdown semantics and takes care of tracing and profiling. We have an alternate implementation which uses atomic operations to reduce lock contention. However, we prefer to check this simple implementation in first and bring the atomic operations in an incremental CL. BUG=553459 Committed: https://crrev.com/0fc7a6669c12c2ef2d1ccbd496232b7c65f6c81c Cr-Commit-Position: refs/heads/master@{#383749} ==========
Message was sent while issue was closed.
Patchset 25 (id:??) landed as https://crrev.com/0fc7a6669c12c2ef2d1ccbd496232b7c65f6c81c Cr-Commit-Position: refs/heads/master@{#383749} |