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

Issue 609977 link

Starred by 2 users
Status: Fixed
Owner:
boliu@chromium.org
Closed: Aug 2016
Cc:
tobiasjs@chromium.org
█ hush@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug



Sign in to add a comment

Untangle synchronous compositor renderer objects

Project Member Reported by boliu@chromium.org, May 6 2016 
Existing sync compositor objects:

SynchronousCompositorOutputSurface -- does not know IPC, talks to a client interface

SynchronousCompositorExternalBeginFrameSource -- does not know IPC, talks to a client interface

SynchronousCompositorProxy -- lives on compositor thread. Implements OSClient and BFSClient. Lifetime implicitly owned by both OS and BFS. Handles IPC. Has shared state for both OS and BFS.

SynchronousCompositorFilter - global, straddles IO and compositor thread, filters SyncCompositor messages and deliver them to Proxy. Also responsible for connecting OS and BFS and creating Proxy for them, with the SynchronousCompositorRegistry interface.


It was set up this way because we wanted to share OS and BFS implementation with the in-process path that do not involve IPCs. But now the in-process path no longer exists. Remove OSClient, BFSClient, Proxy, and Registry. OS and BFS can handle their own IPC and talk directly to the filter. OS and BFS should be completely independent, and the shared state currently living in Proxy should move to OS and BFS.

Comment 1 by boliu@chromium.org, May 6 2016 

Filter might have to talk to InputHandler and SynchronousInputHandlerProxy directly, which is not nice. Maybe this part should be refactored separately to be sane.

Comment 2 by boliu@chromium.org, May 6 2016 

Also.. I should stop worrying about reducing the number of async IPCs. Right now we try to reduce number of IPCs by making each IPC include all states. This won't work after the refactor since IPC is only received by one of OS/BFS/InputHandler, and they don't know/care about all of the states.

If latency for sync IPCs get worse as a result of increased number of IPCs, then the correct solution is to give priority to sync IPC tasks on the renderer side. I don't think that's gonna happen though.

Comment 3 by boliu@chromium.org, May 6 2016 

Hmm, wonder how much code can be shared with chrome afterwards. There isn't that many "synchronous" messages. Everything else should just behave the same way as chrome. Should keep an eye out for that during refactor.

Comment 4 by boliu@chromium.org, May 9 2016 

Writing down more thoughts on IPC.

In the spirit of making things look like chrome, we should make vsync async as well. Of course still need synchronously update scroll offset etc inside the vsync call. I'm thinking of adding a webview-only sync IPC that's only responsible for synchronously *retrieving* renderer state like scroll offset. So the pattern should be any time we need synchronous behavior, we send that IPC following a bunch of async IPCs, and the sync IPC would serve as both a flush (actually more like finish) call, and retrieve the final state from renderer side.

If would be ideal if this is a "global" sync IPC that retrieved state of *all* webviews rather than just a single one, but that requires more work ( crbug.com/596235 ). So start with per-webview I guess.

SyncCompositorCommonBrowserParams should be completely be removed. But SyncCompositorCommonRendererParams needs to stay in some form, including the versioning logic.
Project Member

Comment 5 by bugdroid1@chromium.org, May 10 2016 

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

commit ec72a2edffebb31179ed7b1812ecb94509f8adfe
Author: boliu <boliu@chromium.org>
Date: Tue May 10 00:51:56 2016

sync compostor: Return resources async IPC

Small step toward overall refactor outlined in bug. Make returning
resources an plain old async IPC that's sent whenever there are return
resources, and remove it from CommonBrowserParams that's sent in every
IPC.

On the renderer side, add OnMessageReceived to OutputSurface, and have
OuputSurface get the first chance of handling IPCs before Proxy.
Eventual aim is Proxy should not handle any IPC and should just be
removed.

BUG= 609977 

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

[modify] https://crrev.com/ec72a2edffebb31179ed7b1812ecb94509f8adfe/content/browser/android/synchronous_compositor_host.cc
[modify] https://crrev.com/ec72a2edffebb31179ed7b1812ecb94509f8adfe/content/browser/android/synchronous_compositor_host.h
[modify] https://crrev.com/ec72a2edffebb31179ed7b1812ecb94509f8adfe/content/common/android/sync_compositor_messages.cc
[modify] https://crrev.com/ec72a2edffebb31179ed7b1812ecb94509f8adfe/content/common/android/sync_compositor_messages.h
[modify] https://crrev.com/ec72a2edffebb31179ed7b1812ecb94509f8adfe/content/renderer/android/synchronous_compositor_output_surface.cc
[modify] https://crrev.com/ec72a2edffebb31179ed7b1812ecb94509f8adfe/content/renderer/android/synchronous_compositor_output_surface.h
[modify] https://crrev.com/ec72a2edffebb31179ed7b1812ecb94509f8adfe/content/renderer/android/synchronous_compositor_proxy.cc
Project Member

Comment 6 by bugdroid1@chromium.org, May 12 2016 

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

commit 42e01317f769bd3e40e67874ef4295fd0f782df7
Author: boliu <boliu@chromium.org>
Date: Thu May 12 21:47:52 2016

sync compositor: Memory policy async IPC

Move bytes_limit to an independent async IPC.

Note to handle updating the state after a OutputSurface
loss and recreation, also add a renderer->browser ipc to
notify browser that new output surface is created, and
thus need to have all browser state resent.

BUG= 609977 

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

[modify] https://crrev.com/42e01317f769bd3e40e67874ef4295fd0f782df7/content/browser/android/synchronous_compositor_host.cc
[modify] https://crrev.com/42e01317f769bd3e40e67874ef4295fd0f782df7/content/browser/android/synchronous_compositor_host.h
[modify] https://crrev.com/42e01317f769bd3e40e67874ef4295fd0f782df7/content/common/android/sync_compositor_messages.cc
[modify] https://crrev.com/42e01317f769bd3e40e67874ef4295fd0f782df7/content/common/android/sync_compositor_messages.h
[modify] https://crrev.com/42e01317f769bd3e40e67874ef4295fd0f782df7/content/renderer/android/synchronous_compositor_output_surface.cc
[modify] https://crrev.com/42e01317f769bd3e40e67874ef4295fd0f782df7/content/renderer/android/synchronous_compositor_output_surface.h
[modify] https://crrev.com/42e01317f769bd3e40e67874ef4295fd0f782df7/content/renderer/android/synchronous_compositor_proxy.cc
[modify] https://crrev.com/42e01317f769bd3e40e67874ef4295fd0f782df7/content/renderer/android/synchronous_compositor_proxy.h

Comment 7 by boliu@chromium.org, May 12 2016 

Hmm, getting rid of SynchronousCompositorProxy might not be possible.

Already stated above, SyncCompositorCommonRendererParams needs to stay. And will need a sync IPC to retrieve all of this state. In general, this state is critical to keeping webview's synchronous behavior.

But this state belongs to independent objects on the renderer side: output surface, begin frame source, input handler. So to handle the synchronous retrieve IPC, renderer side has to talk to all of these objects. Right now Proxy is the the thing that's aware of all of these objects. So something like the Proxy has to exist.

Even worse, if a state (like need_invalidate_count) is sent with CommonParams, it means it can't be turned into an independent (and async) ipc from renderer. Because there is no easy way to do versioning, unless we want to version each state separately, which is just overkill, so can't get rid of client interfaces either.

The only silver lining is that I *think* need_begin_frame can actually be removed from SyncCompositorCommonRendererParams. Input is already async, so we already can't get back need_begin_frame from the renderer for the current frame. (Fwiw chrome has the same problem, and sends a "proactive" begin frame if a frame has input, regardless of whether renderer actually wants it or not; webview inherited the same logic so we should be fine.) So at least we should be able to remove our own BeginFrameSource implementation. Yay?

Comment 8 by boliu@chromium.org, May 13 2016 

afaik ipc (and mojo) doesn't separate "send" and "wait for reply" for sync IPCs, probably for good reason, but I can think of a few minor optimizations where it could be useful..
Project Member

Comment 11 by bugdroid1@chromium.org, Jun 2 2016 

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

commit 875afa8e5cc793f01c981472d257b33be860ecc6
Author: boliu <boliu@chromium.org>
Date: Thu Jun 02 05:56:52 2016

Add begin frame paused to android

The pause signal is added to fix  crbug.com/539373  which was
a deadlock issue in webview, but could theoretically happen
in chrome as well. It's basically the visibility signal but
sent directly to the compositor thread without going
through the blink main thread.

Add this to Android's BeginFrameSource as well, so that
webview can share the same BeginFrameSource implementation.

BUG= 609977 

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

[modify] https://crrev.com/875afa8e5cc793f01c981472d257b33be860ecc6/content/browser/renderer_host/render_widget_host_view_android.cc
[modify] https://crrev.com/875afa8e5cc793f01c981472d257b33be860ecc6/content/common/view_messages.h
[modify] https://crrev.com/875afa8e5cc793f01c981472d257b33be860ecc6/content/renderer/gpu/compositor_external_begin_frame_source.cc
[modify] https://crrev.com/875afa8e5cc793f01c981472d257b33be860ecc6/content/renderer/gpu/compositor_forwarding_message_filter.cc
Project Member

Comment 12 by bugdroid1@chromium.org, Jun 4 2016 

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

commit 8f3016ee733ac3e4ccd65e0711f5d3a9b92e5e49
Author: boliu <boliu@chromium.org>
Date: Sat Jun 04 00:59:18 2016

sync compositor: Remove begin frame source

Share the same BeginFrameSource implementation with chrome.

The old synchronous begin frame IPC is split into two parts:
an async begin frame IPC (shared with rest of chrome),
and a sync IPC to synchronously retrieve the renderer state.

Note needs_begin_frame is no longer part of the synchronous
state retrieved from renderer. This is ok because input
is already async and that's the only time when having an
up-to-date needs_begin_frame really matters.

This removes the whole synchronous compositor "IsActive"
concept since it's superceded completely by visibility.

BUG= 609977 

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

[modify] https://crrev.com/8f3016ee733ac3e4ccd65e0711f5d3a9b92e5e49/android_webview/browser/browser_view_renderer.cc
[modify] https://crrev.com/8f3016ee733ac3e4ccd65e0711f5d3a9b92e5e49/android_webview/browser/browser_view_renderer.h
[modify] https://crrev.com/8f3016ee733ac3e4ccd65e0711f5d3a9b92e5e49/content/browser/android/synchronous_compositor_host.cc
[modify] https://crrev.com/8f3016ee733ac3e4ccd65e0711f5d3a9b92e5e49/content/browser/android/synchronous_compositor_host.h
[modify] https://crrev.com/8f3016ee733ac3e4ccd65e0711f5d3a9b92e5e49/content/browser/renderer_host/render_widget_host_view_android.cc
[modify] https://crrev.com/8f3016ee733ac3e4ccd65e0711f5d3a9b92e5e49/content/common/android/sync_compositor_messages.cc
[modify] https://crrev.com/8f3016ee733ac3e4ccd65e0711f5d3a9b92e5e49/content/common/android/sync_compositor_messages.h
[modify] https://crrev.com/8f3016ee733ac3e4ccd65e0711f5d3a9b92e5e49/content/content_renderer.gypi
[modify] https://crrev.com/8f3016ee733ac3e4ccd65e0711f5d3a9b92e5e49/content/public/browser/android/synchronous_compositor.h
[modify] https://crrev.com/8f3016ee733ac3e4ccd65e0711f5d3a9b92e5e49/content/public/test/test_synchronous_compositor_android.h
[delete] https://crrev.com/6ac5be6880e84f4be61d7d8e63b1ff8283f7735c/content/renderer/android/synchronous_compositor_external_begin_frame_source.cc
[delete] https://crrev.com/6ac5be6880e84f4be61d7d8e63b1ff8283f7735c/content/renderer/android/synchronous_compositor_external_begin_frame_source.h
[modify] https://crrev.com/8f3016ee733ac3e4ccd65e0711f5d3a9b92e5e49/content/renderer/android/synchronous_compositor_filter.cc
[modify] https://crrev.com/8f3016ee733ac3e4ccd65e0711f5d3a9b92e5e49/content/renderer/android/synchronous_compositor_filter.h
[modify] https://crrev.com/8f3016ee733ac3e4ccd65e0711f5d3a9b92e5e49/content/renderer/android/synchronous_compositor_output_surface.cc
[modify] https://crrev.com/8f3016ee733ac3e4ccd65e0711f5d3a9b92e5e49/content/renderer/android/synchronous_compositor_proxy.cc
[modify] https://crrev.com/8f3016ee733ac3e4ccd65e0711f5d3a9b92e5e49/content/renderer/android/synchronous_compositor_proxy.h
[modify] https://crrev.com/8f3016ee733ac3e4ccd65e0711f5d3a9b92e5e49/content/renderer/android/synchronous_compositor_registry.h
[modify] https://crrev.com/8f3016ee733ac3e4ccd65e0711f5d3a9b92e5e49/content/renderer/render_thread_impl.cc

Comment 13 by boliu@chromium.org, Aug 29 2016

Status: Fixed (was: Assigned)

Comment 14 by ntfschr@chromium.org, Aug 23

Labels: WebView-Disabled-Test
►

Sign in to add a comment