Closed Bug 1390755 Opened 5 years ago Closed 5 years ago

Painting deadlocks with Skia and async OMTP

Tracking

()

Status:

RESOLVED FIXED

Milestone:

mozilla57

Tracking Flags:

Tracking

Status

firefox57

---

fixed

People

(Reporter: rhunt, Assigned: rhunt)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(4 files, 1 obsolete file)

omtp-end-transaction.patch 5 years ago Ryan Hunt [:rhunt] 4.60 KB, patch		Details \| Diff \| Splinter Review
Bug 1390755 - Notify the paint thread that a layer transaction is completed so it can unblock the main thread. 5 years ago Ryan Hunt [:rhunt] 59 bytes, text/x-review-board-request	mchang : review+	Details
Bug 1390755 - Ensure PaintThread::EndTransaction runs before IPDL messages are resumed. 5 years ago Ryan Hunt [:rhunt] 59 bytes, text/x-review-board-request	mchang : review+	Details
Bug 1390755 - Rename PaintThread and CompositorBridgeChild methods to be more unified. 5 years ago Ryan Hunt [:rhunt] 59 bytes, text/x-review-board-request	mchang : review+	Details
Bug 1390755 - Reorder functions in PaintThread and CompositorBridgeChild. 5 years ago Ryan Hunt [:rhunt] 59 bytes, text/x-review-board-request	mchang : review+	Details

Ryan Hunt [:rhunt]

Assignee

Description

•

5 years ago

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.

Ryan Hunt [:rhunt]

Assignee

Comment 1

•

5 years ago

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.

Ryan Hunt [:rhunt]

Assignee

Comment 2

•

5 years ago

Attached patch omtp-end-transaction.patch (obsolete) — Details — Splinter Review

Attachment #8897735 - Flags: review?(mchang)

Mason Chang [Inactive] [:mchang]

Comment 3

•

5 years ago

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

Mason Chang [Inactive] [:mchang]

Updated

•

5 years ago

Flags: needinfo?(rhunt)

Comment hidden (mozreview-request)

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

Comment hidden (mozreview-request)

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

Comment hidden (mozreview-request)

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

Comment hidden (mozreview-request)

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

Ryan Hunt [:rhunt]

Assignee

Updated

•

5 years ago

Attachment #8897735 - Attachment is obsolete: true

Attachment #8897735 - Flags: review?(mchang)

Ryan Hunt [:rhunt]

Assignee

Updated

•

5 years ago

Blocks: omtp

Flags: needinfo?(rhunt)

Mason Chang [Inactive] [:mchang]

Comment 8

•

5 years ago

mozreview-review

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+

Mason Chang [Inactive] [:mchang]

Comment 9

•

5 years ago

mozreview-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+

Mason Chang [Inactive] [:mchang]

Comment 10

•

5 years ago

mozreview-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+

Mason Chang [Inactive] [:mchang]

Comment 11

•

5 years ago

mozreview-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+

Ryan Hunt [:rhunt]

Assignee

Comment 12

•

5 years ago

(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?

Mason Chang [Inactive] [:mchang]

Comment 13

•

5 years ago

(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.

Pulsebot

Comment 14

•

5 years ago

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

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

Comment 15

•

5 years ago

bugherder

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

Status: NEW → RESOLVED

Closed: 5 years ago

status-firefox57: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla57

You need to log in before you can comment on or make changes to this bug.

Bugzilla

Painting deadlocks with Skia and async OMTP

Categories

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

Tracking

()

People

(Reporter: rhunt, Assigned: rhunt)

References

Details

(Whiteboard: [gfx-noted])

Crash Data

Security

(public)

User Story

Attachments

(4 files, 1 obsolete file)

Description

Comment 1

Comment 2

Comment 3

Updated

Comment 4

Comment 5

Comment 6

Comment 7

Updated

Updated

Comment 8

Comment 9

Comment 10

Comment 11

Comment 12

Comment 13

Comment 14

Comment 15