763234 - Run all CompositorParents on the same compositor thread

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

The DirectVideo that Nico is doing, and the DirectCompositor work I'm doing (bug 745148) would both be made significantly, unnecessarily more complicated by having one thread per CompositorParent.  (I say "would" because we can assume only one compositor thread for now).

omtc is itself a big enough win, and we have no evidence that thread-per-compositor helps or hurts (or would even be *used*, commonly), so we should just use one compositor thread for now.

Comment 1

[Benoit Girard (:BenWa)]

The major con I see is that multiple compositors is a benefit for having multiple windows vsync

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

While working on bug 745148, I'm finding that it would be useful to move the "compositor thread" into CompositorParent.cpp itself.  Let's chat about this.  I'm happy to refactor on top of whatever you're hacking on currently.

Comment 3

[Nicolas Silva [:nical]]

Attachment: Use one compositr thread in OMTC.

The Compositor thread is now created and destroyed along with gfxPlatform.
I moved the logic that checks whether we use OMTC in gfxPlatform as well. So now, instead of checking the pref/environment variable, it is better to check the result of mozilla::layers::CompositorThread::GetSingleton() which will return a thread if OMTC is enabled, or nsnull if OMTC is not enabled (or if the thread somehow failed to start).

I think it makes things a bit less awkward than having the InitOnlyOnce() function in nsBaseWidget.cpp. I guess it doesn't really play well with the idea of cross-process compositing (I suppose in this context the thread/process would have to be initialized somewhere else), but the current way we create compositor threads is not any better in that area. I don't know how the cross-process stuff gets initialized.

I'd gladly change the class CompositorThread to be a namespace instead (it only has static methods and cannot be instanciated), but I am not sure about the policy for adding namespaces, and for some reason, there's often people who don't like functions that are not in a class. 
What do you guys think?

cjones: just read your comment, I'll move CompositorThread from it's own file to CompositorParent.cpp

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

The cross-process stuff is shaping up to be nicely orthogonal to the cross-thread code, so don't worry about that.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

How is vsync going to work after we've done this?

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Do you mean for multiple top-level windows?

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

The situation is
 - on OS X and GLX, we can use glSwapInterval() to sync to vblank without blocking on the GPU (unless the compositor screws up)
 - from what I understand, we don't tear with D3D10 and don't block either.  That may be because of using swap chains.  If so, it seems swap chains are supported with D3D9, so the situation should be the same there.  CC'ing Bas to clarify.
 - eglSwapBuffers() is broken in android GL (blocks), but we don't have multiple top-level windows

What we trade from using only one compositor thread is reduced parallelism across GPU commands from multiple top-level windows, but
 - who cares
 - it's not guaranteed that platform GPU libraries would utilize it anyway

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 634405 [details] [diff] [review]
Use one compositr thread in OMTC.

Let's roll the interface into CompositorParent::GetThread(), and move
the code into CompositorParent.{cpp,h}.  We can destroy the thread
when the CompositorParent dies.

The rest looks mostly ok on skim, but want to hold off review for
reorg.

> bool nsBaseWidget::UseOffMainThreadCompositing()
> {
>-  return sUseOffMainThreadCompositing;
>+  return mozilla::layers::CompositorThread::GetSingleton() != nsnull;

You want to check gfxPlatform here right?

Comment 10

[Nicolas Silva [:nical]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #9)
> Comment on attachment 634405 [details] [diff] [review]
> Use one compositr thread in OMTC.
> 
> Let's roll the interface into CompositorParent::GetThread(), and move
> the code into CompositorParent.{cpp,h}.  

I'll do this.

> We can destroy the thread
> when the CompositorParent dies.

There's one compositor parent per widget and they get destroyed along with the widgets, so I can't bind the thread's life time to a compositorParent.
I think it is simpler to just create and destroy the thread in gfxPlatform.

> 
> > bool nsBaseWidget::UseOffMainThreadCompositing()
> > {
> >-  return sUseOffMainThreadCompositing;
> >+  return mozilla::layers::CompositorThread::GetSingleton() != nsnull;
> 
> You want to check gfxPlatform here right?

Since the thread is created/destroyed in gfxPlatform, checking the existence of the thread is equivalent checking gfxPlatform.

Comment 11

[Nicolas Silva [:nical]]

Attachment: Use one compositor thread in OMTC.

The compositor thread methods (CreateThread, GetThread and DestroyThread) are now static methods of CompositorParent, rather than having 3 lonely funtions in a separate file (previously CompositorThread.h/cpp).

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 635330 [details] [diff] [review]
Use one compositor thread in OMTC.

>diff --git a/gfx/layers/ipc/CompositorParent.cpp b/gfx/layers/ipc/CompositorParent.cpp
>--- a/gfx/layers/ipc/CompositorParent.cpp
>+++ b/gfx/layers/ipc/CompositorParent.cpp
>@@ -18,16 +18,44 @@
> #include <android/log.h>
> #endif
> 
> using base::Thread;
> 
> namespace mozilla {
> namespace layers {
> 
>+namespace {
>+    base::Thread* sCompositorThread = nsnull;

Nit: flush with |namespace {| plz.  |static Thread*
sCompositorThread;| would achieve the same purpose.

>+}
>+
>+bool CompositorParent::CreateThread()
>+{
>+    if (sCompositorThread) return true;

Nits: 2-space indent here, and please do
  if (sCompositorThread) {
    return true;
  }

>+    sCompositorThread = new base::Thread("Compositor");

s/base:://.

>+    if (!sCompositorThread->Start()) {
>+        delete sCompositorThread;

You're leaving a dangling non-null freed pointer here :).  Need to
null out sCompositorThread.

>+void CompositorParent::DestroyThread()
>+{
>+    if (sCompositorThread) {

2-space indent here too.

>diff --git a/gfx/layers/ipc/CompositorParent.h b/gfx/layers/ipc/CompositorParent.h

>+namespace base {
>+  class Thread;

Nit: don't indent.

>+  static bool CreateThread();
>+  static void DestroyThread();
>+  static base::Thread* GetThread();

Please document this, in particular that DestroyThread can only be
called no more than once. Do we need CreateThread?  Can we just make
GetThread create the thread if it doesn't exist?

>diff --git a/gfx/thebes/gfxPlatform.cpp b/gfx/thebes/gfxPlatform.cpp
>--- a/gfx/thebes/gfxPlatform.cpp
>+++ b/gfx/thebes/gfxPlatform.cpp
>@@ -1,17 +1,21 @@
> /* -*- Mode: C++; tab-width: 20; indent-tabs-mode: nil; c-basic-offset: 2 -*-
>  * This Source Code Form is subject to the terms of the Mozilla Public
>  * License, v. 2.0. If a copy of the MPL was not distributed with this
>  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
> #ifdef MOZ_LOGGING
> #define FORCE_PR_LOG /* Allow logging in the release build */
> #endif
>+
>+#include "mozilla/layers/CompositorParent.h"
>+
> #include "prlog.h"
>+#include "prenv.h"
> 
> #include "gfxPlatform.h"
> 
> #if defined(XP_WIN)
> #include "gfxWindowsPlatform.h"
> #include "gfxD2DSurface.h"
> #elif defined(XP_MACOSX)
> #include "gfxPlatformMac.h"
>@@ -238,16 +242,40 @@ gfxPlatform::Init()
> #ifdef PR_LOGGING
>     sFontlistLog = PR_NewLogModule("fontlist");;
>     sFontInitLog = PR_NewLogModule("fontinit");;
>     sTextrunLog = PR_NewLogModule("textrun");;
>     sTextrunuiLog = PR_NewLogModule("textrunui");;
>     sCmapDataLog = PR_NewLogModule("cmapdata");;
> #endif
> 
>+    bool useOffMainThreadCompositing = false;
>+#ifdef MOZ_X11
>+    // On X11 platforms only use OMTC if firefox was initalized with thread-safe 
>+    // X11 (else it would crash).
>+    useOffMainThreadCompositing = (PR_GetEnv("MOZ_USE_OMTC") != NULL);
>+#else
>+    useOffMainThreadCompositing = Preferences::GetBool(
>+          "layers.offmainthreadcomposition.enabled", 
>+          false);
>+    // Until https://bugzilla.mozilla.org/show_bug.cgi?id=745148 lands,
>+    // we use either omtc or content processes, but not both.  Prefer
>+    // OOP content to omtc.  (Currently, this only affects b2g.)
>+    //
>+    // See https://bugzilla.mozilla.org/show_bug.cgi?id=761962 .
>+    if (!Preferences::GetBool("dom.ipc.tabs.disabled", true)) {
>+        // Disable omtc if OOP content isn't force-disabled.
>+        useOffMainThreadCompositing = false;
>+    }
>+#endif
>+

(This logic isn't quite right, but I'll fix it in bug 748145.)

>+    if (useOffMainThreadCompositing) {
>+        mozilla::layers::CompositorParent::CreateThread();

s/mozilla::layers//.

>+    mozilla::layers::CompositorParent::DestroyThread();

s/mozilla::layers//

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Also, I think it might be more convenient to hand out MessageLoop* here instead of Thread*, since most of the time we just want the MessageLoop* for the compositor thread.

Comment 14

[Nicolas Silva [:nical]]

Attachment: Use one compositor thread in OMTC.

cjones: I fixed the things you pointed out, including returning the message loop rather than the thread (So now we call CoompositorParent::compositorLoop which becomes static). I also removed the unnecessary arguments in CompositorParent (message loop and thread id).

I kept CreateCompositorThread separate because we can create the thread in gfxPlatform since we know the compositor's are not created at this point and then we don't have to deal with making the singleton's access/creation thread-safe. a compositor parent will be created shortly after gfx platform's init so there is nothing to gain from creating the thread lazily. Plus, It makes the creation-destruction of the thread symetrical.
Also, this way, after gfxPlatform's init we can check that CompositorParent::CompositorLoop doesn't return null rather than going through the logic behind whether or not to use OMTC.

Comment 15

[Nicolas Silva [:nical]]

Attachment: Use one compositor thread in OMTC.

The previous version didn't build on android, this one fixes it.

Comment 16

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Sorry for the review latency.  I've been test-driving this patch for the last few days so will have some comments based on that.  Biggest issue hit so far is that gfxPlatform isn't available before the first nsWindow is created on gonk, so we'll need to shuffle that around a bit.  (I hacked something.)

Comment 17

[Nicolas Silva [:nical]]

Attachment: Use one compositor thread in OMTC.

I had to rebase the patch (no changes in the way it works).

Comment 18

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 638872 [details] [diff] [review]
Use one compositor thread in OMTC.

>diff --git a/gfx/layers/ipc/CompositorParent.cpp b/gfx/layers/ipc/CompositorParent.cpp

>+static base::Thread* sCompositorThread = nsnull;
>+

s/base:://

>+bool CompositorParent::CreateThread()
>+{

Assert that we're on the main thread.

>+void CompositorParent::DestroyThread()
>+{

Assert that we're on the main thread.

>+MessageLoop* CompositorParent::CompositorLoop()
>+{
>+  if (sCompositorThread) {
>+    return sCompositorThread->message_loop();
>+  }
>+  return nsnull;

  return sCompositorThread ? sCompositorThread->message_loop() : nsnull;

>diff --git a/gfx/thebes/gfxPlatform.cpp b/gfx/thebes/gfxPlatform.cpp

>+    if (!Preferences::GetBool("dom.ipc.tabs.disabled", true)) {
>+        // Disable omtc if OOP content isn't force-disabled.
>+        useOffMainThreadCompositing = false;
>+    }
>+#endif
>+
>+

Nuke one of the blank lines here.

>+    if (useOffMainThreadCompositing) {
>+        CompositorParent::CreateThread();
>+    }

We don't want to do this in content processes.

So it turns out that, as I mentioned above, gfxPlatform::Init() is too
late in startup for omtc to be initialized for gonk.  For some reason
we create a widget earlier than this.  I don't have any clever ideas
for fixing that problem, but what I did in the platform-demo-mc branch
was split out the omtc initialization code into a separate gfxPlatform
helper function.  That wasn't a good fix, because I could have used
gfxPlatform::GetPlatform() instead, I think.  That's a little weird
but I guess there are worse things.

Would like to see the next version.

Comment 19

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Looking over bug 598868, I think we should move the implementation details of omtc startup into CompositorParent instead of spreading them around gfxPlatform.  So please make ContentParent::StartUp() and ContentParent::ShutDown(), and call those from gfxPlatform.

Comment 20

[Nicolas Silva [:nical]]

Attachment: Use one compositor thread in OMTC.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #18)
> Comment on attachment 638872 [details] [diff] [review]
> Use one compositor thread in OMTC.
> 
> >+    if (useOffMainThreadCompositing) {
> >+        CompositorParent::CreateThread();
> >+    }
> 
> We don't want to do this in content processes.

Can you elaborate a bit on this? So far I've been told to not worry about  b2g specific initialization stuff, so I don't know very well how it will (does?) work for omtc -> cross-prcess.

> 
> So it turns out that, as I mentioned above, gfxPlatform::Init() is too
> late in startup for omtc to be initialized for gonk.  For some reason
> we create a widget earlier than this.  I don't have any clever ideas
> for fixing that problem, but what I did in the platform-demo-mc branch
> was split out the omtc initialization code into a separate gfxPlatform
> helper function.  That wasn't a good fix, because I could have used
> gfxPlatform::GetPlatform() instead, I think.  That's a little weird
> but I guess there are worse things.

The other folks in the gfx team agree that this looks like a bug, because widgets assume they can use gfx things like surfaces, so gfxPlatform should be initialized before creating the first widget.

> 
> Would like to see the next version.

I fixed everything except the Startup/Shutdown:
Are you sure we want this? There should be only one compositor thread/process, but it there should be one ImageBridge thread per process that uses the image bridge, right?
I don't know exactly how the thing is supposed to wrap up in the cross-process architecture so I left the two separated in order to make it easier to adapt the initialization for b2g.

Comment 21

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Nicolas Silva [:nical] from comment #20)
> Created attachment 640610 [details] [diff] [review]
> Use one compositor thread in OMTC.
> 
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #18)
> > Comment on attachment 638872 [details] [diff] [review]
> > Use one compositor thread in OMTC.
> > 
> > >+    if (useOffMainThreadCompositing) {
> > >+        CompositorParent::CreateThread();
> > >+    }
> > 
> > We don't want to do this in content processes.
> 
> Can you elaborate a bit on this? So far I've been told to not worry about 
> b2g specific initialization stuff, so I don't know very well how it will
> (does?) work for omtc -> cross-prcess.
> 

There's nothing b2g-specific about content processes.  We never want to use a compositor thread in content processes, so this initialization is just wasteful and misleading.

> > 
> > Would like to see the next version.
> 
> I fixed everything except the Startup/Shutdown:
> Are you sure we want this? There should be only one compositor
> thread/process, but it there should be one ImageBridge thread per process
> that uses the image bridge, right?
> I don't know exactly how the thing is supposed to wrap up in the
> cross-process architecture so I left the two separated in order to make it
> easier to adapt the initialization for b2g.

In bug 598868, a lot of implementation details that are specific to the interaction of CompositorParent/ImageBridge are leaked into gfxPlatform init.  Let's let CompositorParent::StartUp/ShutDown take care of the "friend"-style initialization of those two objects, to keep those details local to gfx/layers/ipc.  This still doesn't have anything to do with multi-process or b2g.

Comment 22

[Nicolas Silva [:nical]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #21)
> There's nothing b2g-specific about content processes.  We never want to use
> a compositor thread in content processes, so this initialization is just
> wasteful and misleading.

I am sorry I must but tired or something. Now lets go through this one again: We are not talking about 'a' compositor thread but 'the' compositor thread. The compositor thread must be created somewhere. In the non-cross-process architecture (which is about every platform but b2g at the moment, hence my wrong use of the term b2g when referring to cross-process related things), gfxPlatform is initialized once, early and before the widgets. That's why it's there.

I suppose that gfxPlatform is initialized once per content process, so creating the compositor thread there works for the non-cross-process architecture, but does not fit the cross-process architecture.

If we start speaking about content processes, it means that we speak about the cross process architecture, the initialization of which has to be done differently, but I have been told not to worry about this. 

Where would we create the compositor process? lets just move the creation of the compositor thread there.

Comment 23

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

We are talking past each other.  Only initialize CompositorParent in gfxPlatform if (XRE_GetProcessType() == GeckoProcessType_Default).

Comment 24

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 640610 [details] [diff] [review]
Use one compositor thread in OMTC.

Nico says he has another patch coming up tomorrow.

Comment 25

[Nicolas Silva [:nical]]

Attachment: Use one compositor thread in OMTC.

Comment 26

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Fix for gonk widget back (please fold into previous patch)

Comment 27

[Nicolas Silva [:nical]]

Attachment: Use one compositor thread in OMTC (+ gonk fix).

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/558eedbae945

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/558eedbae945