Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1806473002:
TaskScheduler [9] Delayed Task Manager (Closed)
DescriptionTaskScheduler [9] Delayed Task Manager
A DelayedTaskManager holds delayed tasks until they become ripe for
execution. When they become ripe for execution, it posts them to their
associated Sequence and PriorityQueue.
This change is a subset of https://codereview.chromium.org/1698183005/
BUG=553459
Committed: https://crrev.com/c308c577db26c7edc37a1002dd62a18ce9bbdef6
Cr-Commit-Position: refs/heads/master@{#388398}
Patch Set 1 #Patch Set 2 : Initial implementation for review. #
Total comments: 15
Patch Set 3 : CR robliao #6 #
Total comments: 10
Patch Set 4 : CR robliao #8 #
Total comments: 38
Patch Set 5 : CR gab #11 #Patch Set 6 : CR gab #14 + stop using a callback to post tasks. #Patch Set 7 : self review #Patch Set 8 : self review #Patch Set 9 : self review #
Total comments: 22
Patch Set 10 : CR gab #17 #
Total comments: 6
Patch Set 11 : CR gab #20 (nits + comments about memory management) #Patch Set 12 : self review #Patch Set 13 : CR gab #22 (TODO for WeakPtr) #Patch Set 14 : rebase #
Total comments: 2
Patch Set 15 : rebase #
Total comments: 8
Patch Set 16 : rebase #
Total comments: 3
Patch Set 17 : CR danakj #29 #
Messages
Total messages: 38 (9 generated)
fdoray@chromium.org changed reviewers: + danakj@chromium.org, gab@chromium.org, robliao@chromium.org
Description was changed from ========== TaskScheduler [7/9] Delayed Task Manager This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ========== to ========== TaskScheduler [8] Delayed Task Manager A DelayedTaskManager hold delayed tasks until they become ripe for execution. When they become ripe for execution, it invokes a custom callback. This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ==========
robliao@: Can you review this CL? Thanks.
A first pass https://codereview.chromium.org/1806473002/diff/20001/base/task_scheduler/del... File base/task_scheduler/delayed_task_manager.cc (right): https://codereview.chromium.org/1806473002/diff/20001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager.cc:39: if (!delayed_tasks_.empty()) Let's move this before the DelayedTask block above to keep the DelayedTask creation and insertion together. https://codereview.chromium.org/1806473002/diff/20001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager.cc:66: ready_tasks.push_back( This can be an emplace_back. https://codereview.chromium.org/1806473002/diff/20001/base/task_scheduler/del... File base/task_scheduler/delayed_task_manager.h (right): https://codereview.chromium.org/1806473002/diff/20001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager.h:28: using PostTaskCallback = Callback<void(std::unique_ptr<Task>)>; It seems to guidance is to continue using scoped_ptr. https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/scoped... https://codereview.chromium.org/1806473002/diff/20001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager.h:52: struct DelayedTask { Given that this is only used via a template here, I think you might be able to forward declare this as follows: Here: struct DelayedTask; delayed_task_manager.cc: struct DelayedTaskManager::DelayedTask { // Definition and implementation inlined. }; https://codereview.chromium.org/1806473002/diff/20001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager.h:53: DelayedTask(); Would it make sense to have an additional constructor that that takes all three data values as arguments to allow for std::priority_queue::emplace(task, post_task_callback, index)? https://codereview.chromium.org/1806473002/diff/20001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager.h:66: bool operator<(const DelayedTask& other) const; Looks like we'll also need operator> and operator== https://codereview.chromium.org/1806473002/diff/20001/base/task_scheduler/del... File base/task_scheduler/delayed_task_manager_unittest.cc (right): https://codereview.chromium.org/1806473002/diff/20001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager_unittest.cc:29: void SetTime(TimeTicks now) { now_ = now; } Maybe SetCurrentTime?
https://codereview.chromium.org/1806473002/diff/20001/base/task_scheduler/del... File base/task_scheduler/delayed_task_manager.h (right): https://codereview.chromium.org/1806473002/diff/20001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager.h:28: using PostTaskCallback = Callback<void(std::unique_ptr<Task>)>; On 2016/04/01 20:48:25, robliao wrote: > It seems to guidance is to continue using scoped_ptr. > > https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/scoped... I just saw the e-mail. Nevermind!
robliao@: Can you take another look? Thanks. https://codereview.chromium.org/1806473002/diff/20001/base/task_scheduler/del... File base/task_scheduler/delayed_task_manager.cc (right): https://codereview.chromium.org/1806473002/diff/20001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager.cc:39: if (!delayed_tasks_.empty()) On 2016/04/01 20:48:25, robliao wrote: > Let's move this before the DelayedTask block above to keep the DelayedTask > creation and insertion together. Done. https://codereview.chromium.org/1806473002/diff/20001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager.cc:66: ready_tasks.push_back( On 2016/04/01 20:48:25, robliao wrote: > This can be an emplace_back. Done. https://codereview.chromium.org/1806473002/diff/20001/base/task_scheduler/del... File base/task_scheduler/delayed_task_manager.h (right): https://codereview.chromium.org/1806473002/diff/20001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager.h:52: struct DelayedTask { On 2016/04/01 20:48:25, robliao wrote: > Given that this is only used via a template here, I think you might be able to > forward declare this as follows: > > Here: > struct DelayedTask; > > delayed_task_manager.cc: > struct DelayedTaskManager::DelayedTask { > // Definition and implementation inlined. > }; Done. https://codereview.chromium.org/1806473002/diff/20001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager.h:53: DelayedTask(); On 2016/04/01 20:48:25, robliao wrote: > Would it make sense to have an additional constructor that that takes all three > data values as arguments to allow for std::priority_queue::emplace(task, > post_task_callback, index)? Done. https://codereview.chromium.org/1806473002/diff/20001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager.h:66: bool operator<(const DelayedTask& other) const; On 2016/04/01 20:48:25, robliao wrote: > Looks like we'll also need operator> and operator== The style guide says "don't define operator overloads just because other libraries expect them. For example, if your type doesn't have a natural ordering, but you want to store it in a std::set, use a custom comparator rather than overloading <". I think that suggests that we should implement a custom comparator for std::priority_queue rather than implement all comparison operators. https://google.github.io/styleguide/cppguide.html#Operator_Overloading https://codereview.chromium.org/1806473002/diff/20001/base/task_scheduler/del... File base/task_scheduler/delayed_task_manager_unittest.cc (right): https://codereview.chromium.org/1806473002/diff/20001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager_unittest.cc:29: void SetTime(TimeTicks now) { now_ = now; } On 2016/04/01 20:48:25, robliao wrote: > Maybe SetCurrentTime? Done.
lgtm + Nits https://codereview.chromium.org/1806473002/diff/20001/base/task_scheduler/del... File base/task_scheduler/delayed_task_manager.h (right): https://codereview.chromium.org/1806473002/diff/20001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager.h:66: bool operator<(const DelayedTask& other) const; On 2016/04/01 21:21:57, fdoray wrote: > On 2016/04/01 20:48:25, robliao wrote: > > Looks like we'll also need operator> and operator== > > The style guide says "don't define operator overloads just because other > libraries expect them. For example, if your type doesn't have a natural > ordering, but you want to store it in a std::set, use a custom comparator rather > than overloading <". I think that suggests that we should implement a custom > comparator for std::priority_queue rather than implement all comparison > operators. > > https://google.github.io/styleguide/cppguide.html#Operator_Overloading sgtm. https://codereview.chromium.org/1806473002/diff/40001/base/task_scheduler/del... File base/task_scheduler/delayed_task_manager.h (right): https://codereview.chromium.org/1806473002/diff/40001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager.h:51: virtual TimeTicks Now(); Nit: This can be const. https://codereview.chromium.org/1806473002/diff/40001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager.h:53: struct DelayedTask; Nit: Move this struct and the one below to the top of the private block. https://codereview.chromium.org/1806473002/diff/40001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager.h:60: // Synchronizes acces to all members below. Nit: access https://codereview.chromium.org/1806473002/diff/40001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager.h:65: std::priority_queue<DelayedTask, Nit: This might be more readable as a type. using DelayedTasksQueue = std::priority_queue<...> DelayedTasksQueue delayed_tasks_; https://codereview.chromium.org/1806473002/diff/40001/base/task_scheduler/del... File base/task_scheduler/delayed_task_manager_unittest.cc (right): https://codereview.chromium.org/1806473002/diff/40001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager_unittest.cc:46: MOCK_METHOD1(PostTaskCallbackMock, void(const Task*)); Nit: linebreak above here.
danakj@: Can you review this CL? robliao@: Thanks for the review. https://codereview.chromium.org/1806473002/diff/40001/base/task_scheduler/del... File base/task_scheduler/delayed_task_manager.h (right): https://codereview.chromium.org/1806473002/diff/40001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager.h:51: virtual TimeTicks Now(); On 2016/04/04 17:59:53, robliao wrote: > Nit: This can be const. Done. https://codereview.chromium.org/1806473002/diff/40001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager.h:53: struct DelayedTask; On 2016/04/04 17:59:53, robliao wrote: > Nit: Move this struct and the one below to the top of the private block. Done. https://codereview.chromium.org/1806473002/diff/40001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager.h:60: // Synchronizes acces to all members below. On 2016/04/04 17:59:53, robliao wrote: > Nit: access Done. https://codereview.chromium.org/1806473002/diff/40001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager.h:65: std::priority_queue<DelayedTask, On 2016/04/04 17:59:53, robliao wrote: > Nit: This might be more readable as a type. > > using DelayedTasksQueue = std::priority_queue<...> > DelayedTasksQueue delayed_tasks_; Done. https://codereview.chromium.org/1806473002/diff/40001/base/task_scheduler/del... File base/task_scheduler/delayed_task_manager_unittest.cc (right): https://codereview.chromium.org/1806473002/diff/40001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager_unittest.cc:46: MOCK_METHOD1(PostTaskCallbackMock, void(const Task*)); On 2016/04/04 17:59:53, robliao wrote: > Nit: linebreak above here. Done.
Description was changed from ========== TaskScheduler [8] Delayed Task Manager A DelayedTaskManager hold delayed tasks until they become ripe for execution. When they become ripe for execution, it invokes a custom callback. This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ========== to ========== TaskScheduler [8] Delayed Task Manager A DelayedTaskManager holds delayed tasks until they become ripe for execution. When they become ripe for execution, it invokes a custom callback. This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ==========
lvg :-) https://codereview.chromium.org/1806473002/diff/60001/base/task_scheduler/del... File base/task_scheduler/delayed_task_manager.cc (right): https://codereview.chromium.org/1806473002/diff/60001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager.cc:21: DelayedTask& operator=(DelayedTask&& other); Should we explicitly delete the copy constructors? Are they needed for the STL? If they are how do they cope with the unique_ptr? https://codereview.chromium.org/1806473002/diff/60001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager.cc:23: ~DelayedTask() = default; Can't inline destructor when class has non-POD types. https://codereview.chromium.org/1806473002/diff/60001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager.cc:31: }; I'd expect the definitions of DelayedTaskManager::DelayedTask's methods to follow right after this (which I guess is out of order with the header, is it here to make the templates compile? feels out of place..) https://codereview.chromium.org/1806473002/diff/60001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager.cc:121: bool DelayedTaskManager::DelayedTaskComparator::operator()( Add a comment explaining what this does in the context of std::priority_queue (i.e. that std::priority_queue bubbles the "largest" element on top and expects the comparator to behave like std::less(left, right)). https://codereview.chromium.org/1806473002/diff/60001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager.cc:125: // popping the DelayedTask, we might actually have null tasks. s/we might actually have null tasks/null tasks can be passed to this comparator in Debug builds/ https://codereview.chromium.org/1806473002/diff/60001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager.cc:126: // To keep the order of the data structure the same, we consider null tasks s/To keep the order of the data structure the same/To satisfy these consistency checks/ https://codereview.chromium.org/1806473002/diff/60001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager.cc:127: // to be the smallest possible |delayed_run_time|. "smallest possible |delayed_run_time|" is confusing because that actually means "largest". How about: s/we consider null tasks to be the smallest possible |delayed_run_time|/this comparator considers null tasks to be the larger than anything/. https://codereview.chromium.org/1806473002/diff/60001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager.cc:128: if (!left.task) Add: // Since null tasks is a special case of the const_cast+move on pop, no two tasks should ever be null at the same time. DCHECK(left.task || right.task); ? https://codereview.chromium.org/1806473002/diff/60001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager.cc:131: return true; Would it make sense to: #ifndef NDEBUG DCHECK(left.task || right.task); if (!left.task) return false; if (!right.task) return true; #else DCHECK(left.task && right.task); #endif (+comments obviously) ? https://codereview.chromium.org/1806473002/diff/60001/base/task_scheduler/del... File base/task_scheduler/delayed_task_manager.h (right): https://codereview.chromium.org/1806473002/diff/60001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager.h:27: class BASE_EXPORT DelayedTaskManager { Does it make sense to hook it up to SchedulerThreadPool in this CL or is that a big enough change to warrant its own CL? I guess it needs the service thread to be functional so we have to wait? https://codereview.chromium.org/1806473002/diff/60001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager.h:33: explicit DelayedTaskManager(const Closure& delayed_run_time_changed); on_next_delayed_run_time_updated ? (to make it clear it's a notification and to reflect GetNextDelayedRunTime()) https://codereview.chromium.org/1806473002/diff/60001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager.h:38: // time being greater or equal to |task->delayed_run_time|. s/with the current time being great or equal to |task->delayed_run_time|/while Now() is passed |task->delayed_run_time|/ ? https://codereview.chromium.org/1806473002/diff/60001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager.h:67: DelayedTaskComparator>; I'm surprised that the definition of DelayedTask isn't required to compile this template. https://codereview.chromium.org/1806473002/diff/60001/base/task_scheduler/del... File base/task_scheduler/delayed_task_manager_unittest.cc (right): https://codereview.chromium.org/1806473002/diff/60001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager_unittest.cc:30: TimeTicks Now() const override { return now_; } // DelayedTaskManager: https://codereview.chromium.org/1806473002/diff/60001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager_unittest.cc:38: void PostTaskCallback(std::unique_ptr<Task> task) { private since you only use it from GetPostTaskCallback() helper? https://codereview.chromium.org/1806473002/diff/60001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager_unittest.cc:51: }; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/1806473002/diff/60001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager_unittest.cc:68: task->delayed_run_time = TimeTicks() + TimeDelta::FromSeconds(1); I'm not sure relying on the null value of TimeTicks being backed by a zero is kosher. Could maybe initialize |now_| to the actual TimeTicks::Now() and from then on use manual TimeDeltas from that value?
https://codereview.chromium.org/1806473002/diff/60001/base/task_scheduler/del... File base/task_scheduler/delayed_task_manager.cc (right): https://codereview.chromium.org/1806473002/diff/60001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager.cc:21: DelayedTask& operator=(DelayedTask&& other); On 2016/04/07 21:32:08, gab wrote: > Should we explicitly delete the copy constructors? Are they needed for the STL? > If they are how do they cope with the unique_ptr? They are not. You can DISALLOW_COPY_AND_ASSIGN and add move constructor/operator. These will work fine in STL containers.
gab@: All done. You can wait before reviewing again because I'll need to make some changes now that we no longer use callbacks to post tasks. https://codereview.chromium.org/1806473002/diff/60001/base/task_scheduler/del... File base/task_scheduler/delayed_task_manager.cc (right): https://codereview.chromium.org/1806473002/diff/60001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager.cc:21: DelayedTask& operator=(DelayedTask&& other); On 2016/04/07 21:39:29, danakj wrote: > On 2016/04/07 21:32:08, gab wrote: > > Should we explicitly delete the copy constructors? Are they needed for the > STL? > > If they are how do they cope with the unique_ptr? > > They are not. You can DISALLOW_COPY_AND_ASSIGN and add move > constructor/operator. These will work fine in STL containers. Done. https://codereview.chromium.org/1806473002/diff/60001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager.cc:23: ~DelayedTask() = default; On 2016/04/07 at 21:32:08, gab wrote: > Can't inline destructor when class has non-POD types. Done. But does it matter given that we are in the .cc file? https://codereview.chromium.org/1806473002/diff/60001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager.cc:31: }; On 2016/04/07 at 21:32:08, gab wrote: > I'd expect the definitions of DelayedTaskManager::DelayedTask's methods to follow right after this (which I guess is out of order with the header, is it here to make the templates compile? feels out of place..) DelayedTaskManager::DelayedTask's definition needs to be before any use of |delayed_tasks_|. Can I keep the method's definition at the bottom of the file to stay in the same order as the header? https://codereview.chromium.org/1806473002/diff/60001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager.cc:121: bool DelayedTaskManager::DelayedTaskComparator::operator()( On 2016/04/07 at 21:32:08, gab wrote: > Add a comment explaining what this does in the context of std::priority_queue (i.e. that std::priority_queue bubbles the "largest" element on top and expects the comparator to behave like std::less(left, right)). Done. https://codereview.chromium.org/1806473002/diff/60001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager.cc:126: // To keep the order of the data structure the same, we consider null tasks On 2016/04/07 at 21:32:08, gab wrote: > s/To keep the order of the data structure the same/To satisfy these consistency checks/ Done. https://codereview.chromium.org/1806473002/diff/60001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager.cc:127: // to be the smallest possible |delayed_run_time|. On 2016/04/07 at 21:32:08, gab wrote: > "smallest possible |delayed_run_time|" is confusing because that actually means "largest". > > How about: s/we consider null tasks to be the smallest possible |delayed_run_time|/this comparator considers null tasks to be the larger than anything/. Done. https://codereview.chromium.org/1806473002/diff/60001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager.cc:128: if (!left.task) On 2016/04/07 at 21:32:08, gab wrote: > Add: > // Since null tasks is a special case of the const_cast+move on pop, no two tasks should ever be null at the same time. > DCHECK(left.task || right.task); > ? Done. https://codereview.chromium.org/1806473002/diff/60001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager.cc:131: return true; On 2016/04/07 at 21:32:08, gab wrote: > Would it make sense to: > > #ifndef NDEBUG > DCHECK(left.task || right.task); > if (!left.task) > return false; > if (!right.task) > return true; > #else > DCHECK(left.task && right.task); > #endif > > (+comments obviously) > > ? Done. https://codereview.chromium.org/1806473002/diff/60001/base/task_scheduler/del... File base/task_scheduler/delayed_task_manager.h (right): https://codereview.chromium.org/1806473002/diff/60001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager.h:27: class BASE_EXPORT DelayedTaskManager { On 2016/04/07 21:32:08, gab wrote: > Does it make sense to hook it up to SchedulerThreadPool in this CL or is that a > big enough change to warrant its own CL? I guess it needs the service thread to > be functional so we have to wait? We need the service thread for this to be functional. https://codereview.chromium.org/1806473002/diff/60001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager.h:33: explicit DelayedTaskManager(const Closure& delayed_run_time_changed); On 2016/04/07 21:32:08, gab wrote: > on_next_delayed_run_time_updated ? > > (to make it clear it's a notification and to reflect GetNextDelayedRunTime()) Done. https://codereview.chromium.org/1806473002/diff/60001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager.h:38: // time being greater or equal to |task->delayed_run_time|. On 2016/04/07 21:32:08, gab wrote: > s/with the current time being great or equal to |task->delayed_run_time|/while > Now() is passed |task->delayed_run_time|/ ? Done. https://codereview.chromium.org/1806473002/diff/60001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager.h:67: DelayedTaskComparator>; On 2016/04/07 21:32:08, gab wrote: > I'm surprised that the definition of DelayedTask isn't required to compile this > template. The definition of DelayedTask isn't required to compute the size of DelayedTaskQueue. https://codereview.chromium.org/1806473002/diff/60001/base/task_scheduler/del... File base/task_scheduler/delayed_task_manager_unittest.cc (right): https://codereview.chromium.org/1806473002/diff/60001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager_unittest.cc:30: TimeTicks Now() const override { return now_; } On 2016/04/07 at 21:32:08, gab wrote: > // DelayedTaskManager: Done. https://codereview.chromium.org/1806473002/diff/60001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager_unittest.cc:38: void PostTaskCallback(std::unique_ptr<Task> task) { On 2016/04/07 at 21:32:09, gab wrote: > private since you only use it from GetPostTaskCallback() helper? Done. https://codereview.chromium.org/1806473002/diff/60001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager_unittest.cc:51: }; On 2016/04/07 at 21:32:09, gab wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/1806473002/diff/60001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager_unittest.cc:68: task->delayed_run_time = TimeTicks() + TimeDelta::FromSeconds(1); On 2016/04/07 at 21:32:08, gab wrote: > I'm not sure relying on the null value of TimeTicks being backed by a zero is kosher. Could maybe initialize |now_| to the actual TimeTicks::Now() and from then on use manual TimeDeltas from that value? Is manager.Now() + TimeDelta::FromSeconds(1) ok?
Replied to comments, but holding on further review per request. https://codereview.chromium.org/1806473002/diff/60001/base/task_scheduler/del... File base/task_scheduler/delayed_task_manager.cc (right): https://codereview.chromium.org/1806473002/diff/60001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager.cc:23: ~DelayedTask() = default; On 2016/04/08 17:24:51, fdoray wrote: > On 2016/04/07 at 21:32:08, gab wrote: > > Can't inline destructor when class has non-POD types. > > Done. But does it matter given that we are in the .cc file? Ah true, I guess it matters less, it basically makes inlining possible at the destruction sites in the .cc file which is probably okay. I've definitely often favored inlining the entire definition of classes declared in the .cc file for simplicity. But here you already had methods not inline so it felt right to also not inline that one (i.e. either inline everything when reasonable or follow default header/impl split style). https://codereview.chromium.org/1806473002/diff/60001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager.cc:31: }; On 2016/04/08 17:24:51, fdoray wrote: > On 2016/04/07 at 21:32:08, gab wrote: > > I'd expect the definitions of DelayedTaskManager::DelayedTask's methods to > follow right after this (which I guess is out of order with the header, is it > here to make the templates compile? feels out of place..) > > DelayedTaskManager::DelayedTask's definition needs to be before any use of > |delayed_tasks_|. Can I keep the method's definition at the bottom of the file > to stay in the same order as the header? Maybe it's fine to just define the entire class inline at the top then? IMO keeping a component together takes precedence over following order of the header. https://codereview.chromium.org/1806473002/diff/60001/base/task_scheduler/del... File base/task_scheduler/delayed_task_manager_unittest.cc (right): https://codereview.chromium.org/1806473002/diff/60001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager_unittest.cc:68: task->delayed_run_time = TimeTicks() + TimeDelta::FromSeconds(1); On 2016/04/08 17:24:52, fdoray wrote: > On 2016/04/07 at 21:32:08, gab wrote: > > I'm not sure relying on the null value of TimeTicks being backed by a zero is > kosher. Could maybe initialize |now_| to the actual TimeTicks::Now() and from > then on use manual TimeDeltas from that value? > > Is manager.Now() + TimeDelta::FromSeconds(1) ok? Yes.
https://codereview.chromium.org/1806473002/diff/60001/base/task_scheduler/del... File base/task_scheduler/delayed_task_manager.cc (right): https://codereview.chromium.org/1806473002/diff/60001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager.cc:31: }; On 2016/04/08 at 18:03:55, gab wrote: > On 2016/04/08 17:24:51, fdoray wrote: > > On 2016/04/07 at 21:32:08, gab wrote: > > > I'd expect the definitions of DelayedTaskManager::DelayedTask's methods to > > follow right after this (which I guess is out of order with the header, is it > > here to make the templates compile? feels out of place..) > > > > DelayedTaskManager::DelayedTask's definition needs to be before any use of > > |delayed_tasks_|. Can I keep the method's definition at the bottom of the file > > to stay in the same order as the header? > > Maybe it's fine to just define the entire class inline at the top then? > > IMO keeping a component together takes precedence over following order of the header. Done. Defined the entire class inline at the top.
Description was changed from ========== TaskScheduler [8] Delayed Task Manager A DelayedTaskManager holds delayed tasks until they become ripe for execution. When they become ripe for execution, it invokes a custom callback. This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ========== to ========== TaskScheduler [8] Delayed Task Manager A DelayedTaskManager holds delayed tasks until they become ripe for execution. When they become ripe for execution, it posts them to their associated Sequence and PriorityQueue. This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ==========
lgtm w/ comments, thanks! https://codereview.chromium.org/1806473002/diff/160001/base/task_scheduler/de... File base/task_scheduler/delayed_task_manager.cc (right): https://codereview.chromium.org/1806473002/diff/160001/base/task_scheduler/de... base/task_scheduler/delayed_task_manager.cc:72: TimeTicks existing_next_delayed_run_time; How about s/existing_next_delayed_run_time/current_delayed_run_time/ ? And reading the rest of this algorithm maybe s/on_next_delayed_run_time_updated_/on_delayed_run_time_updated_/ would also make this less wordy? https://codereview.chromium.org/1806473002/diff/160001/base/task_scheduler/de... base/task_scheduler/delayed_task_manager.cc:104: // The const_cast for std::move is almost okay since we're immediately "almost okay" sound weird here, how about: // The const_cast for std::move is okay since we're immediately moving it to |ready_tasks|. See DelayedTaskComparator::operator() for minor debug-check implications. https://codereview.chromium.org/1806473002/diff/160001/base/task_scheduler/de... File base/task_scheduler/delayed_task_manager.h (right): https://codereview.chromium.org/1806473002/diff/160001/base/task_scheduler/de... base/task_scheduler/delayed_task_manager.h:74: size_t next_delayed_task_index_ = 0; According to http://stackoverflow.com/questions/22514803/maximum-size-of-size-t, the C++ standard says that size_t's max size must be >= 65535 but that does sound pretty low to me for this index. It does appear to be a uint (unsigned 32-bit int, MAX == 4294967295) in practice but we shouldn't rely on that. How about explicitly using a uint32 or uint64? uint64 is probably safer once we get to renderer lands where there are tons of short lived timers using delayed tasks? https://codereview.chromium.org/1806473002/diff/160001/base/task_scheduler/de... File base/task_scheduler/delayed_task_manager_unittest.cc (right): https://codereview.chromium.org/1806473002/diff/160001/base/task_scheduler/de... base/task_scheduler/delayed_task_manager_unittest.cc:30: Unretained(this))) {} Initialize |now_| to TimeTicks::Now() or we're still adding deltas to a null time. https://codereview.chromium.org/1806473002/diff/160001/base/task_scheduler/de... base/task_scheduler/delayed_task_manager_unittest.cc:54: TEST(TaskSchedulerDelayedTaskManagerTest, PostTaskBeforeDelayedRunTime) { s/PostTask.../PostReadyTasks.../ here and below? https://codereview.chromium.org/1806473002/diff/160001/base/task_scheduler/de... base/task_scheduler/delayed_task_manager_unittest.cc:194: priority_queue.BeginTransaction()->Pop(); EXPECT empty PQ https://codereview.chromium.org/1806473002/diff/160001/base/task_scheduler/de... base/task_scheduler/delayed_task_manager_unittest.cc:196: sequence->PopTask(); EXPECT empty sequence https://codereview.chromium.org/1806473002/diff/160001/base/task_scheduler/de... base/task_scheduler/delayed_task_manager_unittest.cc:210: sequence->PopTask(); Verify PQ as well and empty post pops https://codereview.chromium.org/1806473002/diff/160001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.cc (right): https://codereview.chromium.org/1806473002/diff/160001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:44: // TODO(fdoray): Support delayed tasks. Remove TODO https://codereview.chromium.org/1806473002/diff/160001/base/task_scheduler/ut... File base/task_scheduler/utils.h (right): https://codereview.chromium.org/1806473002/diff/160001/base/task_scheduler/ut... base/task_scheduler/utils.h:23: void BASE_EXPORT PostTaskNowHelper(std::unique_ptr<Task> task, Move to anonymous namespace and have tests go through PostTaskHelper and also test DelayedTaskManager interaction.
Description was changed from ========== TaskScheduler [8] Delayed Task Manager A DelayedTaskManager holds delayed tasks until they become ripe for execution. When they become ripe for execution, it posts them to their associated Sequence and PriorityQueue. This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ========== to ========== TaskScheduler [9] Delayed Task Manager A DelayedTaskManager holds delayed tasks until they become ripe for execution. When they become ripe for execution, it posts them to their associated Sequence and PriorityQueue. This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ==========
danakj@: Can you review this CL? Thanks. https://codereview.chromium.org/1806473002/diff/160001/base/task_scheduler/de... File base/task_scheduler/delayed_task_manager.cc (right): https://codereview.chromium.org/1806473002/diff/160001/base/task_scheduler/de... base/task_scheduler/delayed_task_manager.cc:72: TimeTicks existing_next_delayed_run_time; On 2016/04/11 18:28:30, gab wrote: > How about s/existing_next_delayed_run_time/current_delayed_run_time/ ? > > And reading the rest of this algorithm maybe > s/on_next_delayed_run_time_updated_/on_delayed_run_time_updated_/ would also > make this less wordy? Done. https://codereview.chromium.org/1806473002/diff/160001/base/task_scheduler/de... base/task_scheduler/delayed_task_manager.cc:104: // The const_cast for std::move is almost okay since we're immediately On 2016/04/11 18:28:30, gab wrote: > "almost okay" sound weird here, how about: > > // The const_cast for std::move is okay since we're immediately moving it to > |ready_tasks|. See DelayedTaskComparator::operator() for minor debug-check > implications. Done. https://codereview.chromium.org/1806473002/diff/160001/base/task_scheduler/de... File base/task_scheduler/delayed_task_manager.h (right): https://codereview.chromium.org/1806473002/diff/160001/base/task_scheduler/de... base/task_scheduler/delayed_task_manager.h:74: size_t next_delayed_task_index_ = 0; On 2016/04/11 18:28:30, gab wrote: > According to http://stackoverflow.com/questions/22514803/maximum-size-of-size-t, > the C++ standard says that size_t's max size must be >= 65535 but that does > sound pretty low to me for this index. > > It does appear to be a uint (unsigned 32-bit int, MAX == 4294967295) in practice > but we shouldn't rely on that. > > How about explicitly using a uint32 or uint64? uint64 is probably safer once we > get to renderer lands where there are tons of short lived timers using delayed > tasks? Done. https://codereview.chromium.org/1806473002/diff/160001/base/task_scheduler/de... File base/task_scheduler/delayed_task_manager_unittest.cc (right): https://codereview.chromium.org/1806473002/diff/160001/base/task_scheduler/de... base/task_scheduler/delayed_task_manager_unittest.cc:30: Unretained(this))) {} On 2016/04/11 18:28:30, gab wrote: > Initialize |now_| to TimeTicks::Now() or we're still adding deltas to a null > time. Done. https://codereview.chromium.org/1806473002/diff/160001/base/task_scheduler/de... base/task_scheduler/delayed_task_manager_unittest.cc:54: TEST(TaskSchedulerDelayedTaskManagerTest, PostTaskBeforeDelayedRunTime) { On 2016/04/11 18:28:30, gab wrote: > s/PostTask.../PostReadyTasks.../ here and below? Done. https://codereview.chromium.org/1806473002/diff/160001/base/task_scheduler/de... base/task_scheduler/delayed_task_manager_unittest.cc:194: priority_queue.BeginTransaction()->Pop(); On 2016/04/11 18:28:30, gab wrote: > EXPECT empty PQ Done. https://codereview.chromium.org/1806473002/diff/160001/base/task_scheduler/de... base/task_scheduler/delayed_task_manager_unittest.cc:196: sequence->PopTask(); On 2016/04/11 18:28:30, gab wrote: > EXPECT empty sequence Done. https://codereview.chromium.org/1806473002/diff/160001/base/task_scheduler/de... base/task_scheduler/delayed_task_manager_unittest.cc:210: sequence->PopTask(); On 2016/04/11 18:28:30, gab wrote: > Verify PQ as well and empty post pops Done. https://codereview.chromium.org/1806473002/diff/160001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.cc (right): https://codereview.chromium.org/1806473002/diff/160001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:44: // TODO(fdoray): Support delayed tasks. On 2016/04/11 18:28:30, gab wrote: > Remove TODO Delayed tasks are not supported until we have the service thread. https://codereview.chromium.org/1806473002/diff/160001/base/task_scheduler/ut... File base/task_scheduler/utils.h (right): https://codereview.chromium.org/1806473002/diff/160001/base/task_scheduler/ut... base/task_scheduler/utils.h:23: void BASE_EXPORT PostTaskNowHelper(std::unique_ptr<Task> task, On 2016/04/11 18:28:30, gab wrote: > Move to anonymous namespace and have tests go through PostTaskHelper and also > test DelayedTaskManager interaction. Can't move to anonymous namespace because it's used by DelayedTaskManager. Added tests for DelayedTaskManager interaction.
2 nits and a question w.r.t. to memory management. https://codereview.chromium.org/1806473002/diff/160001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.cc (right): https://codereview.chromium.org/1806473002/diff/160001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:44: // TODO(fdoray): Support delayed tasks. On 2016/04/11 19:57:06, fdoray wrote: > On 2016/04/11 18:28:30, gab wrote: > > Remove TODO > > Delayed tasks are not supported until we have the service thread. Ah right, but from this component's POV it *is* posting to the |delayed_task_manager_|, i.e. it won't change once we make them work so it's weird to have the TODO here, is there a better place for it? I guess here is good because it DCHECKs the API isn't being used in an unsupported way -- although PostDelayedTask says the delay doesn't have to be honored so we could remove the DCHECK here and instead add the TODO on DelayedTaskManager or something to hook it up? https://codereview.chromium.org/1806473002/diff/180001/base/task_scheduler/de... File base/task_scheduler/delayed_task_manager.h (right): https://codereview.chromium.org/1806473002/diff/180001/base/task_scheduler/de... base/task_scheduler/delayed_task_manager.h:43: PriorityQueue* priority_queue); Who owns this PriorityQueue? Is it guaranteed to be alive when the DelayedTask is ripe? If so please document this requirement in the API above and document why that is okay at the callsite if it's not obvious there; if not this should be a WeakPtr or something. https://codereview.chromium.org/1806473002/diff/180001/base/task_scheduler/ut... File base/task_scheduler/utils_unittest.cc (right): https://codereview.chromium.org/1806473002/diff/180001/base/task_scheduler/ut... base/task_scheduler/utils_unittest.cc:27: TEST(TaskSchedulerPostTaskNowHelperTest, PostTaskInEmptySequence) { This is using PostTaskHelper not PostTaskNowHelper, fix test name to reflect that (here and below)
https://codereview.chromium.org/1806473002/diff/160001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.cc (right): https://codereview.chromium.org/1806473002/diff/160001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.cc:44: // TODO(fdoray): Support delayed tasks. On 2016/04/12 13:35:47, gab wrote: > On 2016/04/11 19:57:06, fdoray wrote: > > On 2016/04/11 18:28:30, gab wrote: > > > Remove TODO > > > > Delayed tasks are not supported until we have the service thread. > > Ah right, but from this component's POV it *is* posting to the > |delayed_task_manager_|, i.e. it won't change once we make them work so it's > weird to have the TODO here, is there a better place for it? I guess here is > good because it DCHECKs the API isn't being used in an unsupported way -- > although PostDelayedTask says the delay doesn't have to be honored so we could > remove the DCHECK here and instead add the TODO on DelayedTaskManager or > something to hook it up? Done. Removed the DCHECK here and put it above PostReadyTasks() in delayed_task_manager.h. With this change, delayed tasks can be posted without failure but they will never run. TaskRunner::PostTask: "Returns true if the task *may* be run at some point in the future". https://codereview.chromium.org/1806473002/diff/180001/base/task_scheduler/de... File base/task_scheduler/delayed_task_manager.h (right): https://codereview.chromium.org/1806473002/diff/180001/base/task_scheduler/de... base/task_scheduler/delayed_task_manager.h:43: PriorityQueue* priority_queue); On 2016/04/12 13:35:48, gab wrote: > Who owns this PriorityQueue? Is it guaranteed to be alive when the DelayedTask > is ripe? If so please document this requirement in the API above and document > why that is okay at the callsite if it's not obvious there; if not this should > be a WeakPtr or something. Done. Added comments here, above PostTaskHelper in utils.h and above CreateThreadPool in scheduler_thread_pool.h. In production, TaskScheduler, SchedulerThreadPool, SchedulerWorkerThread, DelayedTaskManager, TaskTracker and PriorityQueue instances are never destroyed. https://codereview.chromium.org/1806473002/diff/180001/base/task_scheduler/ut... File base/task_scheduler/utils_unittest.cc (right): https://codereview.chromium.org/1806473002/diff/180001/base/task_scheduler/ut... base/task_scheduler/utils_unittest.cc:27: TEST(TaskSchedulerPostTaskNowHelperTest, PostTaskInEmptySequence) { On 2016/04/12 13:35:48, gab wrote: > This is using PostTaskHelper not PostTaskNowHelper, fix test name to reflect > that (here and below) Done.
https://codereview.chromium.org/1806473002/diff/180001/base/task_scheduler/de... File base/task_scheduler/delayed_task_manager.h (right): https://codereview.chromium.org/1806473002/diff/180001/base/task_scheduler/de... base/task_scheduler/delayed_task_manager.h:43: PriorityQueue* priority_queue); On 2016/04/12 14:43:57, fdoray wrote: > On 2016/04/12 13:35:48, gab wrote: > > Who owns this PriorityQueue? Is it guaranteed to be alive when the DelayedTask > > is ripe? If so please document this requirement in the API above and document > > why that is okay at the callsite if it's not obvious there; if not this should > > be a WeakPtr or something. > > Done. Added comments here, above PostTaskHelper in utils.h and above > CreateThreadPool in scheduler_thread_pool.h. > > In production, TaskScheduler, SchedulerThreadPool, SchedulerWorkerThread, > DelayedTaskManager, TaskTracker and PriorityQueue instances are never destroyed. As discussed offline: I'd prefer not making assumptions about components never being deleted. Localized to a component that's okay, but when passing and storing raw pointers I don't like it so much -- if we one day change our minds I'd rather not have to worry about a gazillion assumptions having crept throughout our codebase.. So let's not add comments about it in this CL (i.e. revert comment part of last PS) and instead add a TODO to fix this raw pointer -- Rob said he'd look into it.
https://codereview.chromium.org/1806473002/diff/180001/base/task_scheduler/de... File base/task_scheduler/delayed_task_manager.h (right): https://codereview.chromium.org/1806473002/diff/180001/base/task_scheduler/de... base/task_scheduler/delayed_task_manager.h:43: PriorityQueue* priority_queue); On 2016/04/12 18:07:30, gab wrote: > On 2016/04/12 14:43:57, fdoray wrote: > > On 2016/04/12 13:35:48, gab wrote: > > > Who owns this PriorityQueue? Is it guaranteed to be alive when the > DelayedTask > > > is ripe? If so please document this requirement in the API above and > document > > > why that is okay at the callsite if it's not obvious there; if not this > should > > > be a WeakPtr or something. > > > > Done. Added comments here, above PostTaskHelper in utils.h and above > > CreateThreadPool in scheduler_thread_pool.h. > > > > In production, TaskScheduler, SchedulerThreadPool, SchedulerWorkerThread, > > DelayedTaskManager, TaskTracker and PriorityQueue instances are never > destroyed. > > As discussed offline: I'd prefer not making assumptions about components never > being deleted. Localized to a component that's okay, but when passing and > storing raw pointers I don't like it so much -- if we one day change our minds > I'd rather not have to worry about a gazillion assumptions having crept > throughout our codebase.. > > So let's not add comments about it in this CL (i.e. revert comment part of last > PS) and instead add a TODO to fix this raw pointer -- Rob said he'd look into > it. Done.
Thanks re-lgtm w/ TODO. @dana for OWNERS
danakj@: I rebased this CL on the latest SchedulerThreadPool. Can you review it after https://codereview.chromium.org/1851403003/? Thanks.
rebase with latest updates still lgtm % comment below https://codereview.chromium.org/1806473002/diff/260001/base/task_scheduler/ut... File base/task_scheduler/utils.cc (right): https://codereview.chromium.org/1806473002/diff/260001/base/task_scheduler/ut... base/task_scheduler/utils.cc:36: std::unique_ptr<Task> task(new Task(posted_from, closure, traits)); TrackingInfo (Task's subsubclass) has a constructor parameter to set |delayed_run_time|, is it worth using that instead of altering it below? (otherwise it'll be confusing later to a reader of task.h that it never declares a delay whereas a constructor will allow this reader to see where it's used from)
gab@: Done. danakj@: I rebased this CL on the latest SchedulerThreadPool. Can you review it after https://codereview.chromium.org/1851403003/? Thanks. https://codereview.chromium.org/1806473002/diff/260001/base/task_scheduler/ut... File base/task_scheduler/utils.cc (right): https://codereview.chromium.org/1806473002/diff/260001/base/task_scheduler/ut... base/task_scheduler/utils.cc:36: std::unique_ptr<Task> task(new Task(posted_from, closure, traits)); On 2016/04/15 15:39:10, gab wrote: > TrackingInfo (Task's subsubclass) has a constructor parameter to set > |delayed_run_time|, is it worth using that instead of altering it below? > (otherwise it'll be confusing later to a reader of task.h that it never declares > a delay whereas a constructor will allow this reader to see where it's used > from) Done. Related CL: https://codereview.chromium.org/1890163003/
danakj@: ping. Can you review this CL?
https://codereview.chromium.org/1806473002/diff/280001/base/task_scheduler/de... File base/task_scheduler/delayed_task_manager.cc (right): https://codereview.chromium.org/1806473002/diff/280001/base/task_scheduler/de... base/task_scheduler/delayed_task_manager.cc:25: DelayedTask(DelayedTask&& other) this looks like you could use =default instead of writing out all the members? https://codereview.chromium.org/1806473002/diff/280001/base/task_scheduler/de... base/task_scheduler/delayed_task_manager.cc:33: DelayedTask& operator=(DelayedTask&& other) { =default here too? https://codereview.chromium.org/1806473002/diff/280001/base/task_scheduler/de... base/task_scheduler/delayed_task_manager.cc:80: delayed_task_index_++); maybe start with an index of 1 instead of 0 (ie pre-increment instead of post) https://codereview.chromium.org/1806473002/diff/280001/base/task_scheduler/de... base/task_scheduler/delayed_task_manager.cc:104: ready_tasks.emplace_back( push_back? https://codereview.chromium.org/1806473002/diff/300001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.h (right): https://codereview.chromium.org/1806473002/diff/300001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:59: DelayedTaskManager* delayed_task_manager); Who will own the DelayedTaskManager? Is it shared between pools? Why doesn't the pool just have a DelayedTaskManager member?
danakj@: Done. Can you take another look? Thanks. https://codereview.chromium.org/1806473002/diff/280001/base/task_scheduler/de... File base/task_scheduler/delayed_task_manager.cc (right): https://codereview.chromium.org/1806473002/diff/280001/base/task_scheduler/de... base/task_scheduler/delayed_task_manager.cc:25: DelayedTask(DelayedTask&& other) On 2016/04/19 21:16:46, danakj wrote: > this looks like you could use =default instead of writing out all the members? Done. https://codereview.chromium.org/1806473002/diff/280001/base/task_scheduler/de... base/task_scheduler/delayed_task_manager.cc:33: DelayedTask& operator=(DelayedTask&& other) { On 2016/04/19 21:16:46, danakj wrote: > =default here too? Done. https://codereview.chromium.org/1806473002/diff/280001/base/task_scheduler/de... base/task_scheduler/delayed_task_manager.cc:80: delayed_task_index_++); On 2016/04/19 21:16:46, danakj wrote: > maybe start with an index of 1 instead of 0 (ie pre-increment instead of post) Done. https://codereview.chromium.org/1806473002/diff/280001/base/task_scheduler/de... base/task_scheduler/delayed_task_manager.cc:104: ready_tasks.emplace_back( On 2016/04/19 21:16:46, danakj wrote: > push_back? Done. https://codereview.chromium.org/1806473002/diff/300001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.h (right): https://codereview.chromium.org/1806473002/diff/300001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:59: DelayedTaskManager* delayed_task_manager); On 2016/04/19 21:16:46, danakj wrote: > Who will own the DelayedTaskManager? Is it shared between pools? Why doesn't the > pool just have a DelayedTaskManager member? The DelayedTaskManager will be owned by the TaskScheduler. Each ThreadPool owned by the TaskScheduler will add its delayed tasks to the same DelayedTaskManager. A service thread will call DelayedTaskManager::PostReadyTasks() when the delays expire (it will use a combination of the DelayedTaskManager's callback and GetNextDelayedRunTime() to be woken up at the right time). Each thread pool could have its own DelayedTaskManager, but then we would need a thread in each pool to wait for delays to expire. It could be a worker thread (delayed tasks would run later than expected when the worker thread has to run a long task) or a service thread (we prefer one service thread per scheduler than one per pool). The service thread will also be responsible for recording UMA histograms regularly and generating crash dumps when threads become unresponsive.
https://codereview.chromium.org/1806473002/diff/300001/base/task_scheduler/sc... File base/task_scheduler/scheduler_thread_pool.h (right): https://codereview.chromium.org/1806473002/diff/300001/base/task_scheduler/sc... base/task_scheduler/scheduler_thread_pool.h:59: DelayedTaskManager* delayed_task_manager); On 2016/04/19 21:46:45, fdoray wrote: > On 2016/04/19 21:16:46, danakj wrote: > > Who will own the DelayedTaskManager? Is it shared between pools? Why doesn't > the > > pool just have a DelayedTaskManager member? > > The DelayedTaskManager will be owned by the TaskScheduler. Each ThreadPool owned > by the TaskScheduler will add its delayed tasks to the same DelayedTaskManager. > A service thread will call DelayedTaskManager::PostReadyTasks() when the delays > expire (it will use a combination of the DelayedTaskManager's callback and > GetNextDelayedRunTime() to be woken up at the right time). > > Each thread pool could have its own DelayedTaskManager, but then we would need a > thread in each pool to wait for delays to expire. It could be a worker thread > (delayed tasks would run later than expected when the worker thread has to run a > long task) or a service thread (we prefer one service thread per scheduler than > one per pool). > > The service thread will also be responsible for recording UMA histograms > regularly and generating crash dumps when threads become unresponsive. Ok thanks, LGTM then.
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robliao@chromium.org, gab@chromium.org Link to the patchset: https://codereview.chromium.org/1806473002/#ps320001 (title: "CR danakj #29")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1806473002/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1806473002/320001
Message was sent while issue was closed.
Description was changed from ========== TaskScheduler [9] Delayed Task Manager A DelayedTaskManager holds delayed tasks until they become ripe for execution. When they become ripe for execution, it posts them to their associated Sequence and PriorityQueue. This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ========== to ========== TaskScheduler [9] Delayed Task Manager A DelayedTaskManager holds delayed tasks until they become ripe for execution. When they become ripe for execution, it posts them to their associated Sequence and PriorityQueue. This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ==========
Message was sent while issue was closed.
Committed patchset #17 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== TaskScheduler [9] Delayed Task Manager A DelayedTaskManager holds delayed tasks until they become ripe for execution. When they become ripe for execution, it posts them to their associated Sequence and PriorityQueue. This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ========== to ========== TaskScheduler [9] Delayed Task Manager A DelayedTaskManager holds delayed tasks until they become ripe for execution. When they become ripe for execution, it posts them to their associated Sequence and PriorityQueue. This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 Committed: https://crrev.com/c308c577db26c7edc37a1002dd62a18ce9bbdef6 Cr-Commit-Position: refs/heads/master@{#388398} ==========
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/c308c577db26c7edc37a1002dd62a18ce9bbdef6 Cr-Commit-Position: refs/heads/master@{#388398} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
