New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 787647 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Feature

Blocking:
issue 768253



Sign in to add a comment

Make sure inproc GPU service is started before navigation starts

Project Member Reported by dskiba@chromium.org, Nov 22 2017

Issue description

This is on low-end device, i.e. GPU service lives inside browser process.
The device is "gobo" running Android O (OMB1.171105.001).

I noticed that sometimes GpuService is initialized before navigation starts and sometimes after.

See the screenshots, where:

* Green line is SingleThreadProxy::ScheduledActionBeginLayerTreeFrameSinkCreation
* Blue line is SingleThreadProxy::RequestNewLayerTreeFrameSink

I.e. ScheduledActionBeginLayerTreeFrameSinkCreation() posts a task that invokes RequestNewLayerTreeFrameSink(), and sometimes that task is invoked after ChromeTabbedActivity.initializeState (which is ~180ms).

Note that ChromeTabbedActivity.initializeState is Java task, while SingleThreadProxy tasks are native. I'm still investigating what exactly is happening, but I doubt that we'll be able to enforce ordering between Java and native tasks (e.g. native tasks are asynchronous tasks, while tasks posted from Java are synchronous).

Anyway, back to the issue. When GpuService is started after navigation, renderer ends up waiting for quite some time in Gpu::EstablishGpuChannelSync. For example in gpu_after_navigation.html renderer waited there for 390ms.

Is it possible to avoid posting a task in ScheduledActionBeginLayerTreeFrameSinkCreation? The comment there seems to imply that we do that only to streamline tests.

Alternatively, can we start GpuService even before that? During browser initialization for example?

 
gpu_before_navigation.png
51.1 KB View Download
gpu_after_navigation.png
45.7 KB View Download
gpu_after_navigation_renderer.png
83.9 KB View Download
gpu_before_navigation.html.gz
1.7 MB Download
gpu_after_navigation.html.gz
1.7 MB Download

Comment 1 by dskiba@chromium.org, Nov 22 2017

Components: Internals>Compositing
Here is how RequestNewLayerTreeFrameSink triggers creation of Chrome_InProcGpuThread thread:

SingleThreadProxy::RequestNewLayerTreeFrameSink ->
	BrowserGpuChannelHostFactory::EstablishRequest::Create -> (posts on IO)
	BrowserGpuChannelHostFactory::EstablishRequest::EstablishOnIO ->
	Thread("Chrome_InProcGpuThread")

Some frames are missing though.

Comment 2 by dskiba@chromium.org, Nov 22 2017

Cc: yfried...@chromium.org mariakho...@chromium.org piman@chromium.org

Comment 3 by dskiba@chromium.org, Nov 22 2017

Blocking: 768253

Comment 4 by dskiba@chromium.org, Nov 22 2017

Cc: ericrk@chromium.org

Comment 5 by dskiba@chromium.org, Nov 23 2017

It turned out that ordering of Java / native tasks is not guaranteed at all, see issue 788008.

Comment 6 by danakj@chromium.org, Nov 24 2017

Status: Available (was: Untriaged)

Comment 7 by fsamuel@google.com, Nov 24 2017

Labels: -Type-Bug Type-Feature

Comment 8 by piman@chromium.org, Nov 27 2017

Cc: boliu@chromium.org

Comment 9 by boliu@chromium.org, Nov 27 2017

we skip gpu warm up if gpu is in-process, probably under the assumption that starting a thread is faster than launching a process (which is totally true on android): https://cs.chromium.org/chromium/src/content/browser/browser_main_loop.cc?rcl=72bc6da43e2b5b52a8a32508fc5f777fbc6053e9&l=1633

but maybe still not fast enough? I find navigation generally tends to do a lot of work on IO thread, so can't be the best point to start the gpu service either

Comment 10 by boliu@chromium.org, Nov 27 2017

> probably under the assumption that starting a thread is faster than launching a process

Err, no, that's not it, at least according to the comment right above where I linked.

  // When running the GPU thread in-process, avoid optimistically starting it
  // since creating the GPU thread races against creation of the one-and-only
  // ChildProcess instance which is created by the renderer thread.

Although I don't really understand that comment
Thanks for the pointer, Bo! That comment indeed looks very cryptic.
OK, so that check (!(kSingleProcess || kInProcessGPU), later refactored into !UsingInProcessGpu) was there initially from 2012: https://chromiumcodereview.appspot.com/11469022

Later in  issue 450396  sievers@ explained why it was that way, and you both agreed that the concern is probably no longer valid.

So I guess I'll experiment with removing UsingInProcessGpu() check from there.
Owner: dskiba@chromium.org
Status: Started (was: Available)
Project Member

Comment 14 by bugdroid1@chromium.org, Dec 5 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6f43f72247f52443c52ee60a908b174cbd90a76b

commit 6f43f72247f52443c52ee60a908b174cbd90a76b
Author: Dmitry Skiba <dskiba@chromium.org>
Date: Tue Dec 05 22:37:28 2017

Start in-proc GPU early.

On low-end Android devices in-proc GPU is started on demand, when
CompositorImpl::SetVisible(true) is called for the first time.

Due to the fact that native / Java messages on Android are not strictly
ordered (see crbug.com/788008) sometimes GPU startup is delayed for too
long, stalling the renderer (see the bug).

We already have code to start GPU early (from BrowserThreadsStarted),
but that code includes explicit !UsingInProcessGpu() check.

However, the check (which was there from 2012), seems obsolete now. As
sievers@ and boliu@ discussed in  crbug.com/450396#c4  the check was there
to avoid a race setting ChildProcess::current, but became obsolete once
ChildProcess::current switched to TLS.

This CL removes !UsingInProcessGpu() check, causing in-proc GPU to start
early.

Bug:  787647 
Change-Id: Ifb6764e8b479003412674ce667dd6c3ea08dcb43
Reviewed-on: https://chromium-review.googlesource.com/804144
Reviewed-by: Antoine Labour <piman@chromium.org>
Commit-Queue: Dmitry Skiba <dskiba@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521865}
[modify] https://crrev.com/6f43f72247f52443c52ee60a908b174cbd90a76b/content/browser/browser_main_loop.cc

Status: Fixed (was: Started)

Sign in to add a comment