Closed Bug 1377060 Opened 5 years ago Closed 5 years ago

Add a toggle for asynchronous OMTP

Categories

(Core :: Graphics: Layers, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: dvander, Assigned: dvander)

References

Details

Attachments

(2 files, 1 obsolete file)

The goal here is to make the "OMT" in "OMTP" actually mean something. The main thread will start delaying IPC messages right before it sends a transaction, and the paint thread will resume IPC once it's done painting.
Mason, this is the same as the previous refactoring patch, except it also refactors PaintContents into two methods. This just makes it a little cleaner since the next patch will allow async dispatch and we need to hold the input arguments in RefPtrs.
Attachment #8882113 - Flags: review?(mchang)
Attached patch part 2, implementation (obsolete) — Splinter Review
This implements asynchronous transactions.

The paint flow now looks something like this, for an example with one layer:
 1. MAIN: ClientLayerManager schedules a paint for a PaintedLayer.
 2. MAIN: CompositorBridgeChild begins "waiting for paints", which postpones IPDL messages.
 3. MAIN: We return to the event loop.
 4. PAINT: PaintThread finishes painting, and notifies CompositorBridgeChild, resuming IPDL messages.

This is not the only valid ordering. For example, if painting is fast, we might get this:
 1. MAIN: ClientLayerManager schedules a paint for a PaintedLayer.
 2. PAINT: PaintThread finishes painting.
 3. MAIN: ShadowLayerForwarder forwards its transaction.
 4. MAIN: We return to the event loop.

Before we begin a transaction, we make sure to flush the paint thread. This is done by just blocking until the CompositorBridgeChild has received all the outstanding paint notifications from the PaintThread.

This is all synchronized with some state in CompositorBridgeChild and a Monitor. I chose CBC for this state instead of PaintThread since painting is fundamentally tied to an IPDL connection, and having the PaintThread associate a paint with a specific CBC makes a lot of sense. If the compositor process dies a new CBC will get created, and the PaintThread will pick it up automatically on the next paint. Old paints won't go to the wrong place, and we don't need to shutdown and restart the paint thread.

Currently this is pretty buggy, I get lots of flickering when scrolling, so it's behind an experimental pref (layers.omtp.force-sync=false).
Attachment #8882120 - Flags: review?(mchang)
Comment on attachment 8882113 [details] [diff] [review]
part 1, refactor PaintThread

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

::: gfx/layers/PaintThread.cpp
@@ +125,5 @@
> +
> +  RefPtr<DrawTargetCapture> capture(aCapture);
> +  RefPtr<DrawTarget> target(aTarget);
> +
> +  PaintThread* self = this;

sorry n00b question, why do you need the self here?
Attachment #8882113 - Flags: review?(mchang) → review+
(In reply to Mason Chang [:mchang] from comment #3)
> Comment on attachment 8882113 [details] [diff] [review]
> part 1, refactor PaintThread
> 
> Review of attachment 8882113 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/PaintThread.cpp
> @@ +125,5 @@
> > +
> > +  RefPtr<DrawTargetCapture> capture(aCapture);
> > +  RefPtr<DrawTarget> target(aTarget);
> > +
> > +  PaintThread* self = this;
> 
> sorry n00b question, why do you need the self here?

We don't, I did it out of habit (if it was RefPtr it'd need it). Will drop it.
Comment on attachment 8882120 [details] [diff] [review]
part 2, implementation

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

::: gfx/layers/ipc/CompositorBridgeChild.cpp
@@ +1147,5 @@
> +
> +void
> +CompositorBridgeChild::NotifyFinishedAsyncPaint()
> +{
> +  MOZ_ASSERT(NS_IsMainThread());

Shouldn't this be on the paint thread?

@@ +1160,5 @@
> +  if (mIsWaitingForPaint && mOutstandingAsyncPaints == 0) {
> +    ResumeIPCAfterAsyncPaint();
> +
> +    // Notify the main thread in case it's blocking. We do this unconditionally
> +    // to avoid deadlocking, though 

nit: whitespace

@@ +1188,5 @@
> +  MOZ_ASSERT(PaintThread::IsOnPaintThread());
> +  MOZ_ASSERT(mOutstandingAsyncPants == 0);
> +  MOZ_ASSERT(mIsWaitingForPaint);
> +
> +  mIsWaitingForPaint = false;

nit: State caller is responsible to grab the mPaintLock

@@ +1199,5 @@
> +  GetIPCChannel()->StopPostponingSends();
> +}
> +
> +void
> +CompositorBridgeChild::FlushAsyncPaints()

Since this is called on the main thread, does this actually work?

Let's say we had the following order:
1) Main thread queues a paint
2) Main thread pauses sends
3) Paint thread starts painting
4) Compositor sends back a transaction, main thread starts painting again
5) We call FlushAsyncPaints
6) NotifyFinishedPainting is called

Couple of questions:
1) Can the compositor send back messages or is postponement one way only?
2) Since we make the main thread wait here, when the paint thread finishes and calls NotifyFinishedPainting, this will flush the messages even while the main thread is asleep and they were queued on the main thread?

@@ +1205,5 @@
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  MonitorAutoLock lock(mPaintLock);
> +  while (mIsWaitingForPaint) {
> +    mPaintLock.Wait();

Shouldn't we be calling wait/lock via MonitorAutoLock [1]

[1] http://searchfox.org/mozilla-central/source/xpcom/threads/Monitor.h#20
(In reply to Mason Chang [:mchang] from comment #5)
> Comment on attachment 8882120 [details] [diff] [review]
> part 2, implementation
> 
> Since this is called on the main thread, does this actually work?
> 
> Let's say we had the following order:
> 1) Main thread queues a paint
> 2) Main thread pauses sends
> 3) Paint thread starts painting
> 4) Compositor sends back a transaction, main thread starts painting again
> 5) We call FlushAsyncPaints
> 6) NotifyFinishedPainting is called
> 
> Couple of questions:
> 1) Can the compositor send back messages or is postponement one way only?
> 2) Since we make the main thread wait here, when the paint thread finishes
> and calls NotifyFinishedPainting, this will flush the messages even while
> the main thread is asleep and they were queued on the main thread?

Answered IRL, but for posterity:

(1) It's one-way, we can still receive messages.
(2) This is okay, we designed the IPDL functionality here explicitly for the OMTP use case.
w/ comments addressed.
Attachment #8882120 - Attachment is obsolete: true
Attachment #8882120 - Flags: review?(mchang)
Attachment #8882365 - Flags: review?(mchang)
Comment on attachment 8882365 [details] [diff] [review]
part 2, implementation v2

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

::: gfx/layers/ipc/CompositorBridgeChild.h
@@ +353,5 @@
>  
>    FixedSizeSmallShmemSectionAllocator* mSectionAllocator;
> +
> +  // Off-Main-Thread Painting state.
> +  Monitor mPaintLock;

nit: State this lock covers both variables.
Attachment #8882365 - Flags: review?(mchang) → review+
Pushed by danderson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf11ec80b0fb
Refactor PaintThread for async painting. (bug 1377060 part 1, r=mchang)
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8f818803df7
Implement asynchronous OMTP behind a pref. (bug 1377060 part 2, r=mchang)
Turns out because PaintThread implements AddRef/Release, even though they are no-ops, static analysis demands we assign |this| to a temporary variable to be captured. Pushing again with that fixed.
Flags: needinfo?(dvander)
Pushed by danderson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dcfbfbd218c9
Refactor PaintThread for async painting. (bug 1377060 part 1, r=mchang)
https://hg.mozilla.org/integration/mozilla-inbound/rev/26efbaef8509
Implement asynchronous OMTP behind a pref. (bug 1377060 part 2, r=mchang)
https://hg.mozilla.org/mozilla-central/rev/dcfbfbd218c9
https://hg.mozilla.org/mozilla-central/rev/26efbaef8509
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.