Open Bug 1457276 Opened 2 years ago Updated 11 months ago

TabChild::RecvRenderLayers often results in two consecutive paints

Categories

(Core :: DOM: Content Processes, enhancement, P2)

enhancement

Tracking

()

People

(Reporter: mconley, Assigned: mconley)

References

Details

(Whiteboard: [fxperf:p3])

Check out this profile from bug 1453080:

https://perfht.ml/2HQLfxw

Here, bdahl was hovering a tab with tab warming enabled. We do two consecutive, long paints.

Why are we doing two paints?
Did some digging. It looks like there are two things contributing to that second paint.

This is the first one:

https://searchfox.org/mozilla-central/rev/78dbe34925f04975f16cb9a5d4938be714d41897/dom/ipc/TabChild.cpp#2646-2650

We schedule a paint right before doing... a paint. I'm not sure why. Digging into the history of those lines, it looks like it goes back to when billm made it so that we can interrupt JS to paint - and even then, I think he was bringing it forward from previous older code that was causing us to paint:

https://bugzilla.mozilla.org/show_bug.cgi?id=1279086#c38

It seems like we might only need to perform that second paint if we're asking the PuppetWidget to paint... we can probably find a new way of doing this without scheduling a paint.

Even removing that though, we'll still see a second paint, and that's because unsuppressing the displayport always results in a paint being scheduled on the next refresh driver tick:

https://searchfox.org/mozilla-central/rev/78dbe34925f04975f16cb9a5d4938be714d41897/gfx/layers/apz/util/APZCCallbackHelper.cpp#919

I'm not 100% sure if those reschedules are necessary in the case of tab warming, or even tab switching in general. Still investigating.
(In reply to Mike Conley (:mconley) (:⚙️) (Catching up on needinfos / reviews) from comment #1)
> I'm not 100% sure if those reschedules are necessary in the case of tab
> warming, or even tab switching in general. Still investigating.

I'm now 95% sure that the paint that we're scheduling in RecvRenderLayers is unnecessary.

It was originally added in bug 1279086. See bug 1279086 comment 38 where billm says that he _thinks_ he needs to ensure a Paint is queued to make sure PuppetWidget paints.

But I think we don’t need it. He says that if it’s safe to run script, then PaintNowIfNeeded will only fire if a paint is scheduled. But a paint should be scheduled because TabChild::MakeVisible calls PuppetWidget::Show, which calls PuppetWidget::Invalidate here: https://searchfox.org/mozilla-central/rev/5ff2d7683078c96e4b11b8a13674daded935aa44/widget/PuppetWidget.cpp#228

So we should already have a PaintTask scheduled and should be able to tell the PuppetWidget to paint without scheduling another paint.

I think the paint suppression extra paint is necessary to avoid checkerboarding in the tab switch case, but we can probably avoid it in the warming case. I'll see if I can reorganize things so that TabChild can know _when_ it's warming, and then add a way of unsuppressing the displayport without queueing another paint.
(In reply to Mike Conley (:mconley) (:⚙️) (Catching up on needinfos / reviews) from comment #2)
> I think the paint suppression extra paint is necessary to avoid
> checkerboarding in the tab switch case, but we can probably avoid it in the
> warming case. I'll see if I can reorganize things so that TabChild can know
> _when_ it's warming, and then add a way of unsuppressing the displayport
> without queueing another paint.

Just a heads up: the work for this might overlap one of my tab layer cache patches[1]. tldr; I need a way of overriding the IsVisible check in RecvRenderLayers[2] to ensure an up-to-date paint.

I'm wondering if we can add something that suits both of our needs? Maybe an mBackground field on TabChild which we set before warming and when the tab is cached in the background, allowing us to avoid the unsuppressed-displayport paint in the warming case. Then the first paint after mBackground is removed could ignore the IsVisible check? The thought is a bit ugly though, so maybe you have something cleaner in mind.

[1] https://reviewboard.mozilla.org/r/244028/diff/1#index_header
[2] https://searchfox.org/mozilla-central/source/dom/ipc/TabChild.cpp#2617
Priority: -- → P2
This was originally fxperf:p1 because it was blocking shipping tab warming on macOS. Apparently, bug 1265824 was enough to let tab warming ship on that platform. I'm going to downgrade this to fxperf:p2, because I still think this is worth doing, but there's some higher priority stuff I'd like to get to first.
Whiteboard: [fxperf:p1] → [fxperf:p2]
Downgrading some more because AIUI it doesn't impact startup.
Whiteboard: [fxperf:p2] → [fxperf:p3]
You need to log in before you can comment on or make changes to this bug.