Reland: Explicitly track the scheduler task enqueueing order in a new field

Explicitly track the scheduler task enqueueing order in a new field

Task enqueueing_order for delayed tasks is not known until the task's delay has
expired.  Preciously we were storing this in sequence_num but that
broke FlowEvents because the computed IDs changed mid-flight.

In addition this patch no longer zeros out delayed_run_time
since that appears to be used by some tracing code.

BUG=515720
Committed: https://crrev.com/8c6b40b808787f00fef268e6c06eb02a0f8f690e
Cr-Commit-Position: refs/heads/master@{#342113}

Committed: https://crrev.com/879c9fb02f86ea29bec4ee45aa5e705948422dc1
Cr-Commit-Position: refs/heads/master@{#342166}

[alex clarke (OOO till 29th), 2015-08-04 17:00:03]

alexclarke@chromium.org changed reviewers:
+ skyostil@chromium.org

[picksi, 2015-08-05 09:09:10]

Drive by comment... is "age" the correct name for this? I understand (a little!)
that you are trying to differentiate if from sequence number but age might be
misleading, as older tasks will have lower 'ages'!

[Sami, 2015-08-05 11:27:34]

Thanks for the quick fix. I agree with Simon about "age" being a somewhat odd
concept since the priority is backwards but I don't have a better suggestion.
run_order? sort_order? Hmm...

https://codereview.chromium.org/1259583006/diff/20001/components/scheduler/ch...
File components/scheduler/child/task_queue_impl.cc (right):

https://codereview.chromium.org/1259583006/diff/20001/components/scheduler/ch...
components/scheduler/child/task_queue_impl.cc:13: :
PendingTask(tracked_objects::Location(), base::Closure()) {}
Should we initialize age here and below?

https://codereview.chromium.org/1259583006/diff/20001/components/scheduler/ch...
components/scheduler/child/task_queue_impl.cc:293: incoming_queue_.back().age =
task_queue_manager_->GetNextAgeNumber();
This means we're doing two atomic increments per PostTask. I think that's
probably unavoidable for delayed tasks but can we make this cheaper for
immediate tasks?

https://codereview.chromium.org/1259583006/diff/20001/components/scheduler/ch...
File components/scheduler/child/task_queue_impl.h (right):

https://codereview.chromium.org/1259583006/diff/20001/components/scheduler/ch...
components/scheduler/child/task_queue_impl.h:23: struct SchedulerTask : public
base::PendingTask {
Sorry to go all bikeshed on this but I think TQM is still distinct from the
scheduler and at a level below. How about just calling this PendingTask or
simply Task?

https://codereview.chromium.org/1259583006/diff/20001/components/scheduler/ch...
components/scheduler/child/task_queue_impl.h:30: // tasks until enqueued on the
|incomming_queue_|.
typo: incoming

https://codereview.chromium.org/1259583006/diff/20001/components/scheduler/ch...
File components/scheduler/child/task_queue_manager.h (right):

https://codereview.chromium.org/1259583006/diff/20001/components/scheduler/ch...
components/scheduler/child/task_queue_manager.h:107: void
DidQueueTask(internal::SchedulerTask& pending_task);
The argument should be a pointer since we're modifying it.

[alex clarke (OOO till 29th), 2015-08-05 13:17:55]

On 2015/08/05 09:09:10, picksi wrote:
> Drive by comment... is "age" the correct name for this? I understand (a
little!)
> that you are trying to differentiate if from sequence number but age might be
> misleading, as older tasks will have lower 'ages'!

It's now listed as enqueue_order

[alex clarke (OOO till 29th), 2015-08-05 14:32:44]

PTAL

https://codereview.chromium.org/1259583006/diff/20001/components/scheduler/ch...
File components/scheduler/child/task_queue_impl.cc (right):

https://codereview.chromium.org/1259583006/diff/20001/components/scheduler/ch...
components/scheduler/child/task_queue_impl.cc:13: :
PendingTask(tracked_objects::Location(), base::Closure()) {}
On 2015/08/05 11:27:34, Sami wrote:
> Should we initialize age here and below?

Done.

https://codereview.chromium.org/1259583006/diff/20001/components/scheduler/ch...
File components/scheduler/child/task_queue_impl.h (right):

https://codereview.chromium.org/1259583006/diff/20001/components/scheduler/ch...
components/scheduler/child/task_queue_impl.h:23: struct SchedulerTask : public
base::PendingTask {
On 2015/08/05 11:27:34, Sami wrote:
> Sorry to go all bikeshed on this but I think TQM is still distinct from the
> scheduler and at a level below. How about just calling this PendingTask or
> simply Task?

Done.

https://codereview.chromium.org/1259583006/diff/20001/components/scheduler/ch...
components/scheduler/child/task_queue_impl.h:30: // tasks until enqueued on the
|incomming_queue_|.
On 2015/08/05 11:27:34, Sami wrote:
> typo: incoming

Done.

https://codereview.chromium.org/1259583006/diff/20001/components/scheduler/ch...
File components/scheduler/child/task_queue_manager.h (right):

https://codereview.chromium.org/1259583006/diff/20001/components/scheduler/ch...
components/scheduler/child/task_queue_manager.h:107: void
DidQueueTask(internal::SchedulerTask& pending_task);
On 2015/08/05 11:27:34, Sami wrote:
> The argument should be a pointer since we're modifying it.

Done.

[Sami, 2015-08-05 14:51:15]

https://codereview.chromium.org/1259583006/diff/40001/components/scheduler/ch...
File components/scheduler/child/task_queue_impl.cc (right):

https://codereview.chromium.org/1259583006/diff/40001/components/scheduler/ch...
components/scheduler/child/task_queue_impl.cc:104:
task_queue_manager_->GetNextSequenceNumber(),
Is the indentation right here?

https://codereview.chromium.org/1259583006/diff/40001/components/scheduler/ch...
components/scheduler/child/task_queue_impl.cc:208: return task->sequence_num <
oldest_queued_task.sequence_num;
Isn't this where we should look at the enqueue_order instead :)

I wonder if in debug builds we should somehow fill sequence_number with garbage
while it isn't needed to make sure we don't accidentally use it?

https://codereview.chromium.org/1259583006/diff/40001/components/scheduler/ch...
File components/scheduler/child/task_queue_impl.h (right):

https://codereview.chromium.org/1259583006/diff/40001/components/scheduler/ch...
components/scheduler/child/task_queue_impl.h:85: // If the work queue isn't
empty, |enqueue_order| gets set to the sequence
nit: s/sequence number/enqueue order/ right?

https://codereview.chromium.org/1259583006/diff/40001/components/scheduler/ch...
File components/scheduler/child/task_queue_sets_unittest.cc (right):

https://codereview.chromium.org/1259583006/diff/40001/components/scheduler/ch...
components/scheduler/child/task_queue_sets_unittest.cc:30: TaskQueueImpl::Task
FakeTaskWithEqueueOrder(int enqueue_order) {
typo: Enqueue

[alex clarke (OOO till 29th), 2015-08-05 15:33:05]

https://codereview.chromium.org/1259583006/diff/40001/components/scheduler/ch...
File components/scheduler/child/task_queue_impl.cc (right):

https://codereview.chromium.org/1259583006/diff/40001/components/scheduler/ch...
components/scheduler/child/task_queue_impl.cc:104:
task_queue_manager_->GetNextSequenceNumber(),
On 2015/08/05 14:51:15, Sami wrote:
> Is the indentation right here?

Done.

https://codereview.chromium.org/1259583006/diff/40001/components/scheduler/ch...
components/scheduler/child/task_queue_impl.cc:208: return task->sequence_num <
oldest_queued_task.sequence_num;
On 2015/08/05 14:51:14, Sami wrote:
> Isn't this where we should look at the enqueue_order instead :)
> 
> I wonder if in debug builds we should somehow fill sequence_number with
garbage
> while it isn't needed to make sure we don't accidentally use it?

Done.

https://codereview.chromium.org/1259583006/diff/40001/components/scheduler/ch...
File components/scheduler/child/task_queue_impl.h (right):

https://codereview.chromium.org/1259583006/diff/40001/components/scheduler/ch...
components/scheduler/child/task_queue_impl.h:85: // If the work queue isn't
empty, |enqueue_order| gets set to the sequence
On 2015/08/05 14:51:15, Sami wrote:
> nit: s/sequence number/enqueue order/ right?

Done.

https://codereview.chromium.org/1259583006/diff/40001/components/scheduler/ch...
File components/scheduler/child/task_queue_sets_unittest.cc (right):

https://codereview.chromium.org/1259583006/diff/40001/components/scheduler/ch...
components/scheduler/child/task_queue_sets_unittest.cc:30: TaskQueueImpl::Task
FakeTaskWithEqueueOrder(int enqueue_order) {
On 2015/08/05 14:51:15, Sami wrote:
> typo: Enqueue

Done.

[alex clarke (OOO till 29th), 2015-08-05 15:33:10]

The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-05 15:34:04]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1259583006/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1259583006/60001

[Sami, 2015-08-05 15:41:12]

Thanks, lgtm!

[Sami, 2015-08-05 15:41:36]

nit: the description is a little out of date.

[commit-bot: I haz the power, 2015-08-05 15:43:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-05 15:43:39]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[alex clarke (OOO till 29th), 2015-08-05 15:45:35]

The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-05 15:45:52]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1259583006/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1259583006/80001

[commit-bot: I haz the power, 2015-08-05 16:24:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-05 16:24:27]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[alexclarke, 2015-08-05 16:32:03]

The CQ bit was checked by alexclarke@google.com

[alexclarke, 2015-08-05 16:32:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/1259583006/#ps80001
(title: "Fix dcheck")

[commit-bot: I haz the power, 2015-08-05 16:32:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1259583006/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1259583006/80001

[commit-bot: I haz the power, 2015-08-05 17:19:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-05 17:19:45]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[alex clarke (OOO till 29th), 2015-08-06 09:50:40]

The CQ bit was checked by alexclarke@chromium.org

[commit-bot: I haz the power, 2015-08-06 09:51:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1259583006/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1259583006/80001

[commit-bot: I haz the power, 2015-08-06 10:53:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-06 10:54:00]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[alex clarke (OOO till 29th), 2015-08-06 10:58:13]

The CQ bit was checked by alexclarke@chromium.org

[alex clarke (OOO till 29th), 2015-08-06 10:58:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/1259583006/#ps100001
(title: "Add SCHEDULER_EXPORT to TaskQueueImpl::Task")

[commit-bot: I haz the power, 2015-08-06 10:58:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1259583006/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1259583006/100001

[commit-bot: I haz the power, 2015-08-06 12:44:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-06 12:44:08]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[alex clarke (OOO till 29th), 2015-08-06 12:46:55]

The CQ bit was checked by alexclarke@chromium.org

[commit-bot: I haz the power, 2015-08-06 12:47:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1259583006/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1259583006/100001

[commit-bot: I haz the power, 2015-08-06 14:37:23]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-08-06 14:38:04]

Patchset 6 (id:??) landed as
https://crrev.com/8c6b40b808787f00fef268e6c06eb02a0f8f690e
Cr-Commit-Position: refs/heads/master@{#342113}

[mfomitchev, 2015-08-06 15:34:06]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/1277923002/ by mfomitchev@chromium.org.

The reason for reverting is: This is causing failures on a bunch of bots,
particularly for the TaskQueueSelectorTest.TestAge test:
http://build.chromium.org/p/chromium.mac/builders/Mac10.9%20Tests%20%28dbg%29...
http://build.chromium.org/p/chromium.linux/builders/Linux%20Tests%20%28dbg%29...
http://build.chromium.org/p/chromium.linux/builders/Linux%20Tests%20%28dbg%29...

Also
BrowserCloseManagerBrowserTest_BrowserCloseManagerBrowserTest.PRE_TestSessionRestore_1
BrowserCloseManagerBrowserTest_BrowserCloseManagerBrowserTest.TestSessionRestore_1
failed on Mac in the same run. Not sure if related:
http://build.chromium.org/p/chromium.mac/builders/Mac10.9%20Tests%20%28dbg%29....

[alex clarke (OOO till 29th), 2015-08-06 17:30:15]

Thanks, I think that's unrelated.  I've fixed the DCHECK and will try to reland.

[alex clarke (OOO till 29th), 2015-08-06 17:30:36]

The CQ bit was checked by alexclarke@chromium.org

[alex clarke (OOO till 29th), 2015-08-06 17:30:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/1259583006/#ps120001
(title: "Fix the DCHECK")

[commit-bot: I haz the power, 2015-08-06 17:31:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1259583006/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1259583006/120001

[waffles, 2015-08-06 17:48:54]

On 2015/08/06 17:31:19, commit-bot: I haz the power wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1259583006/120001
> View timeline at
>  https://chromium-cq-status.appspot.com/patch-timeline/1259583006/120001

(Also Android:
http://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg%...)

[mfomitchev, 2015-08-06 17:51:50]

Hey Alex,
A bunch of components_unittests failed on Android Tests with 
crrev.com/1259583006. It hasn't caught up with my revert yet, but I see you are
relanding the CL. Can you pls take a look?
https://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg...

[chromium-reviews, 2015-08-06 18:12:45]

The problem on the android bots and on the desktop bots is the same thing,
its just the android test runner gives up after a dcheck.

The error was in a test which was not obeying the contract that a Task
enqueue_order is set once only. This should now be fixed in the reland
patch.

Shame the SQ didn't catch this :(
On 2015/08/06 17:31:19, commit-bot: I haz the power wrote:

> CQ is trying da patch. Follow status at
>   https://chromium-cq-status.appspot.com/patch-status/1259583006/120001
> View timeline at
>   https://chromium-cq-status.appspot.com/patch-timeline/1259583006/120001
>

(Also Android:
http://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg%...
)

https://codereview.chromium.org/1259583006/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2015-08-06 19:04:47]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-08-06 19:05:36]

Patchset 7 (id:??) landed as
https://crrev.com/879c9fb02f86ea29bec4ee45aa5e705948422dc1
Cr-Commit-Position: refs/heads/master@{#342166}