1369549 - Bootstrap a painting thread for OMTP

Status: RESOLVED FIXED

(Core :: Graphics: Layers, enhancement)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

After bug 1369542, we can create a painting thread when OMTP is enabled. For now we'll just make an idle thread and make sure it shuts down safely. IPDL integration will get a separate bug.

Let's create a new PaintingThread class with static Start and Shutdown methods. Internally these should create a new singleton instance of PaintingThread that hold a reference to an nsIThread. nsIThreads can be created with NS_NewNamedThread. We should expose the singleton through a Get() method, like we do for GPUProcessManager.

InitLayersIPC [1] is probably the right place to check for OMTP and start this thread. Similarly we can shutdown the thread in ShutdownLayersIPC. Not sure where the PaintingThread.cpp/h files should go. Maybe gfx/layers?

[1] http://searchfox.org/mozilla-central/rev/1a0d9545b9805f50a70de703a3c04fc0d22e3839/gfx/thebes/gfxPlatform.cpp#943

Comment 1

[Dominic Farolino [:domfarolino]]

Attachment: bug1369549.patch

This bootstraps/initializes a thread for us to paint on.

Comment 2

[Mason Chang [Inactive] [:mchang]]

Just an FYI, this doesn't build on Windows for me without https://bugzilla.mozilla.org/show_bug.cgi?id=1371098

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8875069 [details] [diff] [review]
bug1369549.patch

Review of attachment 8875069 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/PaintThread.cpp
@@ +8,5 @@
> +
> +namespace mozilla {
> +namespace layers {
> +
> +PaintThread* PaintThread::sSingleton = nullptr;

If you include StaticAutoPtr.h, you can do this:

> StaticAutoPtr<PaintThread> PaintThread::sSingleton;

And then you can just assign nullptr and it'll auto-delete.

@@ +14,5 @@
> +/* static */ void
> +PaintThread::Start()
> +{
> +  PaintThread::sSingleton = new PaintThread();
> +  NS_NewNamedThread("PaintThread", getter_AddRefs(PaintThread::sSingleton->mThread));

It'd be better to factor this into an Init() method in PaintThread, that returns true/false on success. Then if it fails we can unassigned sSingleton, and do something like:

>  gfxCriticalNote << "Unable to start paint thread";

@@ +18,5 @@
> +  NS_NewNamedThread("PaintThread", getter_AddRefs(PaintThread::sSingleton->mThread));
> +}
> +
> +/* static */ PaintThread*
> +PaintThread::Get() {

nit: { on newline

@@ +38,5 @@
> +  if (NS_IsMainThread()) {
> +    PaintThread::sSingleton->mThread->AsyncShutdown();
> +  } else {
> +    NS_DispatchToMainThread(NewRunnableMethod(PaintThread::sSingleton->mThread, &nsIThread::AsyncShutdown));
> +  }

This part I'd factor into a ShutdownImpl method on PaintThread.

::: gfx/layers/PaintThread.h
@@ +18,5 @@
> +public:
> +  static void Start();
> +  static void Shutdown();
> +  static PaintThread* Get();
> +  static RefPtr<nsIThread> GetThread();

We probably don't need this GetThread accessor. We don't want to expose the threading behavior directly.

::: gfx/thebes/gfxPlatform.cpp
@@ +954,5 @@
>          if (gfxVars::UseWebRender()) {
>              wr::RenderThread::Start();
>          }
> +
> +        if (gfxVars::UseOMTP()) {

We decided today that initially we're only going to do OMTP in content processes, so let's move this to an else-if XRE_IsContentProcess block, here and in ShutdownLayersIPC. The parent process is still responsible for making the call on whether or not to use the feature, but we don't actually want to spin up a PaintThread in the parent process yet.

(The reason is twofold: one, it reduces the amount of work we have to do to ship a v1, and two, the IPDL stuff will be complicated and we don't want to entrain the UI in that until it's bulletproof.)

Comment 4

[Dominic Farolino [:domfarolino]]

Attachment: bug1369549.patch

Address review + minor styling conventions

Comment 5

[Mason Chang [Inactive] [:mchang]]

(In reply to David Anderson [:dvander] from comment #3)
> ::: gfx/layers/PaintThread.h
> @@ +18,5 @@
> > +public:
> > +  static void Start();
> > +  static void Shutdown();
> > +  static PaintThread* Get();
> > +  static RefPtr<nsIThread> GetThread();
> 
> We probably don't need this GetThread accessor. We don't want to expose the
> threading behavior directly.

How did you want this to work? Don't we need the nsIThread to dispatch a runnable to it? Or were you expecting like static PaintThread::PaintTheThings(PaintRecording* aRecording) ?

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Mason Chang [:mchang] from comment #5)
> (In reply to David Anderson [:dvander] from comment #3)
> > ::: gfx/layers/PaintThread.h
> > @@ +18,5 @@
> > > +public:
> > > +  static void Start();
> > > +  static void Shutdown();
> > > +  static PaintThread* Get();
> > > +  static RefPtr<nsIThread> GetThread();
> > 
> > We probably don't need this GetThread accessor. We don't want to expose the
> > threading behavior directly.
> 
> How did you want this to work? Don't we need the nsIThread to dispatch a
> runnable to it? Or were you expecting like static
> PaintThread::PaintTheThings(PaintRecording* aRecording) ?

PaintThread* thread = PaintThread::Get();
thread->PaintTheThings(recording);

Comment 7

[Dominic Farolino [:domfarolino]]

Attachment: bug1369549.patch

This should address the change requests

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8876009 [details] [diff] [review]
bug1369549.patch

Review of attachment 8876009 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/PaintThread.cpp
@@ +42,5 @@
> +
> +/* static */ void
> +PaintThread::Shutdown()
> +{
> +  if (!PaintThread::sSingleton->mThread) {

This should be:

  if (!PaintThread::sSingleton) {

::: gfx/layers/PaintThread.h
@@ +15,5 @@
> +
> +class PaintThread final
> +{
> +public:
> +  bool Init();

nit: Init, ShutdownImpl should be private.

@@ +19,5 @@
> +  bool Init();
> +  static void Start();
> +  static void Shutdown();
> +  void ShutdownImpl();
> +  static StaticAutoPtr<PaintThread> Get();

This must return PaintThread*, not StaticAutoPtr (otherwise PaintThread will probably be destroyed every time we call Get)

::: gfx/thebes/gfxPlatform.cpp
@@ +957,2 @@
>  
> +  if (XRE_IsParentProcess()) {

nit: combine into } else if { with block above

Comment 9

[Dominic Farolino [:domfarolino]]

Attachment: bug1369549.patch

Address latest change requests + nits

Comment 10

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8876307 [details] [diff] [review]
bug1369549.patch

Review of attachment 8876307 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/PaintThread.cpp
@@ +54,5 @@
> +void
> +PaintThread::ShutdownImpl()
> +{
> +  if (NS_IsMainThread()) {
> +    PaintThread::sSingleton->mThread->AsyncShutdown();

This is only ever called from the main thread, so you can drop the check and second condition here. You can even MOZ_ASSERT(NS_IsMainThread()) if you'd like (maybe even a good idea to put one on Init and Get too).

Comment 11

[Dominic Farolino [:domfarolino]]

Attachment: bug1369549.patch

Address MOZ_ASSERTs

Comment 12

[Dominic Farolino [:domfarolino]]

The try submission link for the patches of this bug (1369549) also includes the patches from bug 1369542. When try completes (assuming no errors) both bugs should be able to be checked in:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=795b022af9e183f4653e96a4f5e2eab17535e5e9

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9a0520e85036
Bootstrap a painting thread for OMTP. r=dvander

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/9a0520e85036