1390755 - Painting deadlocks with Skia and async OMTP

Status: RESOLVED FIXED

(Core :: Graphics: Layers, enhancement, P3)

Description

[Ryan Hunt [:rhunt]]

The paint thread is restricted to being one frame behind the main thread before the main thread will block on it. The main thread is currently unblocked in these cases by a runnable that happens after async painting if texture clients need to synchronize. This runnable isn't queued with Skia currently.

Comment 1

[Ryan Hunt [:rhunt]]

The logic for when textures can/should be synced is similar and different from the logic for when we need to notify OMTP to unblock the main thread.

I decided to make them separate methods on the paint thread and that seems to have worked out easier than the other way around.

Comment 2

[Ryan Hunt [:rhunt]]

Attachment: omtp-end-transaction.patch

Comment 3

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8897735 [details] [diff] [review]
omtp-end-transaction.patch

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

::: gfx/layers/PaintThread.cpp
@@ +243,5 @@
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  // If we are forcing synchronization, then the main thread won't
> +  // get ahead of the paint thread, and there is no need to unlock it
> +  if (gfxPrefs::LayersOMTPForceSync()) {

I'd still prefer we go through all the logic even if it doesn't have to happen. Sync OMTP is really there to test everything and i'd still like to ensure EndAsyncLayerTransaction works.

@@ +263,5 @@
> +{
> +  MOZ_ASSERT(IsOnPaintThread());
> +  MOZ_ASSERT(!gfxPrefs::LayersOMTPForceSync());
> +
> +  CompositorBridgeChild::Get()->NotifyFinishedAsyncPaintTransaction();

nit: Can we still assert CompositorBridgeChild::Get() doesn't return null here?

::: gfx/layers/client/ClientLayerManager.cpp
@@ +742,5 @@
>    }
>  
> +  // If a texture sync wasn't requested then no async painting happened,
> +  // and we don't need to notify OMTP that this transaction is done.
> +  if (PaintThread::Get() && mTextureSyncOnPaintThread) {

Can we just roll up SynchronizePaintTextures and remove the lines above:
e.g.:

if (mForwarder->GetSyncObject() && !mTextureSyncOnPaintThread) {
   mForwarder->GetSyncObject()->Sync()
}

if (PaintThread::Get() && mTextureSyncOnPaintThread) {
   PaintThread::Get()->FinishedLayerTransaction(mForwrarder->GetSyncObject()) 
}

Comment 4

[Ryan Hunt [:rhunt]]

Attachment: Bug 1390755 - Notify the paint thread that a layer transaction is completed so it can unblock the main thread.

Review commit: https://reviewboard.mozilla.org/r/169320/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/169320/

Comment 5

[Ryan Hunt [:rhunt]]

Attachment: Bug 1390755 - Ensure PaintThread::EndTransaction runs before IPDL messages are resumed.

Review commit: https://reviewboard.mozilla.org/r/169322/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/169322/

Comment 6

[Ryan Hunt [:rhunt]]

Attachment: Bug 1390755 - Rename PaintThread and CompositorBridgeChild methods to be more unified.

Review commit: https://reviewboard.mozilla.org/r/169324/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/169324/

Comment 7

[Ryan Hunt [:rhunt]]

Attachment: Bug 1390755 - Reorder functions in PaintThread and CompositorBridgeChild.

Review commit: https://reviewboard.mozilla.org/r/169326/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/169326/

Comment 8

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8898023 [details]
Bug 1390755 - Notify the paint thread that a layer transaction is completed so it can unblock the main thread.

https://reviewboard.mozilla.org/r/169320/#review174674

::: gfx/layers/client/ClientLayerManager.cpp:732
(Diff revision 1)
>    TimeStamp start = TimeStamp::Now();
>  
>    // Skip the synchronization for buffer since we also skip the painting during
>    // device-reset status. With OMTP, we have to wait for async paints
>    // before we synchronize and it's done on the paint thread.
> +  SyncObjectClient* syncObject = nullptr;

Should be a RefPtr<SyncObjetClient>

::: gfx/layers/client/ClientLayerManager.cpp:747
(Diff revision 1)
> +  // all async painting is finished to do a texture sync and unblock the main
> +  // thread if it is waiting before doing a new layer transaction.
> -      if (mTextureSyncOnPaintThread) {
> +  if (mTextureSyncOnPaintThread) {
> -        // We have to wait for all async paints to finish to do this
> -        PaintThread::Get()->SynchronizePaintTextures(mForwarder->GetSyncObject());
> +    MOZ_ASSERT(PaintThread::Get());
> +    PaintThread::Get()->FinishedLayerTransaction(syncObject);
> -      } else {
> +  } else {

nit: else if (syncObject)

Comment 9

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8898024 [details]
Bug 1390755 - Ensure PaintThread::EndTransaction runs before IPDL messages are resumed.

https://reviewboard.mozilla.org/r/169322/#review174676

::: gfx/layers/PaintThread.cpp:212
(Diff revision 1)
>    MOZ_ASSERT(NS_IsMainThread());
>  
>    RefPtr<CompositorBridgeChild> cbc;
>    if (!gfxPrefs::LayersOMTPForceSync()) {
>      cbc = CompositorBridgeChild::Get();
> +    cbc->NotifyBeginAsyncPaintEndTransaction();

nit: This name is confusing. Maybe NotifyBeginAsyncTransactionComplete?

::: gfx/layers/ipc/CompositorBridgeChild.cpp:1236
(Diff revision 1)
>  
>    MonitorAutoLock lock(mPaintLock);
>  
>    MOZ_ASSERT(!mIsWaitingForPaint);
>  
> -  if (mOutstandingAsyncPaints > 0) {
> +  if (mOutstandingAsyncPaints > 0 || mOutstandingAsyncEndTransaction) {

nit: Add a comment explaining we should wait for the transaction to end because we need the texture synchronization to happen in addition to no outstanding paints.

Comment 10

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8898025 [details]
Bug 1390755 - Rename PaintThread and CompositorBridgeChild methods to be more unified.

https://reviewboard.mozilla.org/r/169324/#review174682

Comment 11

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8898026 [details]
Bug 1390755 - Reorder functions in PaintThread and CompositorBridgeChild.

https://reviewboard.mozilla.org/r/169326/#review174684

Comment 12

[Ryan Hunt [:rhunt]]

(In reply to Mason Chang [:mchang] from comment #9)
> Comment on attachment 8898024 [details]
> Bug 1390755 - Ensure PaintThread::EndTransaction runs before IPDL messages
> are resumed.
> 
> https://reviewboard.mozilla.org/r/169322/#review174676
> 
> ::: gfx/layers/PaintThread.cpp:212
> (Diff revision 1)
> >    MOZ_ASSERT(NS_IsMainThread());
> >  
> >    RefPtr<CompositorBridgeChild> cbc;
> >    if (!gfxPrefs::LayersOMTPForceSync()) {
> >      cbc = CompositorBridgeChild::Get();
> > +    cbc->NotifyBeginAsyncPaintEndTransaction();
> 
> nit: This name is confusing. Maybe NotifyBeginAsyncTransactionComplete?
> 

Yeah, I agree that it's not good. I renamed it in part 4 to NotifyBeginAsyncEndLayerTransaction, was this including that rename?

Comment 13

[Mason Chang [Inactive] [:mchang]]

(In reply to Ryan Hunt [:rhunt] from comment #12)
> (In reply to Mason Chang [:mchang] from comment #9)
> > Comment on attachment 8898024 [details]
> > Bug 1390755 - Ensure PaintThread::EndTransaction runs before IPDL messages
> > are resumed.
> > 
> > https://reviewboard.mozilla.org/r/169322/#review174676
> > 
> > ::: gfx/layers/PaintThread.cpp:212
> > (Diff revision 1)
> > >    MOZ_ASSERT(NS_IsMainThread());
> > >  
> > >    RefPtr<CompositorBridgeChild> cbc;
> > >    if (!gfxPrefs::LayersOMTPForceSync()) {
> > >      cbc = CompositorBridgeChild::Get();
> > > +    cbc->NotifyBeginAsyncPaintEndTransaction();
> > 
> > nit: This name is confusing. Maybe NotifyBeginAsyncTransactionComplete?
> > 
> 
> Yeah, I agree that it's not good. I renamed it in part 4 to
> NotifyBeginAsyncEndLayerTransaction, was this including that rename?

Yeah sorry reviewed them in order. It's fine.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/53197932a884
Notify the paint thread that a layer transaction is completed so it can unblock the main thread. r=mchang
https://hg.mozilla.org/integration/mozilla-inbound/rev/7bd53f7a2116
Ensure PaintThread::EndTransaction runs before IPDL messages are resumed. r=mchang
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b0eb3f25b74
Rename PaintThread and CompositorBridgeChild methods to be more unified. r=mchang
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2b09cf8f6c5
Reorder functions in PaintThread and CompositorBridgeChild. r=mchang

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/53197932a884
https://hg.mozilla.org/mozilla-central/rev/7bd53f7a2116
https://hg.mozilla.org/mozilla-central/rev/5b0eb3f25b74
https://hg.mozilla.org/mozilla-central/rev/a2b09cf8f6c5