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

Issue 636164 link

Starred by 1 user
Status: Fixed
Owner:
boliu@chromium.org
Closed: Apr 2017
Cc:
█ ojars@google.com
tobiasjs@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug

Blocked on:
issue 661504
issue 661933
issue 662096
issue 664107
issue 664198
issue 665366


Sign in to add a comment

[Android WebView] Async OnDraw

Project Member Reported by boliu@chromium.org, Aug 10 2016 
Reduce IPC overhead and improve frame scheduling

Basic idea is make render thread kModeDraw wait for the frame instead of blocking UI thread. ojars has a prototype already that just makes OnDraw async, but does not have the second part of making kModeDraw wait for the frame: https://codereview.chromium.org/2174203002/


Then some thought dumps:

should use a filter on browser side IO thread to signal frame ready event, don't need to go back to UI thread

OnDraw for frame N+1 should block on having getting the result of frame N, to so that latency doesn't go through the roof. This may be trivial actually, since vsync is still synchronous.

As a first step, CommonRendererParams still needs to be sent back synchronously in OnDraw, but generating that does not require waiting for draw, so draw is still async. However, this means OnDraw IPC is still synchronous, just a bit faster.

The holy grail would be to make the IPC be async. Benefit would be significant say when multiple webviews are drawing. So the only important data in CommonRendererParams things like scroll offset, because drawing children of webview depends on that. ComputeScroll is async, which means the synchronous OnDraw IPC is also responsible for sending back the scroll update from ComputeScroll. So if we could determine ahead of time that ComputeScroll and OnDraw will not change the scroll offset (or other values), then we can safely send an async draw, and not wait for CommonRendererParams.

There are two sources of scroll change to consider: js update, and compute scroll (input is not on the list because vsync is synchronous and comes after input).
There is never going to be a way to correctly deal with js scroll without using synchronous IPC, because it can in theory happen at any point. But it is rare in practice, and synchronizing js never works, so possibly not important.
ComputeScroll is only really needed for ticking root fling animations. So if renderer can send back ahead of time if it's needed, then we can make draws async when they are not needed. And we already do this: need_animate_scroll in CommonRendererParams

So I think except for fling animations, OnDraw IPC can actually be async.

Comment 1 by boliu@chromium.org, Aug 10 2016 

fwiw this should be assigned to ojars, but I'm getting "Issue owner must be a project member" :/
Project Member

Comment 2 by bugdroid1@chromium.org, Sep 30 2016 

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

commit ce5a8f01558bc67c810f4c261af4402ef4bf59db
Author: ojars <ojars@google.com>
Date: Fri Sep 30 20:04:38 2016

Added FrameFuture class

FrameFuture will be passed to hardware renderer so it could block
until the actual frame is ready.

BUG= 636164 

Review-Url: https://codereview.chromium.org/2347563003
Cr-Commit-Position: refs/heads/master@{#422197}

[modify] https://crrev.com/ce5a8f01558bc67c810f4c261af4402ef4bf59db/android_webview/browser/browser_view_renderer.cc
[modify] https://crrev.com/ce5a8f01558bc67c810f4c261af4402ef4bf59db/android_webview/browser/browser_view_renderer.h
[modify] https://crrev.com/ce5a8f01558bc67c810f4c261af4402ef4bf59db/android_webview/browser/compositor_frame_consumer.h
[modify] https://crrev.com/ce5a8f01558bc67c810f4c261af4402ef4bf59db/android_webview/browser/render_thread_manager.cc
[modify] https://crrev.com/ce5a8f01558bc67c810f4c261af4402ef4bf59db/android_webview/browser/render_thread_manager.h
[modify] https://crrev.com/ce5a8f01558bc67c810f4c261af4402ef4bf59db/content/browser/android/synchronous_compositor_host.cc
[modify] https://crrev.com/ce5a8f01558bc67c810f4c261af4402ef4bf59db/content/browser/android/synchronous_compositor_host.h
[modify] https://crrev.com/ce5a8f01558bc67c810f4c261af4402ef4bf59db/content/public/browser/android/synchronous_compositor.cc
[modify] https://crrev.com/ce5a8f01558bc67c810f4c261af4402ef4bf59db/content/public/browser/android/synchronous_compositor.h
[modify] https://crrev.com/ce5a8f01558bc67c810f4c261af4402ef4bf59db/content/public/browser/android/synchronous_compositor_client.h

Comment 3 by hush@chromium.org, Oct 7 2016 

The following commits should also be included:

commit 5339299e1c78677caaf1c3b434ac8f154f0468ec
Author: ojars <ojars@google.com>
Date:   Fri Sep 2 17:09:06 2016 -0700

    Adds alternative OnDrawHardware() implementation that uses async IPC messages
    
    The purpose of this CL is to remove unnecessary blocking on UI thread which
    happens when BrowserViewRenderer sends a synchronous IPC message to get the
    next hardware frame. This new implementation instead sends an async message
    and receives the frame with another async message from render thread.
    This change alone is not sufficient as it causes scroll latency.
    The async implementation is turned on by command line param.
    
    Review-Url: https://codereview.chromium.org/2174203002
    Cr-Commit-Position: refs/heads/master@{#416404}

commit 0d61a39114539db5d506f5cb4fa645477444b6c4
Author: ojars <ojars@google.com>
Date:   Mon Aug 1 12:21:48 2016 -0700

    Removed unnecessary declaration.
    
    The declared function's implementation was removed by refactoring.
    
    Review-Url: https://codereview.chromium.org/2202693002
    Cr-Commit-Position: refs/heads/master@{#409018}

commit 999676d872ff3cbcb0c1e225b049be11b8db2ee1
Author: ojars <ojars@google.com>
Date:   Mon Jul 18 18:52:23 2016 -0700

    Refactored an entry struct into separate storage.
    
    Now synchronous input handler proxies and output surfaces corresponding to some routing_id will be stored in separate maps.
    Previously they were stored together in a struct.
    
    Review-Url: https://codereview.chromium.org/2131783002
    Cr-Commit-Position: refs/heads/master@{#406172}

Comment 4 by boliu@chromium.org, Oct 7 2016 

second and third in that list are not related

Also the working but unreviewed CL which adds the wait on render thread: https://codereview.chromium.org/2383933002/
Project Member

Comment 5 by bugdroid1@chromium.org, Oct 24 2016 

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

commit c37cd75b3d77fb35953685c01df5adf1c744b55c
Author: boliu <boliu@chromium.org>
Date: Mon Oct 24 17:54:39 2016

sync compositor: Signal async frame on IO thread

Continuation of CL by ojars@ here:
https://codereview.chromium.org/2383933002/

Async hardware draw synchronously returns a frame future that is
fulfilled asynchronously on the IO thread. This allows the frame future
to be waited on either the UI thread or android's render thread without
deadlocks.

In content, make SynchronousCompositorObserver a true message
filter owned by RenderProcessHostImpl. Will rename this to SCFilter
in a later CL.

In android_webview, wait on the frame future in kModeDraw,
immediately before where the frame is needed, to maximize
parallelization.

Note that there is a hack in BVR to produce a synchronous frame
first to ensure that bindings are initialized before render thread runs
any code.

This makes a complete more or less working path. But there are still
lots of TODOs needs to be fixed before the async path is ready to
ship.

BUG= 636164 

Review-Url: https://codereview.chromium.org/2418383002
Cr-Commit-Position: refs/heads/master@{#427096}

[modify] https://crrev.com/c37cd75b3d77fb35953685c01df5adf1c744b55c/android_webview/browser/browser_view_renderer.cc
[modify] https://crrev.com/c37cd75b3d77fb35953685c01df5adf1c744b55c/android_webview/browser/browser_view_renderer.h
[modify] https://crrev.com/c37cd75b3d77fb35953685c01df5adf1c744b55c/android_webview/browser/child_frame.h
[modify] https://crrev.com/c37cd75b3d77fb35953685c01df5adf1c744b55c/android_webview/browser/hardware_renderer.cc
[modify] https://crrev.com/c37cd75b3d77fb35953685c01df5adf1c744b55c/android_webview/browser/hardware_renderer.h
[modify] https://crrev.com/c37cd75b3d77fb35953685c01df5adf1c744b55c/android_webview/browser/render_thread_manager.cc
[modify] https://crrev.com/c37cd75b3d77fb35953685c01df5adf1c744b55c/android_webview/browser/render_thread_manager.h
[modify] https://crrev.com/c37cd75b3d77fb35953685c01df5adf1c744b55c/content/browser/android/synchronous_compositor_host.cc
[modify] https://crrev.com/c37cd75b3d77fb35953685c01df5adf1c744b55c/content/browser/android/synchronous_compositor_host.h
[modify] https://crrev.com/c37cd75b3d77fb35953685c01df5adf1c744b55c/content/browser/android/synchronous_compositor_observer.cc
[modify] https://crrev.com/c37cd75b3d77fb35953685c01df5adf1c744b55c/content/browser/android/synchronous_compositor_observer.h
[modify] https://crrev.com/c37cd75b3d77fb35953685c01df5adf1c744b55c/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/c37cd75b3d77fb35953685c01df5adf1c744b55c/content/browser/renderer_host/render_process_host_impl.h
[modify] https://crrev.com/c37cd75b3d77fb35953685c01df5adf1c744b55c/content/public/browser/android/synchronous_compositor.h
[modify] https://crrev.com/c37cd75b3d77fb35953685c01df5adf1c744b55c/content/public/browser/android/synchronous_compositor_client.h
[modify] https://crrev.com/c37cd75b3d77fb35953685c01df5adf1c744b55c/content/public/test/test_synchronous_compositor_android.cc
[modify] https://crrev.com/c37cd75b3d77fb35953685c01df5adf1c744b55c/content/public/test/test_synchronous_compositor_android.h

Comment 6 by hush@chromium.org, Oct 24 2016

Cc: -hush@chromium.org
Project Member

Comment 8 by bugdroid1@chromium.org, Oct 25 2016 

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

commit 1dd070326ef41fe0f2e7e68e0036687a90ba7f3a
Author: boliu <boliu@chromium.org>
Date: Tue Oct 25 21:22:55 2016

sync compositor: Queue up pending frame futures

Allow pending frame future to be queued up.

Alternatives considered:
* Block on previous frame future before setting new one. Android already
  does this for us, so this would only decrease parallelism without
  reducing latency.
* Signal previous frame with empty frame. This is incorrect, as the
  display compositor will then draw the previous frame, which is wrong.

BUG= 636164 

Review-Url: https://codereview.chromium.org/2449973002
Cr-Commit-Position: refs/heads/master@{#427477}

[modify] https://crrev.com/1dd070326ef41fe0f2e7e68e0036687a90ba7f3a/content/browser/android/synchronous_compositor_browser_filter.cc
[modify] https://crrev.com/1dd070326ef41fe0f2e7e68e0036687a90ba7f3a/content/browser/android/synchronous_compositor_browser_filter.h
Project Member

Comment 9 by bugdroid1@chromium.org, Oct 26 2016 

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

commit 3e3ad9a95aa746107d49fd120811e686b915e861
Author: boliu <boliu@chromium.org>
Date: Wed Oct 26 18:06:54 2016

sync compositor: Signal futures on channel shutdown

If the channel shutdown (cleanly or on error) while there are still
pending frame futures in the filter, then browser may wait indefinitely
on those futures. In this case, signal those futures with empty frames.
What frame gets drawn in these cases do not matter very much.

Also add a |filter_ready_| state so that new futures inserted gets
signaled immediately.

BUG= 636164 

Review-Url: https://codereview.chromium.org/2452893002
Cr-Commit-Position: refs/heads/master@{#427742}

[modify] https://crrev.com/3e3ad9a95aa746107d49fd120811e686b915e861/content/browser/android/synchronous_compositor_browser_filter.cc
[modify] https://crrev.com/3e3ad9a95aa746107d49fd120811e686b915e861/content/browser/android/synchronous_compositor_browser_filter.h
Project Member

Comment 11 by bugdroid1@chromium.org, Oct 28 2016 

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

commit 524d4c0b9df4f2df8da612826a4bf539d4d839b1
Author: boliu <boliu@chromium.org>
Date: Fri Oct 28 04:04:05 2016

sync compositor: Implement post metadata to UI

For the async draw path. The frame is signaled on the IO thread, but the
metadata still needs to be posted back to the UI thread.

Host registers itself with Filter on UI thread, so Filter can keep a
routing_id to Host map. Note that Host and Filter lifetimes are still
independent.

With this CL, the async path is feature complete.

BUG= 636164 

Review-Url: https://codereview.chromium.org/2456523004
Cr-Commit-Position: refs/heads/master@{#428277}

[modify] https://crrev.com/524d4c0b9df4f2df8da612826a4bf539d4d839b1/content/browser/android/synchronous_compositor_browser_filter.cc
[modify] https://crrev.com/524d4c0b9df4f2df8da612826a4bf539d4d839b1/content/browser/android/synchronous_compositor_browser_filter.h
[modify] https://crrev.com/524d4c0b9df4f2df8da612826a4bf539d4d839b1/content/browser/android/synchronous_compositor_host.cc
[modify] https://crrev.com/524d4c0b9df4f2df8da612826a4bf539d4d839b1/content/browser/android/synchronous_compositor_host.h
Project Member

Comment 12 by bugdroid1@chromium.org, Oct 31 2016 

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

commit 0be8dd04aef3283ea273061512f53a0f7df9bc69
Author: boliu <boliu@chromium.org>
Date: Mon Oct 31 16:25:42 2016

sync compositor: Keep draw after ComputeScroll synchronous

Async draw will asynchronously send scroll updates that happen between
BeginFrame (which is synchronous) and Draw to the UI thread. Although
the frame is synchronously delivered to the render thread with the frame
future.

The problem with any such scroll is that any children of webview is
scrolled according to the position of the webview will appear to be out
of sync with webview content itself.

THere are two sources of such scrolls:
1) ComputeScroll, which only happen during fling animations
2) Blink main scroll being committed/activated.

This CL fixes 1) by making draws after ComputeScroll always synchronous.
Could reduce this further by checking of webview actually has children.
But this CL is good enough to ship.

Case 2) is generally rarer. In theory, commits should not happen between
BeginFrame and Draw, but webview can't make this guarantee due to other
constraints.

BUG= 636164 

Review-Url: https://codereview.chromium.org/2457333002
Cr-Commit-Position: refs/heads/master@{#428734}

[modify] https://crrev.com/0be8dd04aef3283ea273061512f53a0f7df9bc69/content/browser/android/synchronous_compositor_host.cc
[modify] https://crrev.com/0be8dd04aef3283ea273061512f53a0f7df9bc69/content/browser/android/synchronous_compositor_host.h
Project Member

Comment 13 by bugdroid1@chromium.org, Nov 2 2016 

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

commit 6630072d85dabb1306ed6e4b850a0f9b133f848d
Author: boliu <boliu@chromium.org>
Date: Wed Nov 02 18:53:49 2016

Revert of aw: Enable async ondraw by default (patchset #1 id:1 of https://codereview.chromium.org/2457063002/ )

Reason for revert:
Probable cause for memory regression (or leak?)

BUG=661504

Original issue's description:
> aw: Enable async ondraw by default
>
> First attempt at enabling. Hopefully it sticks.
>
> BUG= 636164 
>
> Committed: https://crrev.com/28281dff0b3e8fd9702bfffa8e90e8212e53f16f
> Cr-Commit-Position: refs/heads/master@{#428460}

TBR=sgurun@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 636164 

Review-Url: https://codereview.chromium.org/2467343002
Cr-Commit-Position: refs/heads/master@{#429352}

[modify] https://crrev.com/6630072d85dabb1306ed6e4b850a0f9b133f848d/android_webview/lib/main/aw_main_delegate.cc

Comment 14 by boliu@chromium.org, Nov 4 2016

Blockedon: 661504 661933 662096
Adding some related bugs.

So turning on async onDraw lead to a chrome-tracked memory regression. And the revert lead to a perf regression, meaning enabling it lead to a perf improvement. And looks like there's also a blank page bug

Comment 16 by boliu@chromium.org, Nov 11 2016

Blockedon: 664107

Comment 17 by boliu@chromium.org, Nov 14 2016

Blockedon: 664198

Comment 18 by boliu@chromium.org, Nov 14 2016 

Looks like it's going to stick this time. Fingers crossed

After this ships to stable and the synchronous path can be removed, there's a few follow up improvements:

* the sync OnDraw to retrieve computeScroll can be optimized a little bit further. it doesn't need wait for the actual draw, and can return immediately, reduces UI waiting a little bit. actually computeScroll can use a future scheme that's waited by in draw

* we should switch all sync IPCs to use futures, and then once again ban sync IPC from browser. futures is more efficient way to build the vsync IPCs anyway, and naturally scales to more than one renderer process

Comment 19 by boliu@chromium.org, Nov 18 2016

Blockedon: 665366
Project Member

Comment 20 by bugdroid1@chromium.org, Jan 23 2017

Labels: merge-merged-2924
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ea7c6682606858dd9224faed4bc568ba75fc901b

commit ea7c6682606858dd9224faed4bc568ba75fc901b
Author: Bo Liu <boliu@chromium.org>
Date: Mon Jan 23 18:24:04 2017

[M56 fork] Disable webview async ondraw

BUG=680458,  636164 

Review-Url: https://codereview.chromium.org/2653673003 .
Cr-Commit-Position: refs/branch-heads/2924@{#842}
Cr-Branched-From: 3a87aecc31cd1ffe751dd72c04e5a96a1fc8108a-refs/heads/master@{#433059}

[modify] https://crrev.com/ea7c6682606858dd9224faed4bc568ba75fc901b/android_webview/lib/main/aw_main_delegate.cc

Comment 21 by boliu@chromium.org, Apr 6 2017

Status: Fixed (was: Assigned)
shipped in m57 \o/
►

Sign in to add a comment