Introduce Document::unthrottledTaskRunner()

Introduce Document::unthrottledTaskRunner()

This CL introduces Document::unthrottledTaskRunner() and move one task in
HTMLCanvasElement to the unthrottled task runner. We'll move more tasks to
the unthrottled task runner.

BUG=624696
Committed: https://crrev.com/31b559c09d324ae0c8b8c12399dd95b0bbc4dd9d
Cr-Commit-Position: refs/heads/master@{#404598}

[haraken, 2016-07-08 07:10:45]

haraken@chromium.org changed reviewers:
+ alexclarke@chromium.org, dcheng@chromium.org, skyostil@chromium.org

[haraken, 2016-07-08 07:10:45]

PTAL

Let's start with easier parts before worrying about MainThreadTaskRunner :)

[Sami, 2016-07-08 09:13:29]

lgtm with some thoughts.

https://codereview.chromium.org/2133623002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/2133623002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:683:
document().unthrottledTaskRunner()->postTask(BLINK_FROM_HERE,
WTF::bind(&BlobCallback::handleEvent, wrapPersistent(callback), nullptr));
Looks like this can execute script. I wonder if it should be throttled or not?
It wasn't throttled before so you're not really changing anything, but we should
think about whether we need another task runner for things like these (or if we
should just reuse the timer task runner).

[dcheng, 2016-07-08 10:27:37]

https://codereview.chromium.org/2133623002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/2133623002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:683:
document().unthrottledTaskRunner()->postTask(BLINK_FROM_HERE,
WTF::bind(&BlobCallback::handleEvent, wrapPersistent(callback), nullptr));
On 2016/07/08 09:13:29, Sami wrote:
> Looks like this can execute script. I wonder if it should be throttled or not?
> It wasn't throttled before so you're not really changing anything, but we
should
> think about whether we need another task runner for things like these (or if
we
> should just reuse the timer task runner).

What are "things like these"?

To me, the distinction between loading and non-loading tasks is pretty clear, so
a loading task runner makes sense.

The distinction between timer and unthrottled tasks is also clear, so those task
runners make sense.

I'm having trouble understanding what category a fourth task runner would
fulfill though.

[dcheng, 2016-07-08 10:28:59]

(The code change LGTM though)

[Sami, 2016-07-08 10:43:58]

On 2016/07/08 10:27:37, dcheng wrote:
> What are "things like these"?
> 
> To me, the distinction between loading and non-loading tasks is pretty clear,
so
> a loading task runner makes sense.
> 
> The distinction between timer and unthrottled tasks is also clear, so those
task
> runners make sense.
> 
> I'm having trouble understanding what category a fourth task runner would
> fulfill though.

That's a great question. In general we try to classify work into semantic
categories like loading, rendering, input, etc. instead of trying to say
something about their "importance". This lets the scheduler dynamically decide
which category is important at any given time based on its knowledge of system
state.

"Unthrottled" goes a bit against this idea since it's really telling the
scheduler that these tasks shouldn't be messed with, but not really saying what
those tasks actually are. In this particular case it's a bit hard to say what
category toBlob() fits in. I would be tempted to call it a rendering task and
say that throttling it is actually fine if necessary.

[haraken, 2016-07-11 01:54:18]

On 2016/07/08 10:43:58, Sami (OoO) wrote:
> On 2016/07/08 10:27:37, dcheng wrote:
> > What are "things like these"?
> > 
> > To me, the distinction between loading and non-loading tasks is pretty
clear,
> so
> > a loading task runner makes sense.
> > 
> > The distinction between timer and unthrottled tasks is also clear, so those
> task
> > runners make sense.
> > 
> > I'm having trouble understanding what category a fourth task runner would
> > fulfill though.
> 
> That's a great question. In general we try to classify work into semantic
> categories like loading, rendering, input, etc. instead of trying to say
> something about their "importance". This lets the scheduler dynamically decide
> which category is important at any given time based on its knowledge of system
> state.
> 
> "Unthrottled" goes a bit against this idea since it's really telling the
> scheduler that these tasks shouldn't be messed with, but not really saying
what
> those tasks actually are. In this particular case it's a bit hard to say what
> category toBlob() fits in. I would be tempted to call it a rendering task and
> say that throttling it is actually fine if necessary.

Then how about organizing the task runners as follows?

- Timer task runner (<-- timers should go here)
- Loading task runner (<-- loading tasks should go here)
- Rendering task runner (<-- rendering tasks should go here)
- Input task runner (<-- input tasks should go here)
- ...
- Throttled task runner (<-- other tasks that can be throttled should go here)
- Unthrottled task runner (<-- tasks that should not be throttled should go
here)

Sami is OOO for two weeks and it would be better to restructure the task runners
after Sami comes back.

So let me land this CL (using the unthrottled task runner).

[haraken, 2016-07-11 01:54:24]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2016-07-11 01:54:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-11 04:13:54]

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

[commit-bot: I haz the power, 2016-07-11 04:13:55]

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

[haraken, 2016-07-11 04:18:41]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2016-07-11 04:18:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-11 05:33:18]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-07-11 05:33:24]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-11 05:34:56]

Description was changed from

==========
Introduce Document::unthrottledTaskRunner()

This CL introduces Document::unthrottledTaskRunner() and move one task in
HTMLCanvasElement to the unthrottled task runner. We'll move more tasks to
the unthrottled task runner.

BUG=624696
==========

to

==========
Introduce Document::unthrottledTaskRunner()

This CL introduces Document::unthrottledTaskRunner() and move one task in
HTMLCanvasElement to the unthrottled task runner. We'll move more tasks to
the unthrottled task runner.

BUG=624696

Committed: https://crrev.com/31b559c09d324ae0c8b8c12399dd95b0bbc4dd9d
Cr-Commit-Position: refs/heads/master@{#404598}
==========

[commit-bot: I haz the power, 2016-07-11 05:34:57]

Patchset 2 (id:??) landed as
https://crrev.com/31b559c09d324ae0c8b8c12399dd95b0bbc4dd9d
Cr-Commit-Position: refs/heads/master@{#404598}

[blink-reviews, 2016-07-11 09:01:43]

On Mon, Jul 11, 2016 at 2:54 AM, <haraken@chromium.org> wrote:

> On 2016/07/08 10:43:58, Sami (OoO) wrote:
> > On 2016/07/08 10:27:37, dcheng wrote:
> > > What are "things like these"?
> > >
> > > To me, the distinction between loading and non-loading tasks is pretty
> clear,
> > so
> > > a loading task runner makes sense.
> > >
> > > The distinction between timer and unthrottled tasks is also clear, so
> those
> > task
> > > runners make sense.
> > >
> > > I'm having trouble understanding what category a fourth task runner
> would
> > > fulfill though.
> >
> > That's a great question. In general we try to classify work into semantic
> > categories like loading, rendering, input, etc. instead of trying to say
> > something about their "importance". This lets the scheduler dynamically
> decide
> > which category is important at any given time based on its knowledge of
> system
> > state.
> >
> > "Unthrottled" goes a bit against this idea since it's really telling the
> > scheduler that these tasks shouldn't be messed with, but not really
> saying
> what
> > those tasks actually are. In this particular case it's a bit hard to say
> what
> > category toBlob() fits in. I would be tempted to call it a rendering
> task and
> > say that throttling it is actually fine if necessary.
>
> Then how about organizing the task runners as follows?
>
> - Timer task runner (<-- timers should go here)
> - Loading task runner (<-- loading tasks should go here)
> - Rendering task runner (<-- rendering tasks should go here)
> - Input task runner (<-- input tasks should go here)
>

Currently input and rendering tasks are not reorderable.  We could consider
having seperate queues for attribution, but they'd need to have the same
settings for priority etc..



> - ...
> - Throttled task runner (<-- other tasks that can be throttled should go
> here)
> - Unthrottled task runner (<-- tasks that should not be throttled should go
> here)
>
> Sami is OOO for two weeks and it would be better to restructure the task
> runners
> after Sami comes back.
>
> So let me land this CL (using the unthrottled task runner).
>
>
> https://codereview.chromium.org/2133623002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2016-07-11 09:01:43]

On Mon, Jul 11, 2016 at 2:54 AM, <haraken@chromium.org> wrote:

> On 2016/07/08 10:43:58, Sami (OoO) wrote:
> > On 2016/07/08 10:27:37, dcheng wrote:
> > > What are "things like these"?
> > >
> > > To me, the distinction between loading and non-loading tasks is pretty
> clear,
> > so
> > > a loading task runner makes sense.
> > >
> > > The distinction between timer and unthrottled tasks is also clear, so
> those
> > task
> > > runners make sense.
> > >
> > > I'm having trouble understanding what category a fourth task runner
> would
> > > fulfill though.
> >
> > That's a great question. In general we try to classify work into semantic
> > categories like loading, rendering, input, etc. instead of trying to say
> > something about their "importance". This lets the scheduler dynamically
> decide
> > which category is important at any given time based on its knowledge of
> system
> > state.
> >
> > "Unthrottled" goes a bit against this idea since it's really telling the
> > scheduler that these tasks shouldn't be messed with, but not really
> saying
> what
> > those tasks actually are. In this particular case it's a bit hard to say
> what
> > category toBlob() fits in. I would be tempted to call it a rendering
> task and
> > say that throttling it is actually fine if necessary.
>
> Then how about organizing the task runners as follows?
>
> - Timer task runner (<-- timers should go here)
> - Loading task runner (<-- loading tasks should go here)
> - Rendering task runner (<-- rendering tasks should go here)
> - Input task runner (<-- input tasks should go here)
>

Currently input and rendering tasks are not reorderable.  We could consider
having seperate queues for attribution, but they'd need to have the same
settings for priority etc..



> - ...
> - Throttled task runner (<-- other tasks that can be throttled should go
> here)
> - Unthrottled task runner (<-- tasks that should not be throttled should go
> here)
>
> Sami is OOO for two weeks and it would be better to restructure the task
> runners
> after Sami comes back.
>
> So let me land this CL (using the unthrottled task runner).
>
>
> https://codereview.chromium.org/2133623002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Sami, 2016-07-25 13:28:37]

On 2016/07/11 09:01:43, chromium-reviews wrote:
> On Mon, Jul 11, 2016 at 2:54 AM, <mailto:haraken@chromium.org> wrote:
> 
> > On 2016/07/08 10:43:58, Sami (OoO) wrote:
> > > On 2016/07/08 10:27:37, dcheng wrote:
> > > > What are "things like these"?
> > > >
> > > > To me, the distinction between loading and non-loading tasks is pretty
> > clear,
> > > so
> > > > a loading task runner makes sense.
> > > >
> > > > The distinction between timer and unthrottled tasks is also clear, so
> > those
> > > task
> > > > runners make sense.
> > > >
> > > > I'm having trouble understanding what category a fourth task runner
> > would
> > > > fulfill though.
> > >
> > > That's a great question. In general we try to classify work into semantic
> > > categories like loading, rendering, input, etc. instead of trying to say
> > > something about their "importance". This lets the scheduler dynamically
> > decide
> > > which category is important at any given time based on its knowledge of
> > system
> > > state.
> > >
> > > "Unthrottled" goes a bit against this idea since it's really telling the
> > > scheduler that these tasks shouldn't be messed with, but not really
> > saying
> > what
> > > those tasks actually are. In this particular case it's a bit hard to say
> > what
> > > category toBlob() fits in. I would be tempted to call it a rendering
> > task and
> > > say that throttling it is actually fine if necessary.
> >
> > Then how about organizing the task runners as follows?
> >
> > - Timer task runner (<-- timers should go here)
> > - Loading task runner (<-- loading tasks should go here)
> > - Rendering task runner (<-- rendering tasks should go here)
> > - Input task runner (<-- input tasks should go here)
> >
> 
> Currently input and rendering tasks are not reorderable.  We could consider
> having seperate queues for attribution, but they'd need to have the same
> settings for priority etc..

That's right. I agree that giving the queues separate names might help with
attribution.

> 
> > - ...
> > - Throttled task runner (<-- other tasks that can be throttled should go
> > here)
> > - Unthrottled task runner (<-- tasks that should not be throttled should go
> > here)
> >
> > Sami is OOO for two weeks and it would be better to restructure the task
> > runners
> > after Sami comes back.
> >
> > So let me land this CL (using the unthrottled task runner).
> >
> >
> > https://codereview.chromium.org/2133623002/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.