Bootstrap a painting thread for OMTP

RESOLVED FIXED in Firefox 56

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: dvander, Assigned: domfarolino)

Tracking

(Blocks 1 bug)

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

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
(Assignee)

Comment 1

2 years ago
Posted patch bug1369549.patch (obsolete) — Splinter Review
This bootstraps/initializes a thread for us to paint on.
Attachment #8875069 - Flags: review?(dvander)
Just an FYI, this doesn't build on Windows for me without https://bugzilla.mozilla.org/show_bug.cgi?id=1371098
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.)
Attachment #8875069 - Flags: review?(dvander)
(Assignee)

Comment 4

2 years ago
Posted patch bug1369549.patch (obsolete) — Splinter Review
Address review + minor styling conventions
Attachment #8875069 - Attachment is obsolete: true
Attachment #8875889 - Flags: review?(dvander)
(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) ?
Flags: needinfo?(dvander)
(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);
Flags: needinfo?(dvander)
(Assignee)

Comment 7

2 years ago
Posted patch bug1369549.patch (obsolete) — Splinter Review
This should address the change requests
Attachment #8875889 - Attachment is obsolete: true
Attachment #8876009 - Flags: review?(dvander)
Attachment #8875889 - Flags: review?(dvander)
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
Attachment #8876009 - Flags: review?(dvander)
(Assignee)

Comment 9

2 years ago
Posted patch bug1369549.patch (obsolete) — Splinter Review
Address latest change requests + nits
Attachment #8876009 - Attachment is obsolete: true
Attachment #8876307 - Flags: review?(dvander)
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).
Attachment #8876307 - Flags: review?(dvander) → review+
(Assignee)

Comment 11

2 years ago
Address MOZ_ASSERTs
Attachment #8876307 - Attachment is obsolete: true
Attachment #8876335 - Flags: review?(dvander)
Attachment #8876335 - Flags: review?(dvander) → review+
(Assignee)

Comment 12

2 years ago
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
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 13

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/9a0520e85036
Bootstrap a painting thread for OMTP. r=dvander
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9a0520e85036
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.