Bug 70838 – [chromium] Enable threaded compositing via CCThreadProxy::hasThread only

• Component: New Bugs
• Status: RESOLVED FIXED

[Nat Duca]

[chromium] Remove enableCompositorThread field from CCSettings and derive it from CCThreadProxy::hasThread

[Nat Duca]

Created attachment 112383 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[James Robinson]

Comment on attachment 112383 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=112383&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:69
> +        // Accelerated Painting is not supported in threaded mode. Turn it off.
> +        m_settings.acceleratePainting = false;

This isn't true any more, you can remove this

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:206
> +    ASSERT(CCThreadProxy::hasThread());
> +    if (CCThreadProxy::hasThread())
> +        m_proxy->setNeedsAnimate();
> +    else
> +        m_client->scheduleComposite();

it doesn't make much sense to have an ASSERT() and then have an else clause to deal with the case where the ASSERT fails

[WebKit Review Bot]

Comment on attachment 112383 [details]
Patch

Attachment 112383 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10210337

[Nat Duca]

Created attachment 112421 [details]
Patch

[James Robinson]

Comment on attachment 112421 [details]
Patch

R=me. the WebKit/chromium/public/ warning is just for a comment, so I think you're OK submitting without waiting for Darin on this one.

[Nat Duca]

Committed r98429: <http://trac.webkit.org/changeset/98429>

[Nat Duca]

Broke the build: 
https://bugs.webkit.org/show_bug.cgi?id=70881

The problem: DRT. But also discovered other issues. Will re-attack this as time permits, lower priority than scheduling. :/

[Nat Duca]

Created attachment 115900 [details]
Patch

[Nat Duca]

My CCMainThread->CCThread integration caused some problems with the way Aura initializes the compositor. I have modified this patch to make init and shutdown of the compositor explicit, which makes things not just cleaner than before [in my estimation] but also makes it clear when the compositor should be available.

This is paired with http://codereview.chromium.org/8558030/ now

[Darin Fisher (:fishd, Google)]

Comment on attachment 115900 [details]
Patch

WebKit API changes LGTM

[James Robinson]

Comment on attachment 115900 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=115900&action=review

Mostly looks good but the shutdown() semantics seem a little scary. I think that needs to be resolved and documented clearly in WebCompositor.h

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:199
> +    if (CCProxy::implThread())

a lot of code in this patch null-checks CCProxy::implThread() to tell if we're in threaded mode or not. should we provide a bool getter with a friendlier name?

> Source/WebCore/platform/graphics/chromium/cc/CCProxy.cpp:70
> +    return !mainThreadIsImplThread && currentThread() == s_mainThread->threadID();

this looks weird but maybe it's just because of the variable name. "mainThreadIsImplThread" makes me think that when that flag is set we should only return true if the current thread is the impl thread. This seems to be doing something more permissive.

maybe the bool should be "currentThreadIsImplThread" ?

> Source/WebCore/platform/graphics/chromium/cc/CCProxy.cpp:76
> +    return mainThreadIsImplThread || currentThread() == implThreadID;

same here - from the variable name, i'd think that if the bool is true this should check that the current thread == the main thread

> Source/WebKit/chromium/public/WebCompositor.h:45
> +    // compositing, pass a WebThread. To use single-threaded compositing, pass
> +    // 0.

line wrapping a bit awkward - can you move the '0' up to the previous line at least?

> Source/WebKit/chromium/public/WebCompositor.h:48
> +    WEBKIT_EXPORT static void shutdown();

the requirements and conditions on this function should be documented here - i.e. it looks like you can't call this with active compositor instances and it's not valid to call any functions on this interface after shutdown() depending on what the initialize/shutdown requirements are

> Source/WebKit/chromium/src/WebCompositorImpl.cpp:31
> +#include "WebKit.h"

this #include doesn't appear to be used

> Source/WebKit/chromium/src/WebCompositorImpl.cpp:42
> +bool webCompositorInitialized = false;

can we make this a static member s_initialized? we're doing that for WebComopsitorImpl::s_nextAvailableIndentifier and WebCompositorImpl::s_compositors so it seems nice to be consistent

> Source/WebKit/chromium/src/WebCompositorImpl.cpp:59
> +    if (webCompositorInitialized)
> +        ASSERT_NOT_REACHED();

ASSERT(!webCompositorInitialized) ?

> Source/WebKit/chromium/src/WebCompositorImpl.cpp:62
> +    CCProxy::setMainThread(CCThreadImpl::create(webKitPlatformSupport()->currentThread()).leakPtr());

seems weird to .leakPtr() this here, and then make an explicit delete call in shutdown(). Can we hang on to this pointer explicitly in a static with some comments on the lifetime?

> Source/WebKit/chromium/src/WebCompositorImpl.cpp:64
> +        CCProxy::setImplThread(CCThreadImpl::create(implThread).leakPtr());

we've been leaking this object since it's pretty small and only exists once per process, but if we're going to delete the mainthread CCThreadImpl it seems to make sense to treat this one the same way.

> Source/WebKit/chromium/src/WebCompositorImpl.cpp:69
> +void WebCompositor::shutdown()

could you add an ASSERT() that the s_compositors map is empty (i.e. enforce that you can't call this with active compositor instances) ?

> Source/WebKit/chromium/src/WebCompositorImpl.cpp:74
> +    webCompositorInitialized = false;

does this mean we now support shutting down and reinitializing in the same address space? WebKit in general does not support this operation, and I'm not sure that our compositor does either

Unless this is really necessary I'd prefer to not support this and leave the initialized flag alone so another call to initialize() will ASSERT(). Alternately if we do need to support this we need some tests to make sure it works and doesn't regress.

> Source/WebKit/chromium/src/WebLayerTreeView.cpp:33
> +#include "cc/CCThreadProxy.h"

this looks unused

> Source/WebKit/chromium/src/WebLayerTreeViewImpl.cpp:96
> +    GraphicsContext3DPrivate::ThreadUsage usage = CCThreadProxy::implThread() ? GraphicsContext3DPrivate::ForUseOnAnotherThread : GraphicsContext3DPrivate::ForUseOnThisThread;

shouldn't this be CCProxy::implThread() ?

> Source/WebKit/chromium/src/WebViewImpl.cpp:137
> +#include "cc/CCThreadProxy.h"

I think this should be CCProxy.h, since it's pulling in a header to check CCProxy::implThread()

[Nat Duca]

(In reply to comment #12)
> could you add an ASSERT() that the s_compositors map is empty (i.e. enforce that you can't call this with active compositor instances) ?
That's not threadsafe. I did something similar though -- checking that there are no CCLTH's is safe.

> > Source/WebKit/chromium/src/WebCompositorImpl.cpp:74
> does this mean we now support shutting down and reinitializing in the same address space? WebKit in general does not support this operation, and I'm not sure that our compositor does either
So we actually already do this in the CCLayerTreeHostProxy. A lot of my refactoring of CCMainThread/CCThread has enabled this. We should be able to do this, and I think it is good software engineering discipline to encourage us to do so. The compositor feature is emphatically not webkit -- its a small library and if you can't turn it on and off, something is screwed up with our implementation.

> Unless this is really necessary I'd prefer to not support this and leave the initialized flag alone so another call to initialize() will ASSERT(). Alternately if we do need to support this we need some tests to make sure it works and doesn't regress.
Disagree. And, the CCLayerTreeHost tests test this.

[Nat Duca]

Also, WebKit.h is needed to obtain webPlatformSupport()

[Nat Duca]

Created attachment 115944 [details]
Patch

[WebKit Review Bot]

Comment on attachment 115944 [details]
Patch

Attachment 115944 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10525043

[Nat Duca]

Created attachment 115946 [details]
Patch

[Nat Duca]

So, in adding asserts to shutdown, I found an issue with the opacity test and our test harness.

The core issue is that opacity test changes the root layer. The shutdown code was hard coded to assume that the root layer was not changed. When it was, the clean up code we had to break the circular reference between layers & treeHost would fail to break the reference and the CCLTH would stay alive.

Fixing this proved tricky because some tests expect endTest to "destroy" the CCLayerTreeHost in order to test recovery. Other tests dont particularly care when the layerTreeHost dies.

I re-did things so that endTest just ends the test. Tests that want the layerTreeHost to die with messages in flight need to orchestrate that death explicitly.

In doing this I also discovered that the continuous redrawing test was set up to just exit right after starting. I.e. not doing anything at all. :'(

[WebKit Review Bot]

Comment on attachment 115946 [details]
Patch

Attachment 115946 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10515450

[James Robinson]

(In reply to comment #13)
> (In reply to comment #12)
> > could you add an ASSERT() that the s_compositors map is empty (i.e. enforce that you can't call this with active compositor instances) ?
> That's not threadsafe. I did something similar though -- checking that there are no CCLTH's is safe.
> 

Interesting - what are the threading requirements for the shutdown() call then?  I assumed it was the same as the initialize and fromIdentifier() (has to be called from the compositor thread, whatever that is) so checking the s_compositor map should be fine.  If this can be called from other threads then it makes things a bit more interesting but still possible.  Earlier versions of this code had a mutex guarding s_compositor.

From the header docs I would assume shutdown() has to be called from the compositor thread.

[Nat Duca]

(In reply to comment #20)
> Interesting - what are the threading requirements for the shutdown() call then?  I assumed it was the same as the initialize and fromIdentifier() (has to be called from the compositor thread, whatever that is) so checking the s_compositor map should be fine.  If this can be called from other threads then it makes things a bit more interesting but still possible.  Earlier versions of this code had a mutex guarding s_compositor.
> 
> From the header docs I would assume shutdown() has to be called from the compositor thread.

Ah, the idea was this is a main thread only concept. This is building on the fact that, on ToT, SetImplThread was a main thread function [not documented to be so, however].

To me, WebCompositor contained, from an API point of view, our global interfaces for the compositor. Perhaps the disconnect here is that your mental model is that WebCompositor is some impl-thread concept?

I'm fine tweaking API / doc / impl once we settle on something.

[James Robinson]

Yeah, I think this is a mental model mismatch.

In my head WebCompositor is the interface that the embedder can use to communicate directly with the compositor's impl thread.  The initialization routine is used to set the impl thread, fromIdentifier() is used to grab an interface to the impl thread, and the instance functions are used to poke at the thread.  The normal WebLayer... interfaces are used to communicate with the 'main thread' data structures (when using that API).

If a separate compositor thread is not being used then I had envisioned that the WebCompositor interface wouldn't be used at all.

Perhaps we need to split out the impl thread interface from common initialization/shutdown routines?  Then the API would be in three parts:

1.) General compositor initialization and shutdown. Apply to all compositor instances in the process and both threaded and single threaded uses.
2.) Per-compositor instance APIs - WebLayer, etc. Always use the 'main thread', whatever that means for a given configuration.
2.) Per-compositor instance impl-thread specific APIs, like input handling.

Does that make sense?

[Nat Duca]

Recapping our discussion, here's where I think we'll try to end up:

> The API would be in three parts:
> 1.) General compositor initialization and shutdown. Apply to all compositor instances in the process and both threaded and single threaded uses

WebCompositor

> 2.) Per-compositor instance APIs - WebLayer, etc. Always use the 'main thread', whatever that means for a given configuration.
WebLayer*, WebLayerTreeView
CCLayerTreeHost gets renamed to CCLayerTreeView


> 2.) Per-compositor instance impl-thread specific APIs, like input handling.
The aspects of WebCompositor that do this move out to WebCompositorInputHandler

[James Robinson]

*thumbsup*

[Darin Fisher (:fishd, Google)]

Comment on attachment 115946 [details]
Patch

WebKit API LGTM + jamesr: "thumbs up" == R+

[Darin Fisher (:fishd, Google)]

Comment on attachment 115946 [details]
Patch

whoops, /me misunderstood the thumbsup

[James Robinson]

Comment on attachment 115946 [details]
Patch

Sorry, that was thumbs up on the plan. I think that plan implies some changes to this patch - set r? if you disagree, Nat.

[Nat Duca]

Trying to pull together a new patch that is worthy of thumbs. ;)

[Nat Duca]

Created attachment 116178 [details]
Patch

[WebKit Review Bot]

Comment on attachment 116178 [details]
Patch

Attachment 116178 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10551100

[James Robinson]

Comment on attachment 116178 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116178&action=review

Require additional pylons! I mean comments!

R=me but please go through comments carefully before landing.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:42
> +namespace {
> +static int numLayerTreeInstances;
> +}

anonymous namespace and static is redundant, normally we just do one or the other.

could (should) this be #ifndef NDEBUG or ASSERT_ENABLED (or whatever it's called)? I think it'd be pretty bad if people started basing logic beyond ASSERT()s on this mechanism

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:112
> +    // Returns true if any CCLayerTreeHost is alive.
> +    static bool anyLayerTreeHostInstanceExists();
> +

#ifndef NDEBUG? I don't think this is normal 'public' API that we want to support outside of checking things

> Source/WebKit/chromium/public/WebCompositor.h:31
> +#include "WebCompositorInputHandler.h"
>  namespace WebKit {

newline between #include and namespace declaration

> Source/WebKit/chromium/public/WebCompositor.h:33
>  class WebInputEvent;

pretty sure you don't need this forward decl any more

> Source/WebKit/chromium/public/WebCompositor.h:41
> +// All calls to the WebCompositor must be made from the main thread.

'main thread' is ill defined here. does that mean the WebKit main thread? I think not for browser compositing.  I think at the API level this means that all calls must be made from the same thread that the WebLayer* APIs are used on, right?

> Source/WebKit/chromium/public/WebCompositor.h:48
> +    // Initializes the compositor. Threaded compositing is enabled by passing in
> +    // a non-null WebThread. No compositor classes or methods should be used

second sentence is a little awkward since it uses the passive voice. how about:

Pass a non-null WebThread* to enable threaded compositing, otherwise the compositor will run on the thread the WebLayer APIs are used on.

or something like that

> Source/WebKit/chromium/public/WebCompositorInputHandlerClient.h:33
> +    // Callbacks invoked from the compositor thread.

can you move this do be a class-level comment?

> Source/WebKit/chromium/public/WebCompositorInputHandlerClient.h:36
> +    // Exactly one of the following two callbacks will be invoked after every call to WebCompositor::handleInputEvent():

update comment for new class name

> Source/WebKit/chromium/public/WebCompositorInputHandlerClient.h:38
> +    // Called when the WebCompositor handled the input event and no further processing is required.

update comment for new class name

> Source/WebKit/chromium/public/WebCompositorInputHandlerClient.h:41
> +    // Called when the WebCompositor did not handle the input event. If sendToWidget is true, the input event

update comment for new class name

> Source/WebKit/chromium/src/WebCompositorImpl.cpp:43
> +OwnPtr<CCThread> WebCompositorImpl::s_mainThread;
> +OwnPtr<CCThread> WebCompositorImpl::s_implThread;

i know i asked you to make these OwnPtr<> but now that I think about it doing this will create static destructors, which we generally try to avoid. I think raw ptrs would probably be better - if we shut down the process without calling WebCompositor::shutdown() then we don't want to attempt to destroy these

> Source/WebKit/chromium/src/WebCompositorImpl.cpp:58
> +/*WebCompositor* WebCompositor::initialize(WebThread* implThread)
>  {
> -    ASSERT(CCProxy::isImplThread());
>  
> -    if (!s_compositors)
> -        s_compositors = new HashSet<WebCompositorImpl*>;
> -    s_compositors->add(this);
>  }
> -
> -WebCompositorImpl::~WebCompositorImpl()
> +*/

please don't check in commented-out code. if it's not needed, just kill it with fire

> Source/WebKit/chromium/src/WebCompositorImpl.cpp:86
> +    s_implThread.clear();
> +    s_mainThread.clear();
> +    CCProxy::setImplThread(0);
> +    CCProxy::setMainThread(0);

nit: would it be safer to reverse the order of these calls, just in case CCProxy::... wanted to do something with the pointer?

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:1013
> +        // clear m_updateCheckLayer so CCLayerTreeHost dies.

Capitalize 'clear' please

[James Robinson]

Oh, and it's probably a good idea to make it compile too.

[Nat Duca]

Committed r101440: <http://trac.webkit.org/changeset/101440>

[Nat Duca]

Created attachment 117274 [details]
Patch for landing

[Nat Duca]

Committed r101576: <http://trac.webkit.org/changeset/101576>