compositor worker: Use a WebThread for the compositor thread.

compositor worker: Use a WebThread for the compositor thread.

The first attempt (crrev.com/362924) had to be reverted (crrev.com/367051)
because it caused perf regressions (crbug.com/565789). It turns out, the
regression was caused because of using the default task-runner from the
scheduler, instead of directly using the thread's task runner. To fix this,
create a slightly specialized version of WebThreadImplForWorkerScheduler
that uses the thread's task runner as the default ask runner.

BUG=565789, 430155
Committed: https://crrev.com/ff12c00a9fbf19fc5ab5621d8e76031d5bef7c11
Cr-Commit-Position: refs/heads/master@{#369532}

[sadrul, 2016-01-13 16:40:06]

Patchset #1 (id:1) has been deleted

[sadrul, 2016-01-13 16:53:32]

Description was changed from

==========
compositor worker: Use a WebThread for the compositor.

[more details]

BUG=tbd
==========

to

==========
compositor worker: Use a WebThread for the compositor.

The first attempt (crrev.com/362924) had to be reverted (crrev.com/367051)
because it caused perf regressions (crbug.com/565789). It turns out, the
regression was caused because of using the default task-runner from the
scheduler, instead of directly using the thread's task runner. To fix this,
create a slightly specialized version of WebThreadImplForWorkerScheduler
that uses the thread's task runner as the default ask runner.

BUG=565789, 430155
==========

[sadrul, 2016-01-13 16:54:01]

Description was changed from

==========
compositor worker: Use a WebThread for the compositor.

The first attempt (crrev.com/362924) had to be reverted (crrev.com/367051)
because it caused perf regressions (crbug.com/565789). It turns out, the
regression was caused because of using the default task-runner from the
scheduler, instead of directly using the thread's task runner. To fix this,
create a slightly specialized version of WebThreadImplForWorkerScheduler
that uses the thread's task runner as the default ask runner.

BUG=565789, 430155
==========

to

==========
compositor worker: Use a WebThread for the compositor thread.

The first attempt (crrev.com/362924) had to be reverted (crrev.com/367051)
because it caused perf regressions (crbug.com/565789). It turns out, the
regression was caused because of using the default task-runner from the
scheduler, instead of directly using the thread's task runner. To fix this,
create a slightly specialized version of WebThreadImplForWorkerScheduler
that uses the thread's task runner as the default ask runner.

BUG=565789, 430155
==========

[sadrul, 2016-01-13 16:58:52]

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

[sadrul, 2016-01-13 16:58:52]

Hi! Would you mind reviewing the changes in components/scheduler and the
WebThreadForCompositor implementation in blink_platform_impl.cc?

An alternate approach I considered was to create a more restrictive
implementation of WorkerScheduler from the WebThreadForCompositor override, and
install that into WebThreadImplForWorkerScheduler*, so that the scheduler's
default-task-runner and the thread's task-runner is the same. I believe that
would also take care of the issue. I went with the simpler change in here, but
happy to switch the approach if you think that'd be better.

[Sami, 2016-01-13 17:17:05]

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

[Sami, 2016-01-13 17:17:05]

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

https://codereview.chromium.org/1589463002/diff/40001/components/scheduler/ch...
components/scheduler/child/webthread_impl_for_worker_scheduler.h:55: virtual
bool UseThreadTaskRunnerAsDefault() const;
This is a dangerous API because tasks will run in a different order depending on
which task queue they end up using, which can cause hard-to-diagnose breakage.
Could we instead try to find out what part of the scheduler code was responsible
for the regression and optimize that? I wouldn't expect the worker scheduler to
have that much overhead compared to a plain message loop -- but I might be wrong
since it's been a while since we last measured this.

[sadrul, 2016-01-13 18:29:32]

Appreciate the quick feedback!

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

https://codereview.chromium.org/1589463002/diff/40001/components/scheduler/ch...
components/scheduler/child/webthread_impl_for_worker_scheduler.h:55: virtual
bool UseThreadTaskRunnerAsDefault() const;
On 2016/01/13 17:17:05, Sami wrote:
> This is a dangerous API because tasks will run in a different order depending
on
> which task queue they end up using, which can cause hard-to-diagnose breakage.

Indeed, that's what I was worried about. A different implementation of
WorkerScheduler should avoid that from happening, right? I have implemented that
in the latest patchset (it now depends on
https://codereview.chromium.org/1585723002/).

> Could we instead try to find out what part of the scheduler code was
responsible
> for the regression and optimize that? I wouldn't expect the worker scheduler
to
> have that much overhead compared to a plain message loop -- but I might be
wrong
> since it's been a while since we last measured this.

It's actually not clear exactly what is causing the regression. As you say,
WorkerSchedulerImpl doesn't do a whole lot. It does install its own task runner
on the thread, and that seems sufficient to trigger the regression. I am not
familiar with this enough to understand how to go about finding the code
responsible for the regression. Any advice? And in the meantime, would it be
acceptable to have a CompositorWorkerScheduler implementation for
WorkerScheduler (see latest patchset)?

[Sami, 2016-01-14 13:55:05]

I think having a special CompositorWorkerScheduler is fine -- mainly because of
the GC issue I mentioned.

I'd really love to know what is causing the performance difference though. One
way to find out might be to run the affected benchmark with --profiler=perf with
and without the patch and seeing if anything bubbles to the top on the
compositor thread.

https://codereview.chromium.org/1589463002/diff/80001/components/scheduler/ch...
File components/scheduler/child/child_scheduler.h (right):

https://codereview.chromium.org/1589463002/diff/80001/components/scheduler/ch...
components/scheduler/child/child_scheduler.h:25: virtual
scoped_refptr<base::SingleThreadTaskRunner> DefaultTaskRunner() = 0;
Probably want to rebase this in top of your other patch?

https://codereview.chromium.org/1589463002/diff/80001/components/scheduler/ch...
File components/scheduler/child/webthread_impl_for_worker_scheduler.h (right):

https://codereview.chromium.org/1589463002/diff/80001/components/scheduler/ch...
components/scheduler/child/webthread_impl_for_worker_scheduler.h:54: virtual
scoped_ptr<scheduler::WorkerScheduler> CreateWorkerScheduler();
Looks like this patch is relative to your older one instead of tip-of-tree.
Could you rebase?

https://codereview.chromium.org/1589463002/diff/80001/content/child/blink_pla...
File content/child/blink_platform_impl.cc (right):

https://codereview.chromium.org/1589463002/diff/80001/content/child/blink_pla...
content/child/blink_platform_impl.cc:93: class CompositorWorkerScheduler :
public scheduler::WorkerScheduler {
Would you mind putting this to
components/scheduler/renderer/compositor_worker_scheduler.{cc,h} instead? Right
now there's not much here but I think we might want to integrate this more
closely with the other scheduling machinery in the future.

https://codereview.chromium.org/1589463002/diff/80001/content/child/blink_pla...
content/child/blink_platform_impl.cc:100: return thread_->task_runner();
If we do decide to land this pass through scheduler as a first step, could you
add a TODO here for adding an actual scheduler here and making sure performance
does not regress?

https://codereview.chromium.org/1589463002/diff/80001/content/child/blink_pla...
content/child/blink_platform_impl.cc:105: return nullptr;
Presumably the compositor worker will need to do some garbage collection at some
point? This is normally done using idle tasks, but if we don't support them v8
falls back to inline GC, which might cause janks. That might be fine for now,
but let's at least have a TODO here mentioning this.

[sadrul, 2016-01-14 15:20:40]

https://codereview.chromium.org/1589463002/diff/80001/components/scheduler/ch...
File components/scheduler/child/child_scheduler.h (right):

https://codereview.chromium.org/1589463002/diff/80001/components/scheduler/ch...
components/scheduler/child/child_scheduler.h:25: virtual
scoped_refptr<base::SingleThreadTaskRunner> DefaultTaskRunner() = 0;
On 2016/01/14 13:55:04, Sami wrote:
> Probably want to rebase this in top of your other patch?

Yep, done.

https://codereview.chromium.org/1589463002/diff/80001/components/scheduler/ch...
File components/scheduler/child/webthread_impl_for_worker_scheduler.h (right):

https://codereview.chromium.org/1589463002/diff/80001/components/scheduler/ch...
components/scheduler/child/webthread_impl_for_worker_scheduler.h:54: virtual
scoped_ptr<scheduler::WorkerScheduler> CreateWorkerScheduler();
On 2016/01/14 13:55:04, Sami wrote:
> Looks like this patch is relative to your older one instead of tip-of-tree.
> Could you rebase?

Yep, sorry. Done.

https://codereview.chromium.org/1589463002/diff/80001/content/child/blink_pla...
File content/child/blink_platform_impl.cc (right):

https://codereview.chromium.org/1589463002/diff/80001/content/child/blink_pla...
content/child/blink_platform_impl.cc:93: class CompositorWorkerScheduler :
public scheduler::WorkerScheduler {
On 2016/01/14 13:55:05, Sami wrote:
> Would you mind putting this to
> components/scheduler/renderer/compositor_worker_scheduler.{cc,h} instead?
Right
> now there's not much here but I think we might want to integrate this more
> closely with the other scheduling machinery in the future.

Done (moved to */child/* instead of in */renderer/* since this is going to be
used from here in content/child/).

https://codereview.chromium.org/1589463002/diff/80001/content/child/blink_pla...
content/child/blink_platform_impl.cc:100: return thread_->task_runner();
On 2016/01/14 13:55:05, Sami wrote:
> If we do decide to land this pass through scheduler as a first step, could you
> add a TODO here for adding an actual scheduler here and making sure
performance
> does not regress?

Done.

https://codereview.chromium.org/1589463002/diff/80001/content/child/blink_pla...
content/child/blink_platform_impl.cc:105: return nullptr;
On 2016/01/14 13:55:05, Sami wrote:
> Presumably the compositor worker will need to do some garbage collection at
some
> point? This is normally done using idle tasks, but if we don't support them v8
> falls back to inline GC, which might cause janks. That might be fine for now,
> but let's at least have a TODO here mentioning this.

Done.

[Sami, 2016-01-14 17:57:08]

Thanks, we can land this pass-through scheduler for now and keep investigating
the perf regression -- could you file a bug with your findings?

components/scheduler lgtm.

[sadrul, 2016-01-14 18:22:05]

Thank you!

> I'd really love to know what is causing the performance difference though. One
> way to find out might be to run the affected benchmark with --profiler=perf
with
> and without the patch and seeing if anything bubbles to the top on the
> compositor thread.

I ran the benchmark with profiler turned on, both with and without the patch.
The top of the list from both are pretty similar, with no notable differences.
However, if I 'zoom into libscheduler.so DSO', then there are some notable
differences. I am not sure how relevant or telling that is, but here's the
summary: with the changes in this CL, approx event-count is ~145mil, while with
the original approach (i.e. directly using WebThreadImplForWorkerScheduler), the
approx. event count goes up to ~430mil.
TaskQueueManager::ProcessTaskFromWorkQueue shows up towards the top of that
zoom'ed view with WebThreadImplForWorkerScheduler (~2.15%), while it's much
farther down with WebThreadForCompositor (~0.99%). The difference is likely just
because the tasks are going through the TQM, vs being run directly by the
message-pump. But nothing else seemed interesting.

[sadrul, 2016-01-14 19:15:32]

sadrul@chromium.org changed reviewers:
+ piman@chromium.org

[sadrul, 2016-01-14 19:15:33]

+piman@ to review the changes in content/ (I have tried to make sure the
concerns raised in the previous CL (https://codereview.chromium.org/1449953002/)
are addressed here too)

[piman, 2016-01-14 19:34:11]

lgtm

[sadrul, 2016-01-14 19:54:49]

The CQ bit was checked by sadrul@chromium.org

[sadrul, 2016-01-14 19:54:50]

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/1589463002/#ps160001
(title: "self.nit")

[commit-bot: I haz the power, 2016-01-14 19:56:32]

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

[commit-bot: I haz the power, 2016-01-14 20:52:27]

Description was changed from

==========
compositor worker: Use a WebThread for the compositor thread.

The first attempt (crrev.com/362924) had to be reverted (crrev.com/367051)
because it caused perf regressions (crbug.com/565789). It turns out, the
regression was caused because of using the default task-runner from the
scheduler, instead of directly using the thread's task runner. To fix this,
create a slightly specialized version of WebThreadImplForWorkerScheduler
that uses the thread's task runner as the default ask runner.

BUG=565789, 430155
==========

to

==========
compositor worker: Use a WebThread for the compositor thread.

The first attempt (crrev.com/362924) had to be reverted (crrev.com/367051)
because it caused perf regressions (crbug.com/565789). It turns out, the
regression was caused because of using the default task-runner from the
scheduler, instead of directly using the thread's task runner. To fix this,
create a slightly specialized version of WebThreadImplForWorkerScheduler
that uses the thread's task runner as the default ask runner.

BUG=565789, 430155
==========

[commit-bot: I haz the power, 2016-01-14 20:52:27]

Committed patchset #8 (id:160001)

[commit-bot: I haz the power, 2016-01-14 20:53:49]

Description was changed from

==========
compositor worker: Use a WebThread for the compositor thread.

The first attempt (crrev.com/362924) had to be reverted (crrev.com/367051)
because it caused perf regressions (crbug.com/565789). It turns out, the
regression was caused because of using the default task-runner from the
scheduler, instead of directly using the thread's task runner. To fix this,
create a slightly specialized version of WebThreadImplForWorkerScheduler
that uses the thread's task runner as the default ask runner.

BUG=565789, 430155
==========

to

==========
compositor worker: Use a WebThread for the compositor thread.

The first attempt (crrev.com/362924) had to be reverted (crrev.com/367051)
because it caused perf regressions (crbug.com/565789). It turns out, the
regression was caused because of using the default task-runner from the
scheduler, instead of directly using the thread's task runner. To fix this,
create a slightly specialized version of WebThreadImplForWorkerScheduler
that uses the thread's task runner as the default ask runner.

BUG=565789, 430155

Committed: https://crrev.com/ff12c00a9fbf19fc5ab5621d8e76031d5bef7c11
Cr-Commit-Position: refs/heads/master@{#369532}
==========

[commit-bot: I haz the power, 2016-01-14 20:53:50]

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