Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1705253002:
TaskScheduler [3/9] Task and Sequence (Closed)
DescriptionTaskScheduler [3/9] Task and Sequence
This change is a subset of https://codereview.chromium.org/1698183005/
A Task is a unit of work in the task scheduler. It has a closure,
a sequenced time, TaskTraits and other metadata inherited from
base::PendingTask.
A Sequence holds Tasks that must run in order. It is ref-counted
and has thread-safe Push, Pop and Peek operations. Priority queues,
worker threads and task runners will have references on Sequences.
BUG=553459
Committed: https://crrev.com/c48d5f0936e192046e11f9f9f03a70570523fc94
Cr-Commit-Position: refs/heads/master@{#381638}
Patch Set 1 #
Total comments: 64
Patch Set 2 : address comments from gab #Patch Set 3 : fix test #Patch Set 4 : self review #
Total comments: 20
Patch Set 5 : address comments from gab #Patch Set 6 : self review #
Total comments: 4
Patch Set 7 : address comments from gab #11 #Patch Set 8 : self review #
Total comments: 48
Patch Set 9 : address comments from robliao #13 #
Total comments: 17
Patch Set 10 : address comments from gab #16 #Patch Set 11 : Set sequenced time in Sequence::PushTask. #Patch Set 12 : remove scheduler lock from diff #
Total comments: 7
Patch Set 13 : Add comments. #Patch Set 14 : typo #Patch Set 15 : typo #Patch Set 16 : fix comment. #Patch Set 17 : rebase #Patch Set 18 : typo. #
Total comments: 4
Patch Set 19 : static_assert #
Total comments: 26
Patch Set 20 : CR from Dana #36-37 #
Total comments: 4
Patch Set 21 : CR from Dana and Gab #40-41 #
Messages
Total messages: 49 (12 generated)
Description was changed from ========== TaskScheduler [3/8] Sequence And Task R=gab@chromium.org, robliao@chromium.org ========== to ========== TaskScheduler [3/8] Task and Sequence R=gab@chromium.org, robliao@chromium.org ==========
Description was changed from ========== TaskScheduler [3/8] Task and Sequence R=gab@chromium.org, robliao@chromium.org ========== to ========== TaskScheduler [3/8] Task and Sequence 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 [3/8] Task and Sequence 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 [3/8] Task and Sequence This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ==========
https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... File base/task_scheduler/sequence.cc (right): https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... base/task_scheduler/sequence.cc:36: AutoSchedulerLock auto_lock(lock_); Put AutoSchedulerLock first, to be consistent with other methods. https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... base/task_scheduler/sequence.cc:50: TaskPriority priority = TaskPriority::BACKGROUND; This loop relies on the fact that the priority values are in [0, kNumTaskPriorities - 1]. Maybe we should add this in task_traits.h: // sequence.h has an array of counters indexed by the underlying values of TaskPriority. That means that the priority values must be in [0, kNumTaskPriorities - 1]. static_assert(static_cast<TaskPriorityUnderlyingType>(TaskPriority::BACKGROUND) == 0, "The value of the lower priority must be 0."); static_assert(static_cast<TaskPriorityUnderlyingType>(TaskPriority::USER_BLOCKING) == kNumTaskPriorities - 1, "The value of the highest priority must be kNumTaskPriorities - 1."); https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... base/task_scheduler/sequence.cc:51: for (TaskPriorityUnderlyingType i = kNumTaskPriorities - 1; i >= 0; --i) { static_assert(std::is_signed<TaskPriorityUnderlyingType>::value, ""); https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequence.h File base/task_scheduler/sequence.h (right): https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... base/task_scheduler/sequence.h:34: // Returns the sequence in front of the sequence's queue, if any. Returns the *task https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... File base/task_scheduler/sequence_sort_key.h (right): https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... base/task_scheduler/sequence_sort_key.h:25: // after the sequence associated with |other|. Or, more simply: Indicates whether the priority of this sort key is less than the priority of |other|. https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... base/task_scheduler/sequence_sort_key.h:36: // Highest task priority in the sequence at the time this sort key is created. *was created? https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... base/task_scheduler/sequence_sort_key.h:39: // Post time of the next task to run in the sequence. ... at the time this sort key was created. https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... File base/task_scheduler/sequence_unittest.cc (right): https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... base/task_scheduler/sequence_unittest.cc:35: Task task_a_; const Task task_a_; https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... base/task_scheduler/sequence_unittest.cc:87: TEST_F(TaskSchedulerSequenceTest, GetSequenceSortKey) { *GetSortKey https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/task.h File base/task_scheduler/task.h (right): https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/task.h#... base/task_scheduler/task.h:33: TimeTicks post_time; Is this clearer? // The time at which the task was inserted in its sequence. For an undelayed // task, this happens at post time. For a delayed task, happens after the task's // delay has expired. TimeTicks inserted_in_sequence_time;
lg, comments below, thanks! https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... File base/task_scheduler/sequence.cc (right): https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... base/task_scheduler/sequence.cc:14: Sequence::Sequence() : num_tasks_per_priority_() {} Does |num_task_per_priority_| really need to be explicitly initialized here? If so use "Class Member Initializers" (ref: https://chromium-cpp.appspot.com/) https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... base/task_scheduler/sequence.cc:36: AutoSchedulerLock auto_lock(lock_); On 2016/02/18 01:46:11, fdoray wrote: > Put AutoSchedulerLock first, to be consistent with other methods. I actually like having it second but would put an empty line after the DCHECK. The DCHECK verifies the contract of the method and should always come first (i.e. not even be considered part of the method's body, it's like the verification of the API contract before the body, e.g. what annotations are meant for in Java) Edit: Now I see that the next line is the DCHECK(!queue_.empty()); call which is also verifying the contract but requires the lock... sigh...! In that light I guess I agree and prefer the AutoLock at the top..! https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... base/task_scheduler/sequence.cc:50: TaskPriority priority = TaskPriority::BACKGROUND; On 2016/02/18 01:46:11, fdoray wrote: > This loop relies on the fact that the priority values are in [0, > kNumTaskPriorities - 1]. Maybe we should add this in task_traits.h: > > // sequence.h has an array of counters indexed by the underlying values of > TaskPriority. That means that the priority values must be in [0, > kNumTaskPriorities - 1]. > static_assert(static_cast<TaskPriorityUnderlyingType>(TaskPriority::BACKGROUND) > == 0, "The value of the lower priority must be 0."); > static_assert(static_cast<TaskPriorityUnderlyingType>(TaskPriority::USER_BLOCKING) > == kNumTaskPriorities - 1, "The value of the highest priority must be > kNumTaskPriorities - 1."); > Agreed, also line 57 in header ( size_t num_tasks_per_priority_[kNumTaskPriorities]; ) relies on them being sequential. Should we instead declare it as : namespace internal { using TaskPriorityUnderlyingType = uint8; } // namespace internal TaskPriority : TaskPriorityUnderlyingType { BACKGROUND = 0, USER_VISIBLE, USER_BLOCKING, HIGHEST = USER_BLOCKING } const TaskPriorityUnderlyingType kNumTaskPriorities = TaskPriority::HIGHEST + 1; and let it just be a zero-based list without assigning explicit value and thus not requiring tons of static assert? This design also nicely allows a task to pick TaskPriority::HIGHEST as a priority if it thinks it really wants to always be at the highest possible priority should we add one above USER_BLOCKING as discussed earlier today. Note that I also just had this thought that TaskPriorityUnderlyingType should probably be in the internal namespace (i.e. it's meant for us to manage internal state, it's not meant for people to use the underlying value from the API -- otherwise we'd just make it an enum, not an enum class). PS: Let's make these changes in this CL and let the other one land. https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... base/task_scheduler/sequence.cc:51: for (TaskPriorityUnderlyingType i = kNumTaskPriorities - 1; i >= 0; --i) { On 2016/02/18 01:46:11, fdoray wrote: > static_assert(std::is_signed<TaskPriorityUnderlyingType>::value, ""); Hmmm, why? I'm actually proposing using uint8 above. |kNumTaskPriorities - 1| is only unsafe is we ever think kNumTaskPriorities would be 0 which would be utterly crazy IMO! https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequence.h File base/task_scheduler/sequence.h (right): https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... base/task_scheduler/sequence.h:32: void PushTask(scoped_ptr<Task> task, size_t* prev_num_tasks); Return |prev_num_tasks| instead of using an out-param. Can this be a bool reflecting pre-post is_empty() state instead of a size_t actually? IIRC the impl only cares about comparing with 0? https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... base/task_scheduler/sequence.h:40: void PopTask(size_t* new_num_tasks); Same comment here, use return value and try to make it a bool. https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... base/task_scheduler/sequence.h:42: // Returns a sort key to use when inserting the sequence in a priority queue. s/sort key/SequenceSortKey/ https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... File base/task_scheduler/sequence_sort_key.cc (right): https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... base/task_scheduler/sequence_sort_key.cc:25: return next_task_post_time_ > other.next_task_post_time_; How about: -------------- // Sort by highest priority first, earliest post time second. const int priority_diff = static_cast<TaskPriorityUnderlyingType>(other.priority_) - static_cast<TaskPriorityUnderlyingType>(priority_); return priority_diff != 0 ? priority_diff < 0 : next_task_post_time_ > other.next_task_post_time_; -------------- Less lines, less branches, less returns :-) https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... File base/task_scheduler/sequence_sort_key.h (right): https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... base/task_scheduler/sequence_sort_key.h:22: SequenceSortKey(TaskPriority priority, TimeTicks next_task_post_time); Add comments on constructors to highlight differences between the two. https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... base/task_scheduler/sequence_sort_key.h:25: // after the sequence associated with |other|. On 2016/02/18 01:46:11, fdoray wrote: > Or, more simply: > > Indicates whether the priority of this sort key is less than the priority of > |other|. Or more simply: no comment as this is implicit from the definition of the class? https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... base/task_scheduler/sequence_sort_key.h:28: // Used for testing. Wouldn't bother with this comment. https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... base/task_scheduler/sequence_sort_key.h:34: FRIEND_TEST_ALL_PREFIXES(TaskSchedulerSequenceTest, GetSequenceSortKey); Let's make this a struct with public members, doesn't matter since it's an immutable structure with const members anyways. Then we don't need getters, nor friending. (note style says that struct members don't end in with a _ suffix -- just in case to avoid a second nit round on that) https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... base/task_scheduler/sequence_sort_key.h:36: // Highest task priority in the sequence at the time this sort key is created. On 2016/02/18 01:46:11, fdoray wrote: > *was created? Yes. https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... base/task_scheduler/sequence_sort_key.h:37: TaskPriority priority_; const https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... base/task_scheduler/sequence_sort_key.h:40: TimeTicks next_task_post_time_; const https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... File base/task_scheduler/sequence_unittest.cc (right): https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... base/task_scheduler/sequence_unittest.cc:39: }; private: DISALLOW_COPY_AND_ASSIGN(TaskSchedulerSequenceTest); (even for test fixtures -- it's not done as religiously in the test side of the codebase but there is no reason not to and style rules that mention to use it don't disqualify tests) https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/task.h File base/task_scheduler/task.h (right): https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/task.h#... base/task_scheduler/task.h:18: // A task is a unit of work inside the task scheduler. Add to the comment that support for tracing is inherited from PendingTask. https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/task.h#... base/task_scheduler/task.h:24: const TimeTicks& post_time); Comments on constructor to highlight differences. https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/task.h#... base/task_scheduler/task.h:27: // The traits describing where, when and how the task should run. // The TaskTraits of this task. (what those are is better described in task_traits.h and anybody that cares will know to look there through the type) https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/task.h#... base/task_scheduler/task.h:33: TimeTicks post_time; On 2016/02/18 01:46:11, fdoray wrote: > Is this clearer? > > // The time at which the task was inserted in its sequence. For an undelayed > // task, this happens at post time. For a delayed task, happens after the task's > // delay has expired. > TimeTicks inserted_in_sequence_time; Actually, for a delayed task isn't this the time at which it was intended to run? I guess our Sequence doesn't support inserting anywhere but at the back and expects sorted post times so we can't really do this. Adding an extra delay is fine for now for BACKGROUND work but won't be later for precise timer-based work in the renderers; just a thought... So I guess I'm okay with slight tweaks: // The time at which the task was inserted in its sequence. For an undelayed // task, this happens at post time. For a delayed task, this happens some // time after the task's delay has expired. TimeTicks sequenced_time; Also, how about |sequenced_time|, i.e. 4 phases: post_time <= sequenced_time <= scheduled_time <= execution_time ? I guess "scheduled" and "execution" are almost the same (i.e to me scheduled is picked up by a thread and execution comes almost right away at that point) -- whatever, beyond this discussion but just mentioning this because I initially thought about |scheduled_time| for this variable but realized "scheduled" was later. https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/test_ut... File base/task_scheduler/test_util.cc (right): https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/test_ut... base/task_scheduler/test_util.cc:24: default: No default, without the default clang compile will break if someone adds a new TaskPriority without adjusting this :-). In general always avoid default in switches when you can. https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/test_ut... File base/task_scheduler/test_util.h (right): https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/test_ut... base/task_scheduler/test_util.h:15: void PrintTo(const TaskPriority& task_priority, std::ostream* os); You can put this at the bottom of task_traits.h; then anybody using it inherits this without having to know about a special header. Even string16.h has a PrintTo() (IIRC that was my actually first commit..!)
all done https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... File base/task_scheduler/sequence.cc (right): https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... base/task_scheduler/sequence.cc:14: Sequence::Sequence() : num_tasks_per_priority_() {} On 2016/02/18 03:00:45, gab wrote: > Does |num_task_per_priority_| really need to be explicitly initialized here? > > If so use "Class Member Initializers" (ref: https://chromium-cpp.appspot.com/) http://stackoverflow.com/questions/15212261/default-initialization-of-pod-typ... says that pod types are not initialized by default. I now use a class member initializer. https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... base/task_scheduler/sequence.cc:36: AutoSchedulerLock auto_lock(lock_); On 2016/02/18 03:00:45, gab wrote: > On 2016/02/18 01:46:11, fdoray wrote: > > Put AutoSchedulerLock first, to be consistent with other methods. > > I actually like having it second but would put an empty line after the DCHECK. > > The DCHECK verifies the contract of the method and should always come first > (i.e. not even be considered part of the method's body, it's like the > verification of the API contract before the body, e.g. what annotations are > meant for in Java) > > Edit: Now I see that the next line is the DCHECK(!queue_.empty()); call which is > also verifying the contract but requires the lock... sigh...! In that light I > guess I agree and prefer the AutoLock at the top..! Done (new_num_tasks is gone) https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... base/task_scheduler/sequence.cc:51: for (TaskPriorityUnderlyingType i = kNumTaskPriorities - 1; i >= 0; --i) { On 2016/02/18 03:00:45, gab wrote: > On 2016/02/18 01:46:11, fdoray wrote: > > static_assert(std::is_signed<TaskPriorityUnderlyingType>::value, ""); > > Hmmm, why? I'm actually proposing using uint8 above. |kNumTaskPriorities - 1| is > only unsafe is we ever think kNumTaskPriorities would be 0 which would be > utterly crazy IMO! |i| needs to be signed. Otherwise, the i >=0 condition will always be true. But you're right that the enum type doesn't have to be signed. https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequence.h File base/task_scheduler/sequence.h (right): https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... base/task_scheduler/sequence.h:32: void PushTask(scoped_ptr<Task> task, size_t* prev_num_tasks); On 2016/02/18 03:00:46, gab wrote: > Return |prev_num_tasks| instead of using an out-param. > > Can this be a bool reflecting pre-post is_empty() state instead of a size_t > actually? IIRC the impl only cares about comparing with 0? Done. https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... base/task_scheduler/sequence.h:34: // Returns the sequence in front of the sequence's queue, if any. On 2016/02/18 01:46:11, fdoray wrote: > Returns the *task Done. https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... base/task_scheduler/sequence.h:40: void PopTask(size_t* new_num_tasks); On 2016/02/18 03:00:46, gab wrote: > Same comment here, use return value and try to make it a bool. Done. https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... base/task_scheduler/sequence.h:42: // Returns a sort key to use when inserting the sequence in a priority queue. On 2016/02/18 03:00:46, gab wrote: > s/sort key/SequenceSortKey/ Done. https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... File base/task_scheduler/sequence_sort_key.cc (right): https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... base/task_scheduler/sequence_sort_key.cc:25: return next_task_post_time_ > other.next_task_post_time_; On 2016/02/18 03:00:46, gab wrote: > How about: > > -------------- > // Sort by highest priority first, earliest post time second. > const int priority_diff = > static_cast<TaskPriorityUnderlyingType>(other.priority_) - > static_cast<TaskPriorityUnderlyingType>(priority_); > return priority_diff != 0 > ? priority_diff < 0 > : next_task_post_time_ > other.next_task_post_time_; > -------------- > > Less lines, less branches, less returns :-) Done. Note: priority_diff > 0 instead of priority_diff < 0. https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... File base/task_scheduler/sequence_sort_key.h (right): https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... base/task_scheduler/sequence_sort_key.h:22: SequenceSortKey(TaskPriority priority, TimeTicks next_task_post_time); On 2016/02/18 03:00:46, gab wrote: > Add comments on constructors to highlight differences between the two. Default constructor is not needed. https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... base/task_scheduler/sequence_sort_key.h:25: // after the sequence associated with |other|. On 2016/02/18 03:00:46, gab wrote: > On 2016/02/18 01:46:11, fdoray wrote: > > Or, more simply: > > > > Indicates whether the priority of this sort key is less than the priority of > > |other|. > > Or more simply: no comment as this is implicit from the definition of the class? Done. https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... base/task_scheduler/sequence_sort_key.h:28: // Used for testing. On 2016/02/18 03:00:46, gab wrote: > Wouldn't bother with this comment. Done. https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... base/task_scheduler/sequence_sort_key.h:34: FRIEND_TEST_ALL_PREFIXES(TaskSchedulerSequenceTest, GetSequenceSortKey); On 2016/02/18 03:00:46, gab wrote: > Let's make this a struct with public members, doesn't matter since it's an > immutable structure with const members anyways. > > Then we don't need getters, nor friending. > > (note style says that struct members don't end in with a _ suffix -- just in > case to avoid a second nit round on that) As explained below, the members can't be const. I don't want non-const public members, because it would no longer be guaranteed that a SequenceSortKey is never modified. https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... base/task_scheduler/sequence_sort_key.h:36: // Highest task priority in the sequence at the time this sort key is created. On 2016/02/18 03:00:46, gab wrote: > On 2016/02/18 01:46:11, fdoray wrote: > > *was created? > > Yes. Done. https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... base/task_scheduler/sequence_sort_key.h:37: TaskPriority priority_; On 2016/02/18 03:00:46, gab wrote: > const Sadly, std::priority_queue needs operator=, which can't exist if there is const members. https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... base/task_scheduler/sequence_sort_key.h:39: // Post time of the next task to run in the sequence. On 2016/02/18 01:46:11, fdoray wrote: > ... at the time this sort key was created. Done. https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... base/task_scheduler/sequence_sort_key.h:40: TimeTicks next_task_post_time_; On 2016/02/18 03:00:46, gab wrote: > const Same comment as above. https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... File base/task_scheduler/sequence_unittest.cc (right): https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... base/task_scheduler/sequence_unittest.cc:35: Task task_a_; On 2016/02/18 01:46:11, fdoray wrote: > const Task task_a_; Done. https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... base/task_scheduler/sequence_unittest.cc:39: }; On 2016/02/18 03:00:46, gab wrote: > private: > DISALLOW_COPY_AND_ASSIGN(TaskSchedulerSequenceTest); > > (even for test fixtures -- it's not done as religiously in the test side of the > codebase but there is no reason not to and style rules that mention to use it > don't disqualify tests) Done. https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... base/task_scheduler/sequence_unittest.cc:87: TEST_F(TaskSchedulerSequenceTest, GetSequenceSortKey) { On 2016/02/18 01:46:11, fdoray wrote: > *GetSortKey Done. https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/task.h File base/task_scheduler/task.h (right): https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/task.h#... base/task_scheduler/task.h:18: // A task is a unit of work inside the task scheduler. On 2016/02/18 03:00:46, gab wrote: > Add to the comment that support for tracing is inherited from PendingTask. Done. https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/task.h#... base/task_scheduler/task.h:24: const TimeTicks& post_time); On 2016/02/18 03:00:46, gab wrote: > Comments on constructor to highlight differences. Default constructor is not needed. https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/task.h#... base/task_scheduler/task.h:27: // The traits describing where, when and how the task should run. On 2016/02/18 03:00:46, gab wrote: > // The TaskTraits of this task. > > (what those are is better described in task_traits.h and anybody that cares will > know to look there through the type) Done. https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/task.h#... base/task_scheduler/task.h:33: TimeTicks post_time; On 2016/02/18 03:00:46, gab wrote: > On 2016/02/18 01:46:11, fdoray wrote: > > Is this clearer? > > > > // The time at which the task was inserted in its sequence. For an undelayed > > // task, this happens at post time. For a delayed task, happens after the > task's > > // delay has expired. > > TimeTicks inserted_in_sequence_time; > > Actually, for a delayed task isn't this the time at which it was intended to > run? I guess our Sequence doesn't support inserting anywhere but at the back and > expects sorted post times so we can't really do this. > > Adding an extra delay is fine for now for BACKGROUND work but won't be later for > precise timer-based work in the renderers; just a thought... > > So I guess I'm okay with slight tweaks: > > // The time at which the task was inserted in its sequence. For an undelayed > // task, this happens at post time. For a delayed task, this happens some > // time after the task's delay has expired. > TimeTicks sequenced_time; > > Also, how about |sequenced_time|, i.e. 4 phases: post_time <= sequenced_time <= > scheduled_time <= execution_time ? I guess "scheduled" and "execution" are > almost the same (i.e to me scheduled is picked up by a thread and execution > comes almost right away at that point) -- whatever, beyond this discussion but > just mentioning this because I initially thought about |scheduled_time| for this > variable but realized "scheduled" was later. Done. There's no need to add |scheduled_time| to this struct, right? https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/test_ut... File base/task_scheduler/test_util.cc (right): https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/test_ut... base/task_scheduler/test_util.cc:24: default: On 2016/02/18 03:00:46, gab wrote: > No default, without the default clang compile will break if someone adds a new > TaskPriority without adjusting this :-). > > In general always avoid default in switches when you can. Done.
lvg, just a couple last things. https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... File base/task_scheduler/sequence_sort_key.cc (right): https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... base/task_scheduler/sequence_sort_key.cc:25: return next_task_post_time_ > other.next_task_post_time_; On 2016/02/18 14:56:12, fdoray wrote: > On 2016/02/18 03:00:46, gab wrote: > > How about: > > > > -------------- > > // Sort by highest priority first, earliest post time second. > > const int priority_diff = > > static_cast<TaskPriorityUnderlyingType>(other.priority_) - > > static_cast<TaskPriorityUnderlyingType>(priority_); > > return priority_diff != 0 > > ? priority_diff < 0 > > : next_task_post_time_ > other.next_task_post_time_; > > -------------- > > > > Less lines, less branches, less returns :-) > > Done. Note: priority_diff > 0 instead of priority_diff < 0. Ah oops, that's because I meant for priority_diff to be |priority_ - other.priority_|. i.e. in an operator< I prefer to compare with a < operator (except on time where higher post time means lesser priority). https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... File base/task_scheduler/sequence_sort_key.h (right): https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... base/task_scheduler/sequence_sort_key.h:37: TaskPriority priority_; On 2016/02/18 14:56:12, fdoray wrote: > On 2016/02/18 03:00:46, gab wrote: > > const > > Sadly, std::priority_queue needs operator=, which can't exist if there is const > members. Ugh :-(... really?! It does in place replacements instead of moving things around?! Feels we should use pointers in std::priority_queue instead then? I guess using pointers to avoid copy of a uint8_t and a uint64_t is not required for performance, but it feels wrong as-is... g2g and can't think of something right now, but I'd much prefer these being const and having DISALLOW_COPY_AND_ASSIGN as usual if at all possible.. https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/task.h File base/task_scheduler/task.h (right): https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/task.h#... base/task_scheduler/task.h:33: TimeTicks post_time; On 2016/02/18 14:56:13, fdoray wrote: > On 2016/02/18 03:00:46, gab wrote: > > On 2016/02/18 01:46:11, fdoray wrote: > > > Is this clearer? > > > > > > // The time at which the task was inserted in its sequence. For an undelayed > > > // task, this happens at post time. For a delayed task, happens after the > > task's > > > // delay has expired. > > > TimeTicks inserted_in_sequence_time; > > > > Actually, for a delayed task isn't this the time at which it was intended to > > run? I guess our Sequence doesn't support inserting anywhere but at the back > and > > expects sorted post times so we can't really do this. > > > > Adding an extra delay is fine for now for BACKGROUND work but won't be later > for > > precise timer-based work in the renderers; just a thought... > > > > So I guess I'm okay with slight tweaks: > > > > // The time at which the task was inserted in its sequence. For an undelayed > > // task, this happens at post time. For a delayed task, this happens some > > // time after the task's delay has expired. > > TimeTicks sequenced_time; > > > > Also, how about |sequenced_time|, i.e. 4 phases: post_time <= sequenced_time > <= > > scheduled_time <= execution_time ? I guess "scheduled" and "execution" are > > almost the same (i.e to me scheduled is picked up by a thread and execution > > comes almost right away at that point) -- whatever, beyond this discussion but > > just mentioning this because I initially thought about |scheduled_time| for > this > > variable but realized "scheduled" was later. > > Done. There's no need to add |scheduled_time| to this struct, right? Right, sorry for making this comment so long and confusing... I was thinking about a name out loud and spewed my reasoning here..! https://codereview.chromium.org/1705253002/diff/60001/base/task_scheduler/seq... File base/task_scheduler/sequence.cc (right): https://codereview.chromium.org/1705253002/diff/60001/base/task_scheduler/seq... base/task_scheduler/sequence.cc:51: TaskPriority priority = TaskPriority::BACKGROUND; One way to avoid the signed int below would be to add TaskPriority::LOWEST. Then since the default value here is initialized to TaskPriority::LOWEST we can loop only until i > TaskPriority::LOWEST :-) Also avoids having this code depend on a specific TaskPriority which is better if we change values later. WDYT? https://codereview.chromium.org/1705253002/diff/60001/base/task_scheduler/seq... base/task_scheduler/sequence.cc:52: for (int i = kNumTaskPriorities - 1; i >= 0; --i) { Ah nice, that's actually nicer too, I used to think it was weird to use TaskPriorityUnderlyingType as the type of kNumTaskPriorities. Using size_t for it and int here is nicer IMO :-). https://codereview.chromium.org/1705253002/diff/60001/base/task_scheduler/seq... File base/task_scheduler/sequence.h (right): https://codereview.chromium.org/1705253002/diff/60001/base/task_scheduler/seq... base/task_scheduler/sequence.h:31: // sequence was empty right before the push. s/right before the push/before this operation/ https://codereview.chromium.org/1705253002/diff/60001/base/task_scheduler/seq... base/task_scheduler/sequence.h:38: // sequence is empty right after the pop. Cannot be called on an empty s/right after the pop/after this operation/ https://codereview.chromium.org/1705253002/diff/60001/base/task_scheduler/seq... File base/task_scheduler/sequence_sort_key.cc (right): https://codereview.chromium.org/1705253002/diff/60001/base/task_scheduler/seq... base/task_scheduler/sequence_sort_key.cc:16: // Sort by highest priority first, earliest post time second. Actually (sorry I know I wrote this...), how about we be even more explicit here and say: // This SequenceSortKey is considered less important than |other| if its lower priority or if it has the same priority but its next task was posted later than |other|'s. (the reason I'm having second thoughts is that I realized it isn't obvious that < means lower priority (if you think of a queue as a vector then being <, i.e. on the LHS, actually means you're first...) https://codereview.chromium.org/1705253002/diff/60001/base/task_scheduler/seq... File base/task_scheduler/sequence_unittest.cc (right): https://codereview.chromium.org/1705253002/diff/60001/base/task_scheduler/seq... base/task_scheduler/sequence_unittest.cc:126: sequence->PopTask(); Remove this last statement given we're not testing anything from it nor after it? https://codereview.chromium.org/1705253002/diff/60001/base/task_scheduler/tas... File base/task_scheduler/task_traits.cc (right): https://codereview.chromium.org/1705253002/diff/60001/base/task_scheduler/tas... base/task_scheduler/task_traits.cc:45: case TaskPriority::BACKGROUND: Put BACKGROUND first now that we flipped the enum upside down. https://codereview.chromium.org/1705253002/diff/60001/base/task_scheduler/tas... File base/task_scheduler/task_traits.h (right): https://codereview.chromium.org/1705253002/diff/60001/base/task_scheduler/tas... base/task_scheduler/task_traits.h:21: // Valid priorities supported by the task scheduler. Append something like this to the comment : "Note: internal algorithms depend on priorities being expressed as a continuous zero-based list from lowest to highest priority. Users of this API shouldn't otherwise care about nor use the underlying values."
1*2 more thing(s) :-) https://codereview.chromium.org/1705253002/diff/60001/base/task_scheduler/seq... File base/task_scheduler/sequence.h (right): https://codereview.chromium.org/1705253002/diff/60001/base/task_scheduler/seq... base/task_scheduler/sequence.h:35: const Task* PeekTask(); const method (you can make |lock_| mutable) https://codereview.chromium.org/1705253002/diff/60001/base/task_scheduler/seq... base/task_scheduler/sequence.h:44: SequenceSortKey GetSortKey(); const method as well
https://codereview.chromium.org/1705253002/diff/60001/base/task_scheduler/seq... File base/task_scheduler/sequence.cc (right): https://codereview.chromium.org/1705253002/diff/60001/base/task_scheduler/seq... base/task_scheduler/sequence.cc:51: TaskPriority priority = TaskPriority::BACKGROUND; On 2016/02/18 20:22:34, gab wrote: > One way to avoid the signed int below would be to add TaskPriority::LOWEST. Then > since the default value here is initialized to TaskPriority::LOWEST we can loop > only until i > TaskPriority::LOWEST :-) > > Also avoids having this code depend on a specific TaskPriority which is better > if we change values later. > > WDYT? Done. https://codereview.chromium.org/1705253002/diff/60001/base/task_scheduler/seq... base/task_scheduler/sequence.cc:52: for (int i = kNumTaskPriorities - 1; i >= 0; --i) { On 2016/02/18 20:22:34, gab wrote: > Ah nice, that's actually nicer too, I used to think it was weird to use > TaskPriorityUnderlyingType as the type of kNumTaskPriorities. Using size_t for > it and int here is nicer IMO :-). Done. https://codereview.chromium.org/1705253002/diff/60001/base/task_scheduler/seq... File base/task_scheduler/sequence.h (right): https://codereview.chromium.org/1705253002/diff/60001/base/task_scheduler/seq... base/task_scheduler/sequence.h:31: // sequence was empty right before the push. On 2016/02/18 20:22:34, gab wrote: > s/right before the push/before this operation/ Done. https://codereview.chromium.org/1705253002/diff/60001/base/task_scheduler/seq... base/task_scheduler/sequence.h:35: const Task* PeekTask(); On 2016/02/18 20:28:10, gab wrote: > const method (you can make |lock_| mutable) Done. https://codereview.chromium.org/1705253002/diff/60001/base/task_scheduler/seq... base/task_scheduler/sequence.h:38: // sequence is empty right after the pop. Cannot be called on an empty On 2016/02/18 20:22:34, gab wrote: > s/right after the pop/after this operation/ Done. https://codereview.chromium.org/1705253002/diff/60001/base/task_scheduler/seq... base/task_scheduler/sequence.h:44: SequenceSortKey GetSortKey(); On 2016/02/18 20:28:10, gab wrote: > const method as well Done. https://codereview.chromium.org/1705253002/diff/60001/base/task_scheduler/seq... File base/task_scheduler/sequence_sort_key.cc (right): https://codereview.chromium.org/1705253002/diff/60001/base/task_scheduler/seq... base/task_scheduler/sequence_sort_key.cc:16: // Sort by highest priority first, earliest post time second. On 2016/02/18 20:22:34, gab wrote: > Actually (sorry I know I wrote this...), how about we be even more explicit here > and say: > > // This SequenceSortKey is considered less important than |other| if its lower > priority or if it has the same priority but its next task was posted later than > |other|'s. > > (the reason I'm having second thoughts is that I realized it isn't obvious that > < means lower priority (if you think of a queue as a vector then being <, i.e. > on the LHS, actually means you're first...) Done. https://codereview.chromium.org/1705253002/diff/60001/base/task_scheduler/seq... File base/task_scheduler/sequence_unittest.cc (right): https://codereview.chromium.org/1705253002/diff/60001/base/task_scheduler/seq... base/task_scheduler/sequence_unittest.cc:126: sequence->PopTask(); On 2016/02/18 20:22:34, gab wrote: > Remove this last statement given we're not testing anything from it nor after > it? Done. https://codereview.chromium.org/1705253002/diff/60001/base/task_scheduler/tas... File base/task_scheduler/task_traits.cc (right): https://codereview.chromium.org/1705253002/diff/60001/base/task_scheduler/tas... base/task_scheduler/task_traits.cc:45: case TaskPriority::BACKGROUND: On 2016/02/18 20:22:34, gab wrote: > Put BACKGROUND first now that we flipped the enum upside down. Done. https://codereview.chromium.org/1705253002/diff/60001/base/task_scheduler/tas... File base/task_scheduler/task_traits.h (right): https://codereview.chromium.org/1705253002/diff/60001/base/task_scheduler/tas... base/task_scheduler/task_traits.h:21: // Valid priorities supported by the task scheduler. On 2016/02/18 20:22:34, gab wrote: > Append something like this to the comment : "Note: internal algorithms depend on > priorities being expressed as a continuous zero-based list from lowest to > highest priority. Users of this API shouldn't otherwise care about nor use the > underlying values." Done.
https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... File base/task_scheduler/sequence_sort_key.h (right): https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... base/task_scheduler/sequence_sort_key.h:37: TaskPriority priority_; On 2016/02/18 20:22:34, gab wrote: > On 2016/02/18 14:56:12, fdoray wrote: > > On 2016/02/18 03:00:46, gab wrote: > > > const > > > > Sadly, std::priority_queue needs operator=, which can't exist if there is > const > > members. > > Ugh :-(... really?! It does in place replacements instead of moving things > around?! Feels we should use pointers in std::priority_queue instead then? I > guess using pointers to avoid copy of a uint8_t and a uint64_t is not required > for performance, but it feels wrong as-is... > > g2g and can't think of something right now, but I'd much prefer these being > const and having DISALLOW_COPY_AND_ASSIGN as usual if at all possible.. Resurfacing this. https://codereview.chromium.org/1705253002/diff/100001/base/task_scheduler/se... File base/task_scheduler/sequence.cc (right): https://codereview.chromium.org/1705253002/diff/100001/base/task_scheduler/se... base/task_scheduler/sequence.cc:52: for (size_t i = kNumTaskPriorities - 1; Actually it probably makes more sense to start at HIGHEST here? Then this can directly be the task type instead of int. https://codereview.chromium.org/1705253002/diff/100001/base/task_scheduler/se... File base/task_scheduler/sequence_sort_key.cc (right): https://codereview.chromium.org/1705253002/diff/100001/base/task_scheduler/se... base/task_scheduler/sequence_sort_key.cc:21: static_cast<TaskPriorityUnderlyingType>(priority_); Ping on flipping these two (see reply to other comment)
gab@: Can you take another look? https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... File base/task_scheduler/sequence_sort_key.h (right): https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/sequenc... base/task_scheduler/sequence_sort_key.h:37: TaskPriority priority_; On 2016/02/18 21:21:16, gab wrote: > On 2016/02/18 20:22:34, gab wrote: > > On 2016/02/18 14:56:12, fdoray wrote: > > > On 2016/02/18 03:00:46, gab wrote: > > > > const > > > > > > Sadly, std::priority_queue needs operator=, which can't exist if there is > > const > > > members. > > > > Ugh :-(... really?! It does in place replacements instead of moving things > > around?! Feels we should use pointers in std::priority_queue instead then? I > > guess using pointers to avoid copy of a uint8_t and a uint64_t is not required > > for performance, but it feels wrong as-is... > > > > g2g and can't think of something right now, but I'd much prefer these being > > const and having DISALLOW_COPY_AND_ASSIGN as usual if at all possible.. > > Resurfacing this. Done. https://codereview.chromium.org/1705253002/diff/100001/base/task_scheduler/se... File base/task_scheduler/sequence.cc (right): https://codereview.chromium.org/1705253002/diff/100001/base/task_scheduler/se... base/task_scheduler/sequence.cc:52: for (size_t i = kNumTaskPriorities - 1; On 2016/02/18 21:21:16, gab wrote: > Actually it probably makes more sense to start at HIGHEST here? > > Then this can directly be the task type instead of int. Done. https://codereview.chromium.org/1705253002/diff/100001/base/task_scheduler/se... File base/task_scheduler/sequence_sort_key.cc (right): https://codereview.chromium.org/1705253002/diff/100001/base/task_scheduler/se... base/task_scheduler/sequence_sort_key.cc:21: static_cast<TaskPriorityUnderlyingType>(priority_); On 2016/02/18 21:21:16, gab wrote: > Ping on flipping these two (see reply to other comment) Done.
An initial pass https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/se... File base/task_scheduler/sequence.cc (right): https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/se... base/task_scheduler/sequence.cc:19: ++num_tasks_per_priority_[static_cast<TaskPriorityUnderlyingType>( Maybe a temporary for the static_cast would make this more readable: TaskPriorityUnderlyingType index_priority = static_cast<TaskPriorityUnderlyingType>(task->traits.priority()); ++num_tasks_per_priority_[index_priority] Same with below. https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/se... base/task_scheduler/sequence.cc:38: DCHECK(!queue_.empty()); I'm thinking this should be a CHECK. It is undefined what happens when you pop an empty queue (so no exception may be thrown). Ideally, it would crash. We don't rely on this behavior, do we? https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/se... File base/task_scheduler/sequence.h (right): https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/se... base/task_scheduler/sequence.h:24: // A sequence holds tasks that must be executed in posting order. This class is Nit: Linebreak after the period here is okay. https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/se... base/task_scheduler/sequence.h:50: // Controls access to all members. Nit: Synchronizes access to all members. https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/se... base/task_scheduler/sequence.h:57: static const size_t kNumTaskPriorities = Shouldn't this be taken from task_traits.h? https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/se... base/task_scheduler/sequence.h:58: static_cast<TaskPriorityUnderlyingType>(TaskPriority::HIGHEST) + 1; This seems like something that should remain at or near TaskPriority. You can static_assert(TaskPriority::HIGHEST == kNumTaskPriorities + 1) in the implementation file. https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/se... File base/task_scheduler/sequence_sort_key.cc (right): https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/se... base/task_scheduler/sequence_sort_key.cc:22: return priority_diff != 0 Remove the double negative by changing this to == and inverting the two branches below. https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/se... File base/task_scheduler/sequence_sort_key.h (right): https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/se... base/task_scheduler/sequence_sort_key.h:16: // Contrary to a Sequence, which can be modified when it is inside a // An immutable representation of the priority of a Sequence. https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/se... base/task_scheduler/sequence_sort_key.h:18: struct BASE_EXPORT SequenceSortKey { Nit: Add final. https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/se... base/task_scheduler/sequence_sort_key.h:22: bool operator<(const SequenceSortKey& other) const; Do we need to support operator reflexivity? a < b is the same as b > a https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/se... base/task_scheduler/sequence_sort_key.h:27: const TaskPriority priority_; Will this immutability cause problems in STL containers? (e.g., we can never have a vector of SequenceSortKeys). https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/se... File base/task_scheduler/sequence_sort_key_unittest.cc (right): https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/se... base/task_scheduler/sequence_sort_key_unittest.cc:14: TEST(TaskSchedulerSequenceSortKeyTest, Comparison) { Add some equality tests too. https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/se... base/task_scheduler/sequence_sort_key_unittest.cc:16: TimeTicks::FromInternalValue(0)); Go for some values other than 0 and 1 for the TimeTicks. https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/ta... File base/task_scheduler/task.h (right): https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/ta... base/task_scheduler/task.h:33: TimeTicks sequenced_time; This should probably be const. I suspect that impacts STL container usage, so you can turn this into a class and make these private and add getters. https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/ta... File base/task_scheduler/task_traits.cc (right): https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/ta... base/task_scheduler/task_traits.cc:37: void PrintTo(const TaskPriority& task_priority, std::ostream* os) { Can this go in a test only utils file? https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/ta... base/task_scheduler/task_traits.cc:47: break; default case: DCHECK(false). https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/ta... File base/task_scheduler/task_traits.h (right): https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/ta... base/task_scheduler/task_traits.h:131: void BASE_EXPORT PrintTo(const TaskPriority& task_priority, std::ostream* os); Can this go in a test only utils file? https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/test/gfx_ut... is an example of this.
robliao@: All done. https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/se... File base/task_scheduler/sequence.cc (right): https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/se... base/task_scheduler/sequence.cc:19: ++num_tasks_per_priority_[static_cast<TaskPriorityUnderlyingType>( On 2016/02/19 02:33:43, robliao wrote: > Maybe a temporary for the static_cast would make this more readable: > TaskPriorityUnderlyingType index_priority = > static_cast<TaskPriorityUnderlyingType>(task->traits.priority()); > ++num_tasks_per_priority_[index_priority] > > Same with below. Done. https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/se... base/task_scheduler/sequence.cc:38: DCHECK(!queue_.empty()); On 2016/02/19 02:33:43, robliao wrote: > I'm thinking this should be a CHECK. It is undefined what happens when you pop > an empty queue (so no exception may be thrown). Ideally, it would crash. > > We don't rely on this behavior, do we? Done. https://www.chromium.org/developers/coding-style says "Use CHECK() if the consequence of a failed assertion would be a security vulnerability". It is an error to call PopTask on an empty sequence. If it happens, something is wrong and I guess it is better to crash than to have an undefined behavior. https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/se... base/task_scheduler/sequence.cc:48: DCHECK(!queue_.empty()); CHECK here too, because front() has an undefined behavior on an empty queue? https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/se... File base/task_scheduler/sequence.h (right): https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/se... base/task_scheduler/sequence.h:24: // A sequence holds tasks that must be executed in posting order. This class is On 2016/02/19 02:33:43, robliao wrote: > Nit: Linebreak after the period here is okay. Done. https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/se... base/task_scheduler/sequence.h:50: // Controls access to all members. On 2016/02/19 02:33:43, robliao wrote: > Nit: Synchronizes access to all members. Done. https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/se... base/task_scheduler/sequence.h:57: static const size_t kNumTaskPriorities = On 2016/02/19 02:33:43, robliao wrote: > Shouldn't this be taken from task_traits.h? Done. https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/se... base/task_scheduler/sequence.h:58: static_cast<TaskPriorityUnderlyingType>(TaskPriority::HIGHEST) + 1; On 2016/02/19 02:33:43, robliao wrote: > This seems like something that should remain at or near TaskPriority. You can > static_assert(TaskPriority::HIGHEST == kNumTaskPriorities + 1) in the > implementation file. Done. https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/se... File base/task_scheduler/sequence_sort_key.cc (right): https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/se... base/task_scheduler/sequence_sort_key.cc:22: return priority_diff != 0 On 2016/02/19 02:33:43, robliao wrote: > Remove the double negative by changing this to == and inverting the two branches > below. Done. https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/se... File base/task_scheduler/sequence_sort_key.h (right): https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/se... base/task_scheduler/sequence_sort_key.h:16: // Contrary to a Sequence, which can be modified when it is inside a On 2016/02/19 02:33:43, robliao wrote: > // An immutable representation of the priority of a Sequence. Done. https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/se... base/task_scheduler/sequence_sort_key.h:18: struct BASE_EXPORT SequenceSortKey { On 2016/02/19 02:33:43, robliao wrote: > Nit: Add final. Done. https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/se... base/task_scheduler/sequence_sort_key.h:22: bool operator<(const SequenceSortKey& other) const; On 2016/02/19 02:33:43, robliao wrote: > Do we need to support operator reflexivity? > a < b is the same as b > a We need operator< to sort sequences in PriorityQueue. I didn't implement operator> because we don't need it. Is it OK? https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/se... base/task_scheduler/sequence_sort_key.h:27: const TaskPriority priority_; On 2016/02/19 02:33:43, robliao wrote: > Will this immutability cause problems in STL containers? (e.g., we can never > have a vector of SequenceSortKeys). You're right, we won't be able to create STL containers of SequenceSortKeys. PriorityQueue will have an std::priority_queue<SequenceAndSortKeyPair*> instead of an std::priority_queue<SequenceAndSortKeyPair>. See gab's comment here https://codereview.chromium.org/1709713002/diff/1/base/task_scheduler/priorit... https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/se... File base/task_scheduler/sequence_sort_key_unittest.cc (right): https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/se... base/task_scheduler/sequence_sort_key_unittest.cc:14: TEST(TaskSchedulerSequenceSortKeyTest, Comparison) { On 2016/02/19 02:33:43, robliao wrote: > Add some equality tests too. Done. https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/se... base/task_scheduler/sequence_sort_key_unittest.cc:16: TimeTicks::FromInternalValue(0)); On 2016/02/19 02:33:43, robliao wrote: > Go for some values other than 0 and 1 for the TimeTicks. Done. https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/ta... File base/task_scheduler/task.h (right): https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/ta... base/task_scheduler/task.h:33: TimeTicks sequenced_time; On 2016/02/19 02:33:43, robliao wrote: > This should probably be const. I suspect that impacts STL container usage, so > you can turn this into a class and make these private and add getters. It can't be const because for a delayed task, it isn't set when the task is constructed. This is currently set by TaskRunners/DelayedTaskManager before pushing the task in a sequence, but it should probably be set by Sequence::PushTask instead. WDYT? https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/ta... File base/task_scheduler/task_traits.cc (right): https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/ta... base/task_scheduler/task_traits.cc:37: void PrintTo(const TaskPriority& task_priority, std::ostream* os) { On 2016/02/19 02:33:43, robliao wrote: > Can this go in a test only utils file? See comment in task_traits.h https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/ta... base/task_scheduler/task_traits.cc:47: break; On 2016/02/19 02:33:43, robliao wrote: > default case: DCHECK(false). See gab's comment https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/test_ut... https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/ta... File base/task_scheduler/task_traits.h (right): https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/ta... base/task_scheduler/task_traits.h:131: void BASE_EXPORT PrintTo(const TaskPriority& task_priority, std::ostream* os); On 2016/02/19 02:33:43, robliao wrote: > Can this go in a test only utils file? > https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/test/gfx_ut... > is an example of this. It was in a test only file, but gab asked me to move it here to match string16.h. A benefit of having this here is that we don't have to think about including the test-only file to get pretty-printing. What should I do now?
https://codereview.chromium.org/1705253002/diff/160001/base/task_scheduler/sc... File base/task_scheduler/scheduler_lock.cc (left): https://codereview.chromium.org/1705253002/diff/160001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock.cc:135: return std::move(cond_var); oups... this shouldn't have been in this CL. Will update the patch soon.
Comments and comments on comments :-), almost there! https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/se... File base/task_scheduler/sequence.cc (right): https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/se... base/task_scheduler/sequence.cc:38: DCHECK(!queue_.empty()); On 2016/02/19 14:12:14, fdoray wrote: > On 2016/02/19 02:33:43, robliao wrote: > > I'm thinking this should be a CHECK. It is undefined what happens when you pop > > an empty queue (so no exception may be thrown). Ideally, it would crash. > > > > We don't rely on this behavior, do we? > > Done. > > https://www.chromium.org/developers/coding-style says "Use CHECK() if the > consequence of a failed assertion would be a security vulnerability". It is an > error to call PopTask on an empty sequence. If it happens, something is wrong > and I guess it is better to crash than to have an undefined behavior. I don't think this should be a CHECK. It's enforcing the method's contract and it would only fire if the algorithm is wrong which is okay to only verify in debug builds IMO (i.e. it's not dynamic based on user input or anything which is what the CHECK comment you linked to is trying to articulate I think). For example, Lock only verifies correct usage in DCHECK_IS_ON() builds despite many things resulting in undefined behaviour (locking twice, releasing twice, deleting an owned lock). https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/se... File base/task_scheduler/sequence_sort_key.cc (right): https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/se... base/task_scheduler/sequence_sort_key.cc:22: return priority_diff != 0 On 2016/02/19 14:12:14, fdoray wrote: > On 2016/02/19 02:33:43, robliao wrote: > > Remove the double negative by changing this to == and inverting the two > branches > > below. > > Done. I typically prefer equality check first but here prefer != as it allows the rules to be in their order of importance and reads better (i.e. "if priority isn't the same, base result on priority, otherwise on time" instead of "if priority is the same, use time, otherwise do use priority"). https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/se... File base/task_scheduler/sequence_sort_key.h (right): https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/se... base/task_scheduler/sequence_sort_key.h:22: bool operator<(const SequenceSortKey& other) const; On 2016/02/19 14:12:14, fdoray wrote: > On 2016/02/19 02:33:43, robliao wrote: > > Do we need to support operator reflexivity? > > a < b is the same as b > a > > We need operator< to sort sequences in PriorityQueue. I didn't implement > operator> because we don't need it. Is it OK? Good catch Rob. The style @ https://google.github.io/styleguide/cppguide.html#Operator_Overloading says "If you define an operator, also define any related operators that make sense, and make sure they are defined consistently. For example, if you overload <, overload all the comparison operators, and make sure < and > never return true for the same arguments." An alternative approach would be to give a custom comparator (e.g. a Compare() method defined on this class) to priority_queue and not even worry about operator overloading? I think I prefer that alternate option, WDYT? https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/ta... File base/task_scheduler/task_traits.h (right): https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/ta... base/task_scheduler/task_traits.h:131: void BASE_EXPORT PrintTo(const TaskPriority& task_priority, std::ostream* os); On 2016/02/19 14:12:15, fdoray wrote: > On 2016/02/19 02:33:43, robliao wrote: > > Can this go in a test only utils file? > > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/test/gfx_ut... > > is an example of this. > > It was in a test only file, but gab asked me to move it here to match > string16.h. A benefit of having this here is that we don't have to think about > including the test-only file to get pretty-printing. What should I do now? Keep it here is my vote. string16 does it, it won't affect non-test binary size as it won't be linked in, and we can just use TaskPriority comparison checks without thinking about an extra include. IMO this goes alongside the type. The only other option IMO is to define it in the test cc that needs it if there is only one (and even then I still prefer this), but a separate header I say no. https://codereview.chromium.org/1705253002/diff/160001/base/task_scheduler/se... File base/task_scheduler/sequence.cc (right): https://codereview.chromium.org/1705253002/diff/160001/base/task_scheduler/se... base/task_scheduler/sequence.cc:19: const TaskPriorityUnderlyingType index_priority = s/index_priority/priority_index/ ? (reads better to me..) https://codereview.chromium.org/1705253002/diff/160001/base/task_scheduler/se... base/task_scheduler/sequence.cc:58: ""); "Unexpected priority range." or something? Well actually, do we really need to static_assert that this is equal to its exact definition? They're both in internal namespace, you'd sure hope anyone modifying its definition would look for its very few use cases... https://codereview.chromium.org/1705253002/diff/160001/base/task_scheduler/se... base/task_scheduler/sequence.cc:60: for (size_t i = s/size_t/TaskPriorityUnderlyingType/ now that we're starting with one and comparing against one. https://codereview.chromium.org/1705253002/diff/160001/base/task_scheduler/se... File base/task_scheduler/sequence_sort_key.h (right): https://codereview.chromium.org/1705253002/diff/160001/base/task_scheduler/se... base/task_scheduler/sequence_sort_key.h:28: const TimeTicks next_task_sequenced_time_; Style dictates that members in struct aren't suffixed with _. https://codereview.chromium.org/1705253002/diff/160001/base/task_scheduler/se... File base/task_scheduler/sequence_sort_key_unittest.cc (right): https://codereview.chromium.org/1705253002/diff/160001/base/task_scheduler/se... base/task_scheduler/sequence_sort_key_unittest.cc:86: EXPECT_FALSE(key_a == key_e); EXPECT_NE instead of EXPECT_FALSE https://codereview.chromium.org/1705253002/diff/160001/base/task_scheduler/se... File base/task_scheduler/sequence_unittest.cc (right): https://codereview.chromium.org/1705253002/diff/160001/base/task_scheduler/se... base/task_scheduler/sequence_unittest.cc:122: const SequenceSortKey sort_key_9(sequence->GetSortKey()); I'm not against const for these, but feels like const is a bit overkill for an immutable type ;-), WDYT? https://codereview.chromium.org/1705253002/diff/160001/base/task_scheduler/se... base/task_scheduler/sequence_unittest.cc:124: EXPECT_EQ(task_a_.sequenced_time, sort_key_9.next_task_sequenced_time_); Instead of using different variable name (and risking a typo verifying the wrong thing). WDYT of either: 1) Scoping these checks in inlined {} or 2) Having a helper (define right above this test's body) void VerifySortKey(const Sequence& sequence, TaskPriority expected_priority, TimeTicks expected_time) { SequenceSortKey sort_key(sequence.GetSortKey()); EXPECT_EQ(expected_priority, sort_key.priority_); EXPECT_EQ(expected_time, sort_key.next_task_sequenced_time_); } I think I prefer (2) and it may even make this test case more readable. https://codereview.chromium.org/1705253002/diff/160001/base/task_scheduler/ta... File base/task_scheduler/task.h (right): https://codereview.chromium.org/1705253002/diff/160001/base/task_scheduler/ta... base/task_scheduler/task.h:33: // in a sequence yet, the value is a default TimeTicks. s/the value is a default TimeTicks/this defaults to a null TimeTicks/ ?
https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/se... File base/task_scheduler/sequence_sort_key.h (right): https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/se... base/task_scheduler/sequence_sort_key.h:22: bool operator<(const SequenceSortKey& other) const; On 2016/02/19 16:50:47, gab wrote: > On 2016/02/19 14:12:14, fdoray wrote: > > On 2016/02/19 02:33:43, robliao wrote: > > > Do we need to support operator reflexivity? > > > a < b is the same as b > a > > > > We need operator< to sort sequences in PriorityQueue. I didn't implement > > operator> because we don't need it. Is it OK? > > Good catch Rob. > > The style @ > https://google.github.io/styleguide/cppguide.html#Operator_Overloading says "If > you define an operator, also define any related operators that make sense, and > make sure they are defined consistently. For example, if you overload <, > overload all the comparison operators, and make sure < and > never return true > for the same arguments." > > An alternative approach would be to give a custom comparator (e.g. a Compare() > method defined on this class) to priority_queue and not even worry about > operator overloading? > I think I prefer that alternate option, WDYT? Note: it may also make sense to remove comparison altogether in this CL and only bring it in the PQ CL when there is context for it? Up to you.
https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/se... File base/task_scheduler/sequence.cc (right): https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/se... base/task_scheduler/sequence.cc:38: DCHECK(!queue_.empty()); On 2016/02/19 16:50:47, gab wrote: > On 2016/02/19 14:12:14, fdoray wrote: > > On 2016/02/19 02:33:43, robliao wrote: > > > I'm thinking this should be a CHECK. It is undefined what happens when you > pop > > > an empty queue (so no exception may be thrown). Ideally, it would crash. > > > > > > We don't rely on this behavior, do we? > > > > Done. > > > > https://www.chromium.org/developers/coding-style says "Use CHECK() if the > > consequence of a failed assertion would be a security vulnerability". It is an > > error to call PopTask on an empty sequence. If it happens, something is wrong > > and I guess it is better to crash than to have an undefined behavior. > > I don't think this should be a CHECK. > > It's enforcing the method's contract and it would only fire if the algorithm is > wrong which is okay to only verify in debug builds IMO (i.e. it's not dynamic > based on user input or anything which is what the CHECK comment you linked to is > trying to articulate I think). > > For example, Lock only verifies correct usage in DCHECK_IS_ON() builds despite > many things resulting in undefined behaviour (locking twice, releasing twice, > deleting an owned lock). Done. I do prefer DCHECK, but I agreed to change it to CHECK because not crashing leads to an undefined behavior, which can be dangerous. For the usual check that a pointer is not null before dereferencing, there is no undefined behavior: we know that the process will crash. It is still unclear to me when CHECK is appropriate. Clearly, for user input, it is better to show an error than to provoke a crash with CHECK. https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/se... File base/task_scheduler/sequence_sort_key.cc (right): https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/se... base/task_scheduler/sequence_sort_key.cc:22: return priority_diff != 0 On 2016/02/19 16:50:47, gab wrote: > On 2016/02/19 14:12:14, fdoray wrote: > > On 2016/02/19 02:33:43, robliao wrote: > > > Remove the double negative by changing this to == and inverting the two > > branches > > > below. > > > > Done. > > I typically prefer equality check first but here prefer != as it allows the > rules to be in their order of importance and reads better (i.e. "if priority > isn't the same, base result on priority, otherwise on time" instead of "if > priority is the same, use time, otherwise do use priority"). Done. https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/se... File base/task_scheduler/sequence_sort_key.h (right): https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/se... base/task_scheduler/sequence_sort_key.h:22: bool operator<(const SequenceSortKey& other) const; On 2016/02/19 16:52:56, gab wrote: > On 2016/02/19 16:50:47, gab wrote: > > On 2016/02/19 14:12:14, fdoray wrote: > > > On 2016/02/19 02:33:43, robliao wrote: > > > > Do we need to support operator reflexivity? > > > > a < b is the same as b > a > > > > > > We need operator< to sort sequences in PriorityQueue. I didn't implement > > > operator> because we don't need it. Is it OK? > > > > Good catch Rob. > > > > The style @ > > https://google.github.io/styleguide/cppguide.html#Operator_Overloading says > "If > > you define an operator, also define any related operators that make sense, and > > make sure they are defined consistently. For example, if you overload <, > > overload all the comparison operators, and make sure < and > never return true > > for the same arguments." > > > > An alternative approach would be to give a custom comparator (e.g. a Compare() > > method defined on this class) to priority_queue and not even worry about > > operator overloading? > > I think I prefer that alternate option, WDYT? > > Note: it may also make sense to remove comparison altogether in this CL and only > bring it in the PQ CL when there is context for it? Up to you. As we discussed, the comparison is required outside of the PQ: we need it to compare the sequences extracted from a single-threaded and a shared PQ. https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/ta... File base/task_scheduler/task_traits.h (right): https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/ta... base/task_scheduler/task_traits.h:131: void BASE_EXPORT PrintTo(const TaskPriority& task_priority, std::ostream* os); On 2016/02/19 16:50:47, gab wrote: > On 2016/02/19 14:12:15, fdoray wrote: > > On 2016/02/19 02:33:43, robliao wrote: > > > Can this go in a test only utils file? > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/test/gfx_ut... > > > is an example of this. > > > > It was in a test only file, but gab asked me to move it here to match > > string16.h. A benefit of having this here is that we don't have to think about > > including the test-only file to get pretty-printing. What should I do now? > > Keep it here is my vote. string16 does it, it won't affect non-test binary size > as it won't be linked in, and we can just use TaskPriority comparison checks > without thinking about an extra include. IMO this goes alongside the type. > > The only other option IMO is to define it in the test cc that needs it if there > is only one (and even then I still prefer this), but a separate header I say no. ok, keeping the declaration here. https://codereview.chromium.org/1705253002/diff/160001/base/task_scheduler/se... File base/task_scheduler/sequence.cc (right): https://codereview.chromium.org/1705253002/diff/160001/base/task_scheduler/se... base/task_scheduler/sequence.cc:19: const TaskPriorityUnderlyingType index_priority = On 2016/02/19 16:50:47, gab wrote: > s/index_priority/priority_index/ ? (reads better to me..) Done. https://codereview.chromium.org/1705253002/diff/160001/base/task_scheduler/se... base/task_scheduler/sequence.cc:58: ""); On 2016/02/19 16:50:47, gab wrote: > "Unexpected priority range." or something? > > Well actually, do we really need to static_assert that this is equal to its > exact definition? > > They're both in internal namespace, you'd sure hope anyone modifying its > definition would look for its very few use cases... Removed the static_assert. https://codereview.chromium.org/1705253002/diff/160001/base/task_scheduler/se... base/task_scheduler/sequence.cc:60: for (size_t i = On 2016/02/19 16:50:47, gab wrote: > s/size_t/TaskPriorityUnderlyingType/ now that we're starting with one and > comparing against one. Done. https://codereview.chromium.org/1705253002/diff/160001/base/task_scheduler/se... File base/task_scheduler/sequence_sort_key.h (right): https://codereview.chromium.org/1705253002/diff/160001/base/task_scheduler/se... base/task_scheduler/sequence_sort_key.h:28: const TimeTicks next_task_sequenced_time_; On 2016/02/19 16:50:47, gab wrote: > Style dictates that members in struct aren't suffixed with _. Done. https://codereview.chromium.org/1705253002/diff/160001/base/task_scheduler/se... File base/task_scheduler/sequence_sort_key_unittest.cc (right): https://codereview.chromium.org/1705253002/diff/160001/base/task_scheduler/se... base/task_scheduler/sequence_sort_key_unittest.cc:86: EXPECT_FALSE(key_a == key_e); On 2016/02/19 16:50:47, gab wrote: > EXPECT_NE instead of EXPECT_FALSE As we discussed, I'm testing the operator here. https://codereview.chromium.org/1705253002/diff/160001/base/task_scheduler/se... File base/task_scheduler/sequence_unittest.cc (right): https://codereview.chromium.org/1705253002/diff/160001/base/task_scheduler/se... base/task_scheduler/sequence_unittest.cc:122: const SequenceSortKey sort_key_9(sequence->GetSortKey()); On 2016/02/19 16:50:47, gab wrote: > I'm not against const for these, but feels like const is a bit overkill for an > immutable type ;-), WDYT? Done. https://codereview.chromium.org/1705253002/diff/160001/base/task_scheduler/se... base/task_scheduler/sequence_unittest.cc:124: EXPECT_EQ(task_a_.sequenced_time, sort_key_9.next_task_sequenced_time_); On 2016/02/19 16:50:47, gab wrote: > Instead of using different variable name (and risking a typo verifying the wrong > thing). WDYT of either: > > 1) Scoping these checks in inlined {} > or > 2) Having a helper (define right above this test's body) > void VerifySortKey(const Sequence& sequence, TaskPriority expected_priority, > TimeTicks expected_time) { > SequenceSortKey sort_key(sequence.GetSortKey()); > EXPECT_EQ(expected_priority, sort_key.priority_); > EXPECT_EQ(expected_time, sort_key.next_task_sequenced_time_); > } > > I think I prefer (2) and it may even make this test case more readable. Done. https://codereview.chromium.org/1705253002/diff/160001/base/task_scheduler/ta... File base/task_scheduler/task.h (right): https://codereview.chromium.org/1705253002/diff/160001/base/task_scheduler/ta... base/task_scheduler/task.h:33: // in a sequence yet, the value is a default TimeTicks. On 2016/02/19 16:50:47, gab wrote: > s/the value is a default TimeTicks/this defaults to a null TimeTicks/ ? Done.
lgtm % nits Thanks! Gab https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/se... File base/task_scheduler/sequence.cc (right): https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/se... base/task_scheduler/sequence.cc:38: DCHECK(!queue_.empty()); On 2016/02/19 22:28:29, fdoray wrote: > On 2016/02/19 16:50:47, gab wrote: > > On 2016/02/19 14:12:14, fdoray wrote: > > > On 2016/02/19 02:33:43, robliao wrote: > > > > I'm thinking this should be a CHECK. It is undefined what happens when you > > pop > > > > an empty queue (so no exception may be thrown). Ideally, it would crash. > > > > > > > > We don't rely on this behavior, do we? > > > > > > Done. > > > > > > https://www.chromium.org/developers/coding-style says "Use CHECK() if the > > > consequence of a failed assertion would be a security vulnerability". It is > an > > > error to call PopTask on an empty sequence. If it happens, something is > wrong > > > and I guess it is better to crash than to have an undefined behavior. > > > > I don't think this should be a CHECK. > > > > It's enforcing the method's contract and it would only fire if the algorithm > is > > wrong which is okay to only verify in debug builds IMO (i.e. it's not dynamic > > based on user input or anything which is what the CHECK comment you linked to > is > > trying to articulate I think). > > > > For example, Lock only verifies correct usage in DCHECK_IS_ON() builds despite > > many things resulting in undefined behaviour (locking twice, releasing twice, > > deleting an owned lock). > > Done. I do prefer DCHECK, but I agreed to change it to CHECK because not > crashing leads to an undefined behavior, which can be dangerous. For the usual > check that a pointer is not null before dereferencing, there is no undefined > behavior: we know that the process will crash. > > It is still unclear to me when CHECK is appropriate. Clearly, for user input, it > is better to show an error than to provoke a crash with CHECK. > Agreed that for identifiable user input errors a nice error is much prefered to a CHECK. Still I think CHECK doesn't belong anywhere in this file. This API is internal to the task scheduler, and DCHECKs are sufficient to verify that the code uses it correctly and then be confident that it can't be triggered the wrong way in Release builds. I guess CHECK applies when you can't be confident that the lack of DCHECK hits in a debug build implies that the situation will never occur in a Release build (and is a security issue if it does). https://codereview.chromium.org/1705253002/diff/220001/base/task_scheduler/se... File base/task_scheduler/sequence_unittest.cc (right): https://codereview.chromium.org/1705253002/diff/220001/base/task_scheduler/se... base/task_scheduler/sequence_unittest.cc:44: scoped_ptr<Task> task_a_; // Tasks to handed off to a Sequence for testing. https://codereview.chromium.org/1705253002/diff/220001/base/task_scheduler/se... base/task_scheduler/sequence_unittest.cc:50: const Task* task_a_raw_; // Raw pointers to those same tasks for verification. https://codereview.chromium.org/1705253002/diff/220001/base/task_scheduler/se... base/task_scheduler/sequence_unittest.cc:54: const Task* task_e_raw_; Why not just inline .get()? ".get()" almost the same # chars as "_raw_".. and saves 6 member vars. Much preferred to me.
https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/se... File base/task_scheduler/sequence.cc (right): https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/se... base/task_scheduler/sequence.cc:38: DCHECK(!queue_.empty()); On 2016/02/22 17:12:21, gab wrote: > On 2016/02/19 22:28:29, fdoray wrote: > > On 2016/02/19 16:50:47, gab wrote: > > > On 2016/02/19 14:12:14, fdoray wrote: > > > > On 2016/02/19 02:33:43, robliao wrote: > > > > > I'm thinking this should be a CHECK. It is undefined what happens when > you > > > pop > > > > > an empty queue (so no exception may be thrown). Ideally, it would crash. > > > > > > > > > > > We don't rely on this behavior, do we? > > > > > > > > Done. > > > > > > > > https://www.chromium.org/developers/coding-style says "Use CHECK() if the > > > > consequence of a failed assertion would be a security vulnerability". It > is > > an > > > > error to call PopTask on an empty sequence. If it happens, something is > > wrong > > > > and I guess it is better to crash than to have an undefined behavior. > > > > > > I don't think this should be a CHECK. > > > > > > It's enforcing the method's contract and it would only fire if the algorithm > > is > > > wrong which is okay to only verify in debug builds IMO (i.e. it's not > dynamic > > > based on user input or anything which is what the CHECK comment you linked > to > > is > > > trying to articulate I think). > > > > > > For example, Lock only verifies correct usage in DCHECK_IS_ON() builds > despite > > > many things resulting in undefined behaviour (locking twice, releasing > twice, > > > deleting an owned lock). > > > > Done. I do prefer DCHECK, but I agreed to change it to CHECK because not > > crashing leads to an undefined behavior, which can be dangerous. For the usual > > check that a pointer is not null before dereferencing, there is no undefined > > behavior: we know that the process will crash. > > > > It is still unclear to me when CHECK is appropriate. Clearly, for user input, > it > > is better to show an error than to provoke a crash with CHECK. > > > Agreed that for identifiable user input errors a nice error is much prefered to > a CHECK. > > Still I think CHECK doesn't belong anywhere in this file. This API is internal > to the task scheduler, and DCHECKs are sufficient to verify that the code uses > it correctly and then be confident that it can't be triggered the wrong way in > Release builds. > > I guess CHECK applies when you can't be confident that the lack of DCHECK hits > in a debug build implies that the situation will never occur in a Release build > (and is a security issue if it does). Ack. https://codereview.chromium.org/1705253002/diff/220001/base/task_scheduler/se... File base/task_scheduler/sequence_unittest.cc (right): https://codereview.chromium.org/1705253002/diff/220001/base/task_scheduler/se... base/task_scheduler/sequence_unittest.cc:44: scoped_ptr<Task> task_a_; On 2016/02/22 17:12:21, gab wrote: > // Tasks to handed off to a Sequence for testing. Done. https://codereview.chromium.org/1705253002/diff/220001/base/task_scheduler/se... base/task_scheduler/sequence_unittest.cc:50: const Task* task_a_raw_; On 2016/02/22 17:12:21, gab wrote: > // Raw pointers to those same tasks for verification. Done. https://codereview.chromium.org/1705253002/diff/220001/base/task_scheduler/se... base/task_scheduler/sequence_unittest.cc:54: const Task* task_e_raw_; On 2016/02/22 17:12:21, gab wrote: > Why not just inline .get()? ".get()" almost the same # chars as "_raw_".. and > saves 6 member vars. Much preferred to me. Once |task_x_| has been moved in a sequence, task_x_.get() returns nullptr. I added a comment to explain that.
lgtm++ go ahead and submit to jam for OWNERS as it doesn't actually depend on 2/8 in practice. https://codereview.chromium.org/1705253002/diff/220001/base/task_scheduler/se... File base/task_scheduler/sequence_unittest.cc (right): https://codereview.chromium.org/1705253002/diff/220001/base/task_scheduler/se... base/task_scheduler/sequence_unittest.cc:54: const Task* task_e_raw_; On 2016/02/22 18:07:55, fdoray wrote: > On 2016/02/22 17:12:21, gab wrote: > > Why not just inline .get()? ".get()" almost the same # chars as "_raw_".. and > > saves 6 member vars. Much preferred to me. > > Once |task_x_| has been moved in a sequence, task_x_.get() returns nullptr. I > added a comment to explain that. Oops, forgot to discard that comment after I read further and realized exactly that (and then made the above two suggestions to clarify this for the reader here instead of later) :-).
fdoray@chromium.org changed reviewers: + jam@chromium.org
jam@: Can you review this CL? Thanks. Note: Sequence uses SchedulerLock, introduced by another CL (https://codereview.chromium.org/1706123002) which we will ask you to review soon.
lgtm + nits https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/test_ut... File base/task_scheduler/test_util.cc (right): https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/test_ut... base/task_scheduler/test_util.cc:24: default: On 2016/02/18 14:56:13, fdoray wrote: > On 2016/02/18 03:00:46, gab wrote: > > No default, without the default clang compile will break if someone adds a new > > TaskPriority without adjusting this :-). > > > > In general always avoid default in switches when you can. > > Done. https://google.github.io/styleguide/cppguide.html#Loops_and_Switch_Statements If not conditional on an enumerated value, switch statements should always have a default case (in the case of an enumerated value, the compiler will warn you if any values are not handled). Given that this is an enum, we're good to go here. https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/se... File base/task_scheduler/sequence_sort_key.h (right): https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/se... base/task_scheduler/sequence_sort_key.h:27: const TaskPriority priority_; On 2016/02/19 14:12:14, fdoray wrote: > On 2016/02/19 02:33:43, robliao wrote: > > Will this immutability cause problems in STL containers? (e.g., we can never > > have a vector of SequenceSortKeys). > > You're right, we won't be able to create STL containers of SequenceSortKeys. > PriorityQueue will have an std::priority_queue<SequenceAndSortKeyPair*> instead > of an std::priority_queue<SequenceAndSortKeyPair>. See gab's comment here > https://codereview.chromium.org/1709713002/diff/1/base/task_scheduler/priorit... Acknowledged. https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/ta... File base/task_scheduler/task.h (right): https://codereview.chromium.org/1705253002/diff/140001/base/task_scheduler/ta... base/task_scheduler/task.h:33: TimeTicks sequenced_time; On 2016/02/19 14:12:15, fdoray wrote: > On 2016/02/19 02:33:43, robliao wrote: > > This should probably be const. I suspect that impacts STL container usage, so > > you can turn this into a class and make these private and add getters. > > It can't be const because for a delayed task, it isn't set when the task is > constructed. > > This is currently set by TaskRunners/DelayedTaskManager before pushing the task > in a sequence, but it should probably be set by Sequence::PushTask instead. > WDYT? sgtm. https://codereview.chromium.org/1705253002/diff/340001/base/task_scheduler/se... File base/task_scheduler/sequence.cc (right): https://codereview.chromium.org/1705253002/diff/340001/base/task_scheduler/se... base/task_scheduler/sequence.cc:59: // Find the highest task priority in the sequence. We should add a static_assert here static_assert(TaskPriority::HIGHEST == kNumTaskPriorities - 1)
https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/test_ut... File base/task_scheduler/test_util.cc (right): https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/test_ut... base/task_scheduler/test_util.cc:24: default: On 2016/02/22 21:34:12, robliao wrote: > On 2016/02/18 14:56:13, fdoray wrote: > > On 2016/02/18 03:00:46, gab wrote: > > > No default, without the default clang compile will break if someone adds a > new > > > TaskPriority without adjusting this :-). > > > > > > In general always avoid default in switches when you can. > > > > Done. > > https://google.github.io/styleguide/cppguide.html#Loops_and_Switch_Statements > > If not conditional on an enumerated value, switch statements should always have > a default case (in the case of an enumerated value, the compiler will warn you > if any values are not handled). > > Given that this is an enum, we're good to go here. Right, are we saying the same thing? I'm confused by your reply on this? https://codereview.chromium.org/1705253002/diff/340001/base/task_scheduler/se... File base/task_scheduler/sequence.cc (right): https://codereview.chromium.org/1705253002/diff/340001/base/task_scheduler/se... base/task_scheduler/sequence.cc:59: // Find the highest task priority in the sequence. On 2016/02/22 21:34:12, robliao wrote: > We should add a static_assert here > static_assert(TaskPriority::HIGHEST == kNumTaskPriorities - 1) There was one, but removed per my request. Ref: https://codereview.chromium.org/1705253002/diff/160001/base/task_scheduler/se...
https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/test_ut... File base/task_scheduler/test_util.cc (right): https://codereview.chromium.org/1705253002/diff/1/base/task_scheduler/test_ut... base/task_scheduler/test_util.cc:24: default: On 2016/02/22 21:40:24, gab wrote: > On 2016/02/22 21:34:12, robliao wrote: > > On 2016/02/18 14:56:13, fdoray wrote: > > > On 2016/02/18 03:00:46, gab wrote: > > > > No default, without the default clang compile will break if someone adds a > > new > > > > TaskPriority without adjusting this :-). > > > > > > > > In general always avoid default in switches when you can. > > > > > > Done. > > > > https://google.github.io/styleguide/cppguide.html#Loops_and_Switch_Statements > > > > If not conditional on an enumerated value, switch statements should always > have > > a default case (in the case of an enumerated value, the compiler will warn you > > if any values are not handled). > > > > Given that this is an enum, we're good to go here. > > Right, are we saying the same thing? I'm confused by your reply on this? Yup. We're saying the same thing. No default required for this case. https://codereview.chromium.org/1705253002/diff/340001/base/task_scheduler/se... File base/task_scheduler/sequence.cc (right): https://codereview.chromium.org/1705253002/diff/340001/base/task_scheduler/se... base/task_scheduler/sequence.cc:59: // Find the highest task priority in the sequence. On 2016/02/22 21:40:24, gab wrote: > On 2016/02/22 21:34:12, robliao wrote: > > We should add a static_assert here > > static_assert(TaskPriority::HIGHEST == kNumTaskPriorities - 1) > > There was one, but removed per my request. Ref: > https://codereview.chromium.org/1705253002/diff/160001/base/task_scheduler/se... Ah, missed that. The reason why a static_assert would be appropriate is that SequenceSortKey takes advantage of an implementation detail of how TaskPriority is defined. This file depends on that definition, and so a static_assert would be helpful in pointing that out.
On 2016/02/22 20:29:04, fdoray wrote: > jam@: Can you review this CL? Thanks. > > Note: Sequence uses SchedulerLock, introduced by another CL > (https://codereview.chromium.org/1706123002) which we will ask you to review > soon. I'm not the best reviewer for this; can you find someone who's worked a lot with messageloop/messagepump implementations?
gab@chromium.org changed reviewers: + thakis@chromium.org
On 2016/02/22 23:34:34, jam wrote: > On 2016/02/22 20:29:04, fdoray wrote: > > jam@: Can you review this CL? Thanks. > > > > Note: Sequence uses SchedulerLock, introduced by another CL > > (https://codereview.chromium.org/1706123002) which we will ask you to review > > soon. > > I'm not the best reviewer for this; can you find someone who's worked a lot with > messageloop/messagepump implementations? +Nico who has stepped up to be our base owner (thanks Nico!)
gab@chromium.org changed reviewers: + danakj@chromium.org
On 2016/02/23 01:17:57, gab wrote: > On 2016/02/22 23:34:34, jam wrote: > > On 2016/02/22 20:29:04, fdoray wrote: > > > jam@: Can you review this CL? Thanks. > > > > > > Note: Sequence uses SchedulerLock, introduced by another CL > > > (https://codereview.chromium.org/1706123002) which we will ask you to review > > > soon. > > > > I'm not the best reviewer for this; can you find someone who's worked a lot > with > > messageloop/messagepump implementations? > > +Nico who has stepped up to be our base owner (thanks Nico!) +Dana who's highly versed in all the schedulery things and may also be interested in reviewing base/task_scheduler/.
https://codereview.chromium.org/1705253002/diff/340001/base/task_scheduler/se... File base/task_scheduler/sequence.cc (right): https://codereview.chromium.org/1705253002/diff/340001/base/task_scheduler/se... base/task_scheduler/sequence.cc:59: // Find the highest task priority in the sequence. On 2016/02/22 21:45:48, robliao wrote: > On 2016/02/22 21:40:24, gab wrote: > > On 2016/02/22 21:34:12, robliao wrote: > > > We should add a static_assert here > > > static_assert(TaskPriority::HIGHEST == kNumTaskPriorities - 1) > > > > There was one, but removed per my request. Ref: > > > https://codereview.chromium.org/1705253002/diff/160001/base/task_scheduler/se... > > Ah, missed that. The reason why a static_assert would be appropriate is that > SequenceSortKey takes advantage of an implementation detail of how TaskPriority > is defined. This file depends on that definition, and so a static_assert would > be helpful in pointing that out. Done. static_assert is back as we discussed yesterday.
Thanks, @Dana
Description was changed from ========== TaskScheduler [3/8] Task and Sequence This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ========== to ========== TaskScheduler [3/9] Task and Sequence This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ==========
Dana: This is the next CL to review for the new task scheduler.
Can you add more to the CL description to say what this is doing on its own? Less clicking and trying to understand for sheriffs and bisectors and the like.
https://codereview.chromium.org/1705253002/diff/360001/base/task_scheduler/se... File base/task_scheduler/sequence.cc (right): https://codereview.chromium.org/1705253002/diff/360001/base/task_scheduler/se... base/task_scheduler/sequence.cc:24: static_cast<TaskPriorityUnderlyingType>(task->traits.priority()); Can you put code that doesn't access members above the auto_lock? https://codereview.chromium.org/1705253002/diff/360001/base/task_scheduler/se... base/task_scheduler/sequence.cc:49: --num_tasks_per_priority_[priority_index]; maybe DCHECK that num_tasks_per_priority > 0 before this? https://codereview.chromium.org/1705253002/diff/360001/base/task_scheduler/se... base/task_scheduler/sequence.cc:61: TaskPriority::HIGHEST) == internal::kNumTaskPriorities - 1, Do you even needs kNumTaskPriorities? Why not just use HIGHEST (or HIGHEST+1) instead? https://codereview.chromium.org/1705253002/diff/360001/base/task_scheduler/se... base/task_scheduler/sequence.cc:66: static_cast<TaskPriorityUnderlyingType>(TaskPriority::HIGHEST); This would probably be a lot easier to read if you made some temp vars for highest and lowest outside the loop and used those here instead of static_casting inside the for declaration. https://codereview.chromium.org/1705253002/diff/360001/base/task_scheduler/se... base/task_scheduler/sequence.cc:74: return SequenceSortKey(priority, queue_.front()->sequenced_time); this is a bit pendantic, but you could put the sequenced time into a local variable, then construct and return the SequenceSortKey after releasing the lock_. https://codereview.chromium.org/1705253002/diff/360001/base/task_scheduler/se... File base/task_scheduler/sequence_sort_key.cc (right): https://codereview.chromium.org/1705253002/diff/360001/base/task_scheduler/se... base/task_scheduler/sequence_sort_key.cc:21: return priority_diff != 0 not a big fan of multiline ternary if statements. if (priority_diff < 0) return true; if (priority_diff > 0) return false; return time > othertime; ^ Would be easier to read, and maybe you wouldn't even need explanatory comments, up to you. Or if you're concerned about # of ops.. if (priority_diff == 0) return time > othertime; return priorify_diff < 0; Idk if those compile to anything different but yeh. https://codereview.chromium.org/1705253002/diff/360001/base/task_scheduler/se... base/task_scheduler/sequence_sort_key.cc:27: const int priority_diff = Can this not be implemented in terms of operator<? return other < *this; You could inline that in the header too. https://codereview.chromium.org/1705253002/diff/360001/base/task_scheduler/se... File base/task_scheduler/sequence_unittest.cc (right): https://codereview.chromium.org/1705253002/diff/360001/base/task_scheduler/se... base/task_scheduler/sequence_unittest.cc:54: const Task* task_a_raw_; fwiw, in this type of pattern, i usually name the scoped_ptr task_a_owned_ and the raw pointer as task_a_, so the more commonly written one is shorter. this is fine too. https://codereview.chromium.org/1705253002/diff/360001/base/task_scheduler/se... base/task_scheduler/sequence_unittest.cc:131: sequence->PopTask(); This test would benefit from some comments along the way, like here: // Pop task_a from the sequence, the highest priority is still USER_BLOCKING. More comments the better in tests. https://codereview.chromium.org/1705253002/diff/360001/base/task_scheduler/se... base/task_scheduler/sequence_unittest.cc:147: sequence->PopTask(); // Pop task_d from the sequence, the highest priority is now from task_e. https://codereview.chromium.org/1705253002/diff/360001/base/task_scheduler/ta... File base/task_scheduler/task.cc (right): https://codereview.chromium.org/1705253002/diff/360001/base/task_scheduler/ta... base/task_scheduler/task.cc:13: : PendingTask(posted_from, task, TimeTicks(), false), traits(traits) {} Can you leave a comment what the TimeTicks() and false (and maybe why false) are here? https://codereview.chromium.org/1705253002/diff/360001/base/task_scheduler/ta... File base/task_scheduler/task_traits.h (right): https://codereview.chromium.org/1705253002/diff/360001/base/task_scheduler/ta... base/task_scheduler/task_traits.h:25: enum class TaskPriority : internal::TaskPriorityUnderlyingType { Is there a reason why you need to define the UnderlyingType and use that explicitly, instead of just static_cast to "int" when you want to use it that way? https://codereview.chromium.org/1705253002/diff/360001/base/task_scheduler/ta... base/task_scheduler/task_traits.h:46: static_cast<TaskPriorityUnderlyingType>(TaskPriority::HIGHEST) + 1; use an enum to put a static const in a header file until we get constexptr (to avoid weird storage concerns - see chromium-dev about it, or chromium style guide)
Description was changed from ========== TaskScheduler [3/9] Task and Sequence This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ========== to ========== TaskScheduler [3/9] Task and Sequence This change is a subset of https://codereview.chromium.org/1698183005/ A Task is a unit of work in the task scheduler. It has a closure, a sequenced time, TaskTraits and other metadata inherited from base::PendingTask. A Sequence holds Tasks that must run in order. It is ref-counted and has thread-safe Push, Pop and Peek operations. Priority queues, worker threads and task runners will have references on Sequences. BUG=553459 ==========
danakj@: All done. Can you take another look? Thanks. https://codereview.chromium.org/1705253002/diff/360001/base/task_scheduler/se... File base/task_scheduler/sequence.cc (right): https://codereview.chromium.org/1705253002/diff/360001/base/task_scheduler/se... base/task_scheduler/sequence.cc:24: static_cast<TaskPriorityUnderlyingType>(task->traits.priority()); On 2016/03/15 23:16:31, danakj wrote: > Can you put code that doesn't access members above the auto_lock? |priority_index| was there to make the code simpler. Now that I cast to int instead of TaskPriorityUnderlyingType, everything fits on 1 line and I don't think that |priority_index| is required anymore. WDYT? https://codereview.chromium.org/1705253002/diff/360001/base/task_scheduler/se... base/task_scheduler/sequence.cc:49: --num_tasks_per_priority_[priority_index]; On 2016/03/15 23:16:31, danakj wrote: > maybe DCHECK that num_tasks_per_priority > 0 before this? Done. https://codereview.chromium.org/1705253002/diff/360001/base/task_scheduler/se... base/task_scheduler/sequence.cc:61: TaskPriority::HIGHEST) == internal::kNumTaskPriorities - 1, On 2016/03/15 23:16:32, danakj wrote: > Do you even needs kNumTaskPriorities? Why not just use HIGHEST (or HIGHEST+1) > instead? Done. kNumTaskPriorities is not needed. https://codereview.chromium.org/1705253002/diff/360001/base/task_scheduler/se... base/task_scheduler/sequence.cc:66: static_cast<TaskPriorityUnderlyingType>(TaskPriority::HIGHEST); On 2016/03/15 23:16:31, danakj wrote: > This would probably be a lot easier to read if you made some temp vars for > highest and lowest outside the loop and used those here instead of > static_casting inside the for declaration. Done. https://codereview.chromium.org/1705253002/diff/360001/base/task_scheduler/se... base/task_scheduler/sequence.cc:74: return SequenceSortKey(priority, queue_.front()->sequenced_time); On 2016/03/15 23:16:31, danakj wrote: > this is a bit pendantic, but you could put the sequenced time into a local > variable, then construct and return the SequenceSortKey after releasing the > lock_. Done. https://codereview.chromium.org/1705253002/diff/360001/base/task_scheduler/se... File base/task_scheduler/sequence_sort_key.cc (right): https://codereview.chromium.org/1705253002/diff/360001/base/task_scheduler/se... base/task_scheduler/sequence_sort_key.cc:21: return priority_diff != 0 On 2016/03/15 23:16:32, danakj wrote: > not a big fan of multiline ternary if statements. > > if (priority_diff < 0) return true; > if (priority_diff > 0) return false; > return time > othertime; > > ^ Would be easier to read, and maybe you wouldn't even need explanatory > comments, up to you. > > Or if you're concerned about # of ops.. > > if (priority_diff == 0) > return time > othertime; > return priorify_diff < 0; > > Idk if those compile to anything different but yeh. Done. I think the comment is still useful to explain why we use operator> to implement an operator< (line 24). https://codereview.chromium.org/1705253002/diff/360001/base/task_scheduler/se... base/task_scheduler/sequence_sort_key.cc:27: const int priority_diff = On 2016/03/15 23:16:32, danakj wrote: > Can this not be implemented in terms of operator<? > > return other < *this; > > You could inline that in the header too. Done. https://codereview.chromium.org/1705253002/diff/360001/base/task_scheduler/se... File base/task_scheduler/sequence_unittest.cc (right): https://codereview.chromium.org/1705253002/diff/360001/base/task_scheduler/se... base/task_scheduler/sequence_unittest.cc:54: const Task* task_a_raw_; On 2016/03/15 23:16:32, danakj wrote: > fwiw, in this type of pattern, i usually name the scoped_ptr task_a_owned_ and > the raw pointer as task_a_, so the more commonly written one is shorter. this is > fine too. Done. https://codereview.chromium.org/1705253002/diff/360001/base/task_scheduler/se... base/task_scheduler/sequence_unittest.cc:131: sequence->PopTask(); On 2016/03/15 23:16:32, danakj wrote: > This test would benefit from some comments along the way, like here: > > // Pop task_a from the sequence, the highest priority is still USER_BLOCKING. > > More comments the better in tests. Done. https://codereview.chromium.org/1705253002/diff/360001/base/task_scheduler/se... base/task_scheduler/sequence_unittest.cc:147: sequence->PopTask(); On 2016/03/15 23:16:32, danakj wrote: > // Pop task_d from the sequence, the highest priority is now from task_e. Done. https://codereview.chromium.org/1705253002/diff/360001/base/task_scheduler/ta... File base/task_scheduler/task.cc (right): https://codereview.chromium.org/1705253002/diff/360001/base/task_scheduler/ta... base/task_scheduler/task.cc:13: : PendingTask(posted_from, task, TimeTicks(), false), traits(traits) {} On 2016/03/15 23:16:32, danakj wrote: > Can you leave a comment what the TimeTicks() and false (and maybe why false) are > here? Done. https://codereview.chromium.org/1705253002/diff/360001/base/task_scheduler/ta... File base/task_scheduler/task_traits.h (right): https://codereview.chromium.org/1705253002/diff/360001/base/task_scheduler/ta... base/task_scheduler/task_traits.h:25: enum class TaskPriority : internal::TaskPriorityUnderlyingType { On 2016/03/15 23:16:32, danakj wrote: > Is there a reason why you need to define the UnderlyingType and use that > explicitly, instead of just static_cast to "int" when you want to use it that > way? static_cast to int is fine. https://codereview.chromium.org/1705253002/diff/360001/base/task_scheduler/ta... base/task_scheduler/task_traits.h:46: static_cast<TaskPriorityUnderlyingType>(TaskPriority::HIGHEST) + 1; On 2016/03/15 23:16:32, danakj wrote: > use an enum to put a static const in a header file until we get constexptr (to > avoid weird storage concerns - see chromium-dev about it, or chromium style > guide) I removed this constant. I use static_cast<int>(TaskPriority::HIGHEST) + 1 directly when needed now.
LGTM 1 more thing tho: https://codereview.chromium.org/1705253002/diff/380001/base/task_scheduler/se... File base/task_scheduler/sequence.cc (right): https://codereview.chromium.org/1705253002/diff/380001/base/task_scheduler/se... base/task_scheduler/sequence.cc:52: scoped_ptr<AutoSchedulerLock> auto_lock(new AutoSchedulerLock(lock_)); Rather than using a scoped_ptr which requires malloc/free (which are costly), you can just use nested braces to accomplish this
Thanks all, one last thing from me https://codereview.chromium.org/1705253002/diff/380001/base/task_scheduler/ta... File base/task_scheduler/task_traits.h (right): https://codereview.chromium.org/1705253002/diff/380001/base/task_scheduler/ta... base/task_scheduler/task_traits.h:79: class BASE_EXPORT TaskTraits { Since classes should typically have DISALLOW_COPY_AND_ASSIGN and this one is intentionally copyable I think it makes sense to highlight that this was an intentional decision by declaring the use of the default copy constructor below. TaskTraits(const TaskTraits& other) = default; And also declare: TaskTraits& operator=(const TaskTraits& other) = default; per the style rule that if one is defined the other one also should be: "If you define a copy or move constructor, define the corresponding assignment operator, and vice-versa. If your type is copyable" @ https://google.github.io/styleguide/cppguide.html#Copyable_Movable_Types
https://codereview.chromium.org/1705253002/diff/380001/base/task_scheduler/se... File base/task_scheduler/sequence.cc (right): https://codereview.chromium.org/1705253002/diff/380001/base/task_scheduler/se... base/task_scheduler/sequence.cc:52: scoped_ptr<AutoSchedulerLock> auto_lock(new AutoSchedulerLock(lock_)); On 2016/03/16 19:40:33, danakj wrote: > Rather than using a scoped_ptr which requires malloc/free (which are costly), > you can just use nested braces to accomplish this Done. https://codereview.chromium.org/1705253002/diff/380001/base/task_scheduler/ta... File base/task_scheduler/task_traits.h (right): https://codereview.chromium.org/1705253002/diff/380001/base/task_scheduler/ta... base/task_scheduler/task_traits.h:79: class BASE_EXPORT TaskTraits { On 2016/03/16 22:18:50, gab wrote: > Since classes should typically have DISALLOW_COPY_AND_ASSIGN and this one is > intentionally copyable I think it makes sense to highlight that this was an > intentional decision by declaring the use of the default copy constructor below. > > TaskTraits(const TaskTraits& other) = default; > > And also declare: > > TaskTraits& operator=(const TaskTraits& other) = default; > > per the style rule that if one is defined the other one also should be: > > "If you define a copy or move constructor, define the corresponding assignment > operator, and vice-versa. If your type is copyable" > @ > https://google.github.io/styleguide/cppguide.html#Copyable_Movable_Types Done.
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org, robliao@chromium.org, danakj@chromium.org Link to the patchset: https://codereview.chromium.org/1705253002/#ps400001 (title: "CR from Dana and Gab #40-41")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1705253002/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1705253002/400001
Message was sent while issue was closed.
Description was changed from ========== TaskScheduler [3/9] Task and Sequence This change is a subset of https://codereview.chromium.org/1698183005/ A Task is a unit of work in the task scheduler. It has a closure, a sequenced time, TaskTraits and other metadata inherited from base::PendingTask. A Sequence holds Tasks that must run in order. It is ref-counted and has thread-safe Push, Pop and Peek operations. Priority queues, worker threads and task runners will have references on Sequences. BUG=553459 ========== to ========== TaskScheduler [3/9] Task and Sequence This change is a subset of https://codereview.chromium.org/1698183005/ A Task is a unit of work in the task scheduler. It has a closure, a sequenced time, TaskTraits and other metadata inherited from base::PendingTask. A Sequence holds Tasks that must run in order. It is ref-counted and has thread-safe Push, Pop and Peek operations. Priority queues, worker threads and task runners will have references on Sequences. BUG=553459 ==========
Message was sent while issue was closed.
Committed patchset #21 (id:400001)
Message was sent while issue was closed.
Description was changed from ========== TaskScheduler [3/9] Task and Sequence This change is a subset of https://codereview.chromium.org/1698183005/ A Task is a unit of work in the task scheduler. It has a closure, a sequenced time, TaskTraits and other metadata inherited from base::PendingTask. A Sequence holds Tasks that must run in order. It is ref-counted and has thread-safe Push, Pop and Peek operations. Priority queues, worker threads and task runners will have references on Sequences. BUG=553459 ========== to ========== TaskScheduler [3/9] Task and Sequence This change is a subset of https://codereview.chromium.org/1698183005/ A Task is a unit of work in the task scheduler. It has a closure, a sequenced time, TaskTraits and other metadata inherited from base::PendingTask. A Sequence holds Tasks that must run in order. It is ref-counted and has thread-safe Push, Pop and Peek operations. Priority queues, worker threads and task runners will have references on Sequences. BUG=553459 Committed: https://crrev.com/c48d5f0936e192046e11f9f9f03a70570523fc94 Cr-Commit-Position: refs/heads/master@{#381638} ==========
Message was sent while issue was closed.
Patchset 21 (id:??) landed as https://crrev.com/c48d5f0936e192046e11f9f9f03a70570523fc94 Cr-Commit-Position: refs/heads/master@{#381638} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
