[Chromium] Separate compositor client thread from webkit's main thread

• Component: New Bugs
• Status: RESOLVED FIXED

[Antoine Labour]

Separate compositor platform support from webkit's platform support

[Antoine Labour]

Created attachment 109105 [details]
Patch

[WebKit Review Bot]

Attachment 109105 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/graphics/chromium/CompositorSupport.h:34:  The parameter name "id" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/chromium/CompositorSupport.h:35:  The parameter name "id" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/chromium/CompositorSupport.h:48:  The parameter name "id" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/chromium/CompositorSupport.h:49:  The parameter name "id" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/chromium/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 6 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antoine Labour]

Created attachment 109109 [details]
Patch

[Antoine Labour]

Created attachment 109111 [details]
Patch

[WebKit Review Bot]

Attachment 109109 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/graphics/chromium/CompositorSupport.h:34:  The parameter name "id" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/chromium/CompositorSupport.h:35:  The parameter name "id" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/chromium/CompositorSupport.h:48:  The parameter name "id" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/chromium/CompositorSupport.h:49:  The parameter name "id" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 4 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 109111 [details]
Patch

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

[James Robinson]

Comment on attachment 109111 [details]
Patch

CCProxy::setMainThread is debug-only, so you'll have to add #ifndef NDEBUGs in CompositorSupport::initialize() / CompositorSupport::shutdown().

Otherwise I think this looks pretty reasonable.  I do wonder about the combination of need v8 / need compositor - feels like we're growing a bit oddly there.  Also, don't we initialize WebKit with v8 on in the browser process in order to do PAC resolution?

[James Robinson]

+cc Darin for WebKit API

[Antoine Labour]

Created attachment 109116 [details]
Patch

[Antoine Labour]

(In reply to comment #7)
> (From update of attachment 109111 [details])
> CCProxy::setMainThread is debug-only, so you'll have to add #ifndef NDEBUGs in CompositorSupport::initialize() / CompositorSupport::shutdown().

Done. My bad.

> Otherwise I think this looks pretty reasonable.  I do wonder about the combination of need v8 / need compositor - feels like we're growing a bit oddly there.  Also, don't we initialize WebKit with v8 on in the browser process in order to do PAC resolution?

Today, AFAICT we initialize it without V8, see http://codesearch.google.com/#OAMlx_jo-ck/src/content/browser/in_process_webkit/webkit_thread.cc&exact_package=chromium&q=WebKit::initialize&type=cs
I think that's the raison d'être of this call.
I'm absolutely not attached to the "initializeWithoutV8AndCompositor" concept. I didn't want to "sneak in" the compositor initialization. My plan was to update the browser-side webkit initialization to use initializeWithoutV8AndCompositor and then remove the initializeWithoutV8 call altogether.
I do agree that cherry-picking what we initialize and encoding that in the name is a bit clunky. We could call it "initializeForBrowser" or something...
One option is to have a bitfield of webkit sub-modules that we want to initialize, though I suspect that's going to add a bunch of untested (and most likely not working) combinations.
Any good idea?

[Darin Fisher (:fishd, Google)]

Comment on attachment 109116 [details]
Patch

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

> Source/WebKit/chromium/public/WebKit.h:58
> +WEBKIT_EXPORT void initializeWithoutV8AndCompositor(WebKitPlatformSupport*);

lol, when we just recently added initializeWithoutV8, there was a debate about
whether or not we should just add flags to initialize().  I argued that since
we only had the use case of running without V8, that we should just go for the
simpler form.

I'm pretty sure that initializeWithoutV8AndCompositor is getting out of hand
though ;-)

This might be better:

  struct WebKitInitializationOptions {
      bool initializeV8;
      bool initializeCompositor;
      // ... default ctor sets them to true ...
  };
  WEBKIT_EXPORT void initializeWithOptions(WebKitPlatformSupport*, const WebKitInitializationOptions&);

> Source/WebKit/chromium/src/WebKit.cpp:61
> +class WebKitCompositorSupport : public WebCore::CompositorSupportDelegate {

I'm guessing there is some next patch that will make it possible for there to be
a CompositorSupportDelegate that doesn't just thunk to WebKitPlatformSupport?
Can you explain how that might look?

Also, it seems like the 4 functions on this interface could be defined as safe to
call from any thread in WebKit.  I'm not quite sure why they need to be factored
out like this.  It may already be the case that they are safe to call from background
threads.  I think webworkers can query the current time, and the trace and histogram
systems are threadsafe.  Am I missing something?

[Antoine Labour]

(In reply to comment #11)
> (From update of attachment 109116 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=109116&action=review
> 
> > Source/WebKit/chromium/public/WebKit.h:58
> > +WEBKIT_EXPORT void initializeWithoutV8AndCompositor(WebKitPlatformSupport*);
> 
> lol, when we just recently added initializeWithoutV8, there was a debate about
> whether or not we should just add flags to initialize().  I argued that since
> we only had the use case of running without V8, that we should just go for the
> simpler form.
> 
> I'm pretty sure that initializeWithoutV8AndCompositor is getting out of hand
> though ;-)
> 
> This might be better:
> 
>   struct WebKitInitializationOptions {
>       bool initializeV8;
>       bool initializeCompositor;
>       // ... default ctor sets them to true ...
>   };
>   WEBKIT_EXPORT void initializeWithOptions(WebKitPlatformSupport*, const WebKitInitializationOptions&);

That works for me.
 
> > Source/WebKit/chromium/src/WebKit.cpp:61
> > +class WebKitCompositorSupport : public WebCore::CompositorSupportDelegate {
> 
> I'm guessing there is some next patch that will make it possible for there to be
> a CompositorSupportDelegate that doesn't just thunk to WebKitPlatformSupport?

Right, it's part of the compositor wenkit API, I'll upload it as soon as I figure out how to get webkit-patch to use the right base commit.

> Can you explain how that might look?

Nothing fancy, a separate compositor initialization function that takes a WebCompositorPlatformSupport that looks just like CompositorSupportDelegate.

> 
> Also, it seems like the 4 functions on this interface could be defined as safe to
> call from any thread in WebKit.  I'm not quite sure why they need to be factored
> out like this.  It may already be the case that they are safe to call from background
> threads.  I think webworkers can query the current time, and the trace and histogram
> systems are threadsafe.  Am I missing something?

There's a few reasons:
1- the functions themselves may be thread-safe (I'd have to check), but the way to get to them doesn't look like it is (s_webKitPlatformSupport isn't protected by any lock).
2- when we want to extract the compositor out of WebKit, we'll need those anyway. This is part of this effort.
3- it makes it harder to add dependencies to the rest of webcore, including calling functions that wouldn't be thread safe.

Note: in the browser process, the compositor will be called from a non-webkit thread, if that makes things clearer...

[Darin Fisher (:fishd, Google)]

(In reply to comment #12)
> (In reply to comment #11)
> > (From update of attachment 109116 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=109116&action=review
> > 
> > > Source/WebKit/chromium/public/WebKit.h:58
> > > +WEBKIT_EXPORT void initializeWithoutV8AndCompositor(WebKitPlatformSupport*);
> > 
> > lol, when we just recently added initializeWithoutV8, there was a debate about
> > whether or not we should just add flags to initialize().  I argued that since
> > we only had the use case of running without V8, that we should just go for the
> > simpler form.
> > 
> > I'm pretty sure that initializeWithoutV8AndCompositor is getting out of hand
> > though ;-)
> > 
> > This might be better:
> > 
> >   struct WebKitInitializationOptions {
> >       bool initializeV8;
> >       bool initializeCompositor;
> >       // ... default ctor sets them to true ...
> >   };
> >   WEBKIT_EXPORT void initializeWithOptions(WebKitPlatformSupport*, const WebKitInitializationOptions&);
> 
> That works for me.
> 
> > > Source/WebKit/chromium/src/WebKit.cpp:61
> > > +class WebKitCompositorSupport : public WebCore::CompositorSupportDelegate {
> > 
> > I'm guessing there is some next patch that will make it possible for there to be
> > a CompositorSupportDelegate that doesn't just thunk to WebKitPlatformSupport?
> 
> Right, it's part of the compositor wenkit API, I'll upload it as soon as I figure out how to get webkit-patch to use the right base commit.
> 
> > Can you explain how that might look?
> 
> Nothing fancy, a separate compositor initialization function that takes a WebCompositorPlatformSupport that looks just like CompositorSupportDelegate.
> 
> > 
> > Also, it seems like the 4 functions on this interface could be defined as safe to
> > call from any thread in WebKit.  I'm not quite sure why they need to be factored
> > out like this.  It may already be the case that they are safe to call from background
> > threads.  I think webworkers can query the current time, and the trace and histogram
> > systems are threadsafe.  Am I missing something?
> 
> There's a few reasons:
> 1- the functions themselves may be thread-safe (I'd have to check), but the way to get to them doesn't look like it is (s_webKitPlatformSupport isn't protected by any lock).

WebKit has to be initialized before s_webKitPlatformSupport is used, and we should ensure that any background threads are gone before WebKit::shutdown returns.


> 2- when we want to extract the compositor out of WebKit, we'll need those anyway. This is part of this effort.

I see.  It seems like we could have WebCompositorPlatformSupport be a getter on WebKitPlatformSupport.  That would be the much more natural way to handle this kind of thing.  If you want to get that inside WebKit::initialize(), then you can certainly avoid any concerns about thread safety.


> 3- it makes it harder to add dependencies to the rest of webcore, including calling functions that wouldn't be thread safe.

I'm not sure I understand this issue.  Can you explain more?


> Note: in the browser process, the compositor will be called from a non-webkit thread, if that makes things clearer...

I'm concerned about calling more WebKit code in the browser process.  We are actually trying to call less of it.  Maybe the extraction of this code into a separate library should happen sooner?

[James Robinson]

(In reply to comment #11)
> This might be better:
> 
>   struct WebKitInitializationOptions {
>       bool initializeV8;
>       bool initializeCompositor;
>       // ... default ctor sets them to true ...
>   };
>   WEBKIT_EXPORT void initializeWithOptions(WebKitPlatformSupport*, const WebKitInitializationOptions&);
> 

The initialization for v8 and for the compositor isn't really initializing anything - it's more binding gloop together so that it'll work together.  We use V8 in the browser process but still call initializeWithoutV8() cos we don't bind V8 and WebKit together in the browser.

One issue with this sort of API is that it seems to imply that a caller can reasonably set any combination and expect it to work.  In practice we only have two configs that are important - the one the renderer uses (webkit should initialize, bind to v8 and bind to compositor) and the one the browser and utility process (webkit should initialize but not bind to either).

Maybe we need an initialize(), bindToV8(), bindToCompositor()?

[James Robinson]

(In reply to comment #14)
> (In reply to comment #11)
> > This might be better:
> > 
> >   struct WebKitInitializationOptions {
> >       bool initializeV8;
> >       bool initializeCompositor;
> >       // ... default ctor sets them to true ...
> >   };
> >   WEBKIT_EXPORT void initializeWithOptions(WebKitPlatformSupport*, const WebKitInitializationOptions&);
> > 
> 
> The initialization for v8 and for the compositor isn't really initializing anything - it's more binding gloop together so that it'll work together.  We use V8 in the browser process but still call initializeWithoutV8() cos we don't bind V8 and WebKit together in the browser.
>

I guess that's not entirely true - for V8 the call actually does initialize V8, but also does binding gloop.  For compositor it'll only do binding gloop, it won't actually initialize anything.

[Antoine Labour]

Created attachment 110074 [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 110074 [details]
Patch

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

I think a lot of our threading stuff will explode if the compositor's notion of the "main thread" and the "compositor thread" are actually the same thing - we do blocking calls from one to the other.

How are you ensuring that the compositor thread shuts down before the thread backing CompositorSupport?

> Source/WebCore/platform/graphics/chromium/cc/CCMainThread.cpp:45
> +    CompositorSupport::callOnMainThread(performTask, task.leakPtr());

hm, so you aren't using WTF::callOnMainThread() directly here any more?

If so you should update the comment in CCMainThread.h, remove the #include <wtf/MainThread.h> from this .cpp, and possibly go update CCLayerTreeHostTests to use your thread infrastructure instead of using webkit_support::'s main thread interaction functions.

[Antoine Labour]

(In reply to comment #18)
> (From update of attachment 110074 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=110074&action=review
> 
> I think a lot of our threading stuff will explode if the compositor's notion of the "main thread" and the "compositor thread" are actually the same thing - we do blocking calls from one to the other.

I know it's confusing... there's possibly up to 3 threads involved:
(1) the webkit main thread
(2) the compositor client thread
(3) the compositor thread.

This patch makes it possible to have (1) != (2). In the renderer, nothing changes, and (1) == (2), but in the browser process we instantiate webkit on a background thread, but we need to call the compositor from the UI thread.
This patch doesn't change anything wrt (3): either you use the single thread proxy, making (2) == (3), or you use the proxy, making (2) != (3) - regardless of the relationship between (1) and (2).

> 
> How are you ensuring that the compositor thread shuts down before the thread backing CompositorSupport?

For the renderer, nothing changes.
For the browser, by ensuring that we destroy WebLayerTreeViews, then we destroy the compositor WebThread (3) before we destroy the compositor client thread (2), which is the main thread anyway. We also ensure, client side, that the webkit main thread (1) is destroyed after that is done.
(note: threading is currently disabled when going through WebLayerTreeView, but a follow up patch will activate).

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCMainThread.cpp:45
> > +    CompositorSupport::callOnMainThread(performTask, task.leakPtr());
> 
> hm, so you aren't using WTF::callOnMainThread() directly here any more?

Correct. Is that a problem? From my quick reading of the core, WTF::callOnMainThread is essentially a wrapper around WebKitSupport::callOnMainThread. Since we end up calling that in the renderer, nothing should change. But if it's a problem, we can go through WTF::callOnMainThread.

> 
> If so you should update the comment in CCMainThread.h, remove the #include <wtf/MainThread.h> from this .cpp,

Will do.

> and possibly go update CCLayerTreeHostTests to use your thread infrastructure instead of using webkit_support::'s main thread interaction functions.

That shouldn't be necessary, since we'll go through the same path given the current test harness.

[Antoine Labour]

Created attachment 110204 [details]
Patch

[Antoine Labour]

(In reply to comment #18)
> (From update of attachment 110074 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=110074&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCMainThread.cpp:45
> > +    CompositorSupport::callOnMainThread(performTask, task.leakPtr());
> 
> hm, so you aren't using WTF::callOnMainThread() directly here any more?
> 
> If so you should update the comment in CCMainThread.h, remove the #include <wtf/MainThread.h> from this .cpp,

Done.

[Antoine Labour]

Any more thoughts about this patch / the approach?

[James Robinson]

Looks reasonable to me from the compositor side, but I think Darin should review.

[Darin Fisher (:fishd, Google)]

Comment on attachment 110204 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/CompositorSupport.cpp:34
> +    return s_delegate->currentTime();

Instead of thunking through a Delegate interface, we normally just move the
implementation of these methods to WebKit/chromium/src, so that we can call
WebKit APIs directly.  it avoids the need to define the intermediate vtable.

> Source/WebKit/chromium/public/WebKit.h:78
> +WEBKIT_EXPORT void initializeCompositor(WebCallOnMainThreadFunc);

it seems like these should be methods on WebCompositor.  we already have WebCompositor::setThread.
it seems like you can do the WebCore::CCProxy::setMainThread(currentThread()) call there, if we
don't already.  it seems like WebCompositor could have a static method for setting the
WebCompositorSupport interface.  also, perhaps the CallOnMainThread support should be provided
via WebCompositorSupport instead?

maybe WebCompositor::setThread should be changed to WebCompositor::initialize?

[Nat Duca]

Comment on attachment 110204 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/CompositorSupport.cpp:37
> +void CompositorSupport::histogramCustomCounts(const char* name, int sample, int min, int max, int bucketCount)

If you want to just get rid of the histograms, it might be easier. We're not using them anymore, anyway.

> Source/WebCore/platform/graphics/chromium/CompositorSupport.h:27
> +

Any reason this is in graphics/chromium rather than graphics/chromium/cc?

> Source/WebCore/platform/graphics/chromium/CompositorSupport.h:42
> +class CompositorSupport {

Sorry to be name police, and I agree this is a problematic use case, but are we ditching CC prefixing?

[Antoine Labour]

Created attachment 110615 [details]
Patch

[Antoine Labour]

Following in-person discussion with Darin, new patch here only deals with the threading issue.

[Nat Duca]

Comment on attachment 110615 [details]
Patch

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

> Source/WebKit/chromium/src/CCMainThreadImpl.cpp:79
> +        WTF::callOnMainThread(performTask, task.leakPtr());

For my eduation, when is s_clientThread NULL? Is this when webkit_unit_tests are running? Or is this just a way to land this patch when platformSupport()->currentThread() isn't implemented?

[Antoine Labour]

Comment on attachment 110615 [details]
Patch

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

>> Source/WebKit/chromium/src/CCMainThreadImpl.cpp:79
>> +        WTF::callOnMainThread(performTask, task.leakPtr());
> 
> For my eduation, when is s_clientThread NULL? Is this when webkit_unit_tests are running? Or is this just a way to land this patch when platformSupport()->currentThread() isn't implemented?

Correct, it's for the 2-sided patch.

[Antoine Labour]

FYI, http://codereview.chromium.org/8228025 for the chrome-side change

[James Robinson]

Comment on attachment 110615 [details]
Patch

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

> Source/WebKit/chromium/src/CCMainThreadImpl.cpp:41
> +bool s_initializedThread = false;
> +WebKit::WebThread* s_clientThread = 0;

it looks like the bool is the same thing as s_clientThread being NULL.

> Source/WebKit/chromium/src/CCMainThreadImpl.cpp:79
> +    else
> +        WTF::callOnMainThread(performTask, task.leakPtr());

why do you need this branch? s_clientThread should always be valid when we get here

[Antoine Labour]

Actually, s_clientThread could be NULL for non-MessageLoop threads. I'll change to an assert once the chrome side lands.

[WebKit Review Bot]

Comment on attachment 110615 [details]
Patch

Clearing flags on attachment: 110615

Committed r97232: <http://trac.webkit.org/changeset/97232>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.