Closed Bug 1390755 Opened 5 years ago Closed 5 years ago

Painting deadlocks with Skia and async OMTP

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: rhunt, Assigned: rhunt)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(4 files, 1 obsolete file)

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.
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.
Attached patch omtp-end-transaction.patch (obsolete) — Splinter Review
Attachment #8897735 - Flags: review?(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()) 
}
Flags: needinfo?(rhunt)
Attachment #8897735 - Attachment is obsolete: true
Attachment #8897735 - Flags: review?(mchang)
Blocks: omtp
Flags: needinfo?(rhunt)
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)
Attachment #8898023 - Flags: review?(mchang) → review+
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.
Attachment #8898024 - Flags: review?(mchang) → review+
Comment on attachment 8898025 [details]
Bug 1390755 - Rename PaintThread and CompositorBridgeChild methods to be more unified.

https://reviewboard.mozilla.org/r/169324/#review174682
Attachment #8898025 - Flags: review?(mchang) → review+
Comment on attachment 8898026 [details]
Bug 1390755 - Reorder functions in PaintThread and CompositorBridgeChild.

https://reviewboard.mozilla.org/r/169326/#review174684
Attachment #8898026 - Flags: review?(mchang) → review+
(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?
(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.
Pushed by rhunt@eqrion.net:
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
You need to log in before you can comment on or make changes to this bug.