Allow deferring messages in MessageChannel for OMTP

RESOLVED FIXED in Firefox 56

Status

()

Core
IPC
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: dvander, Assigned: dvander)

Tracking

(Blocks: 1 bug)

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Our initial plan for OMTP is to send rasterization messages from the paint thread and layers transactions from the main thread. This results in a nasty ordering problem: if the transaction arrives first, we will break rendering by having incomplete textures. If we block the compositor, APZ will break and other windows will stop painting.

The two easiest options we explored were (1) proxying CompositorBridge messages to the paint thread, and (2) deferring messages on CompositorBridgeChild. We're going to start off with message deferral. This is what Jerry tried in bug 1235018, and the rationale there was (a) it's easier and (b) it results in less context switching.

We will have to be careful that this doesn't regress tab switching time or APZ input latency. Certain messages will need to avoid being deferred.

Another problem is that when rasterization completes, we don't want to unblock *all* messages that have been deferred. Each new transaction we start needs to begin a new queue of deferred messages, and rasterization must only undefer the most recent queue. Initially we plan to block the next paint on the previous paint being rasterized (required since content/webgl does not triple-buffer). So this won't be a problem as long as we expose a way for the main thread to synchronize.

So this bug encompasses the following things:
 * Adding a function to MessageChannel such that messages go into a separate queue instead of the I/O thread.
 * Adding a function to MessageChannel so the paint thread can move this queue into the I/O thread, without posting a message to the main thread.
 * Making sure a MessageChannel wakes up if it is blocked sending a sync message that gets deferred.
 * We might need a filter mechanism so some messages can bypass the deferral queue (like APZ).
Created attachment 8873721 [details] [diff] [review]
part 1, IPDL unit tests

This is an IPDL unit test for how painting will work: two threads on the child side sending transactions to the parent. The test fails if the parent receives a transaction before it receives a matching painting, or if it receives messages before a transaction is completed.
Created attachment 8877870 [details] [diff] [review]
part 1, ipdl tests
Attachment #8873721 - Attachment is obsolete: true
Attachment #8877870 - Flags: review?(wmccloskey)
Created attachment 8877871 [details] [diff] [review]
part 2, implementation

This adds two new functions to MessageChannel.

The first is "BeginPostponingSends". It must be called from the main thread. When called it simply sets a flag on the channel. When this flag is set, any kind of non-special-io message is not sent over the MessageLink - instead it is added to a local queue.

The second is "StopPostponingSends". It can be called from any thread, though really it is meant to be called off the worker thread. It passes the entire postponed queue of messages over to the link, and then unsets the postpone flag.

At first I thought we would need special handling for sync messages, but now I'm not sure if that's true. The worker thread can stay blocked, and it'll just automatically wake up when the reply to the postponed message comes in.

Things I'm less sure about: interaction with high priority messages during a sync message, or special IO messages (right now they are not postponed). But we don't really use priorities in the PCompositorBridge protocol.
Attachment #8877871 - Flags: review?(wmccloskey)
Comment on attachment 8877871 [details] [diff] [review]
part 2, implementation

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

Can you add an assertion in Call() that we're not postponing messages?

::: ipc/glue/MessageChannel.h
@@ +261,5 @@
> +    // This must be called from the worker thread.
> +    void BeginPostponingSends();
> +
> +    // Stop postponing sent messages, and immediately flush all postponed
> +    // messages to the link. This may be called from any thread.

As we discussed, I think it's important to point out that there are still no ordering guarantees between channels unless you do some additional synchronization, like using a sync message.

@@ +812,5 @@
>      RefPtr<CancelableRunnable> mOnChannelConnectedTask;
>      bool mPeerPidSet;
>      int32_t mPeerPid;
> +
> +    // Channels can enter a state where any sent message results is not sent

The grammar here is weird. Maybe a typo?

::: ipc/ipdl/test/cxx/TestOffMainThreadPainting.cpp
@@ +259,5 @@
>    // Sleep for some time to simulate painting being slow.
>    PR_Sleep(PR_MillisecondsToInterval(500));
>    SendFinishedPaint(aTxnId);
> +
> +  mMainChannel->StopPostponingSends();

I think this is still racy. Main thread messages could still be received before the FinishedPaint message unless FinishedPaint is sync. So maybe make it sync?
Attachment #8877871 - Flags: review?(wmccloskey) → review+
Comment on attachment 8877870 [details] [diff] [review]
part 1, ipdl tests

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

::: ipc/ipdl/test/cxx/TestOffMainThreadPainting.cpp
@@ +31,5 @@
> +  if (NS_FAILED(rv)) {
> +    fail("create pipes");
> +  }
> +
> +  mPaintActor = MakeAndAddRef<TestPaintThreadParent>(this);

Don't think you need MakeAndAddRef here. Using |new| will accomplish the same thing.

@@ +140,5 @@
> +  if (!mPaintThread->Start()) {
> +    return IPC_FAIL_NO_REASON(this);
> +  }
> +
> +  mPaintActor = MakeAndAddRef<TestPaintThreadChild>();

Same about MakeAndAddRef.

@@ +142,5 @@
> +  }
> +
> +  mPaintActor = MakeAndAddRef<TestPaintThreadChild>();
> +  RefPtr<Runnable> task = NewRunnableMethod<ipc::Endpoint<PTestPaintThreadChild>&&>(
> +    mPaintActor, &TestPaintThreadChild::Bind, Move(aEndpoint));

You'll help me out if you provide a runnable name ("TestPaintThreadChild::Bind") as the first argument here. I'm trying to remove the anonymous version of NewRunnableMethod.

@@ +183,5 @@
> +  SendEndTest();
> +}
> +
> +/*************************
> + * PTestPaintThreadChild *

This should say PTestPaintThreadParent.
Attachment #8877870 - Flags: review?(wmccloskey) → review+

Comment 6

a year ago
Pushed by danderson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/94aedd725ced
Add IPDL test files for message ordering problems in OMTP. (bug 1369529 part 1, r=billm)
https://hg.mozilla.org/integration/mozilla-inbound/rev/055f29a7a93d
Allow IPDL message sends to be deferred and re-sent as needed. (bug 1369529 part 2, r=billm)

Comment 7

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/94aedd725ced
https://hg.mozilla.org/mozilla-central/rev/055f29a7a93d
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.