1360946 - Interruptible layout does not appear to be interrupted by force painting with e10s

Status: RESOLVED FIXED

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

Description

[Mike Conley (:mconley) (:⚙️)]

I _think_ I understand how and where interruptible reflows happen when using Gecko. My (perhaps low resolution) understanding is that these are layout calculations that only occur on initial page load, and that things like input events or maybe event a timeout can result in the reflow being interrupted and continued later.

I _suspect_ that tab switching is one of those things that isn't interrupting these interruptible reflows.

I captured a particularly long interruptible reflow in the profiler. I was loading a tryserver log in the background, and then immediately switched back to a treeherder tab.

I've highlighted the point where the content process (4 of 4) received the message to force paint:

https://perf-html.io/public/2135d14a1a93d86f407c56ad854092af412c2846/calltree/?range=58225.6535_59213.8689~59106.9992_59213.8689~59201.0060_59203.1183&thread=7

But here's the point where I requested the tab - right at the start of the reflow:

https://perf-html.io/public/07530fa654c4fbd641c9766894cd69a48e90c775/calltree/?callTreeFilters=prefix-0123456789zN6zXeQjFLpyHqxOpxwrCF3NpNqNrNsDK2O0R2O2O3CykDX2xU3zMazLdxC9BQuxCbAAnylx0GRhx2xU4xU5FEczI2xUmz3GRhx2xUeFEczI2xUmz3GRhx2xUfFEczI2xUmz3GRhx2xUgFEczI2xUmz3GRhx2xUk&thread=0

So the process hang monitor thread had to wait quite a while before it could cause the force paint. Rounding down aggressively, the tab switch request happened in the parent process around the 6s mark in the profile. The force paint occurred around 14s. That means I had to wait 8s for the force paint to occur[1].

In any case, I wonder if perhaps the JS interruption stuff that we do for tab switching can / should be extended to interrupt interruptible reflow as well. Or do we already do that, and I'm misunderstanding this profile?

[1]: I was doing a build in the background, so my machine was under pretty heavy load.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Does the above sound plausible, or am I off my rocker?

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Well, interruptible reflow means that we stop reflow at a specific interruption point and return back to the event loop. For it to work, we need to be able to dirty the frames that haven't been reflowed yet so that we can reflow them correctly the next time we reflow.

The stuff we do for interrupting JS is a bit different. We do the painting while the JS is still on the stack. We don't need to worry about dirtying stuff since we can just proceed where we left off when painting finishes. That, in theory, makes it easier to interrupt to paint: we don't need to save our state.

However, there are a couple problems:

First, you need to choose a good interruption point. Based on the profile, we spent 4 seconds in SplitAndInitTextRun. For your optimization to be useful, you would need to be able to interrupt that code. Honestly, I doubt we would ever spend so much time in this function if we weren't displaying an enormous text file (as you are). So I'm not sure that this is that realistic a scenario. In any case, I don't know much about that code, so I don't know how easy it would be to interrupt there.

In general, if you interrupt somewhere, you have to make sure that whatever state things are left in is a good state to paint in. I ran into this problem in https://bugzilla.mozilla.org/show_bug.cgi?id=1305130#c7. I suspect you may hit similar issues with what you're trying to do. Anyway, reading that bug might give you some insight into the difficulties you might see here.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Thinking about this a bit more, it's a lot easier to interrupt a TabGroup different than the one you intend to paint. It looks like that's the situation here. So this should be relatively easy to fix. It's also possible that this would naturally be fixed by Quantum DOM work.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1360946 - Consider a SetDocShellIsActive message a 'pending input event' so it can interrupt reflow.

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

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Ooooh boy this is pretty speculative. Basically, from what I can tell, HasPendingInputEvent checks the message queue for a TabChild periodically during layout to see if there are any incoming messages for user input - and if so, I think in some cases we can interrupt layout to paint.

This patch adds mozilla::dom::PBrowser::Msg_SetDocShellIsActive__ID to the list of message IDs that it will interrupt layout for. What this means is that if we've started layout in a background tab, and the user then selects that background tab, presumably, we'll have the opportunity to paint something sooner rather than later.

Interruptible reflow isn't something I super understand right now, but I thought I'd give this a shot.

Comment 6

[Ting-Yu Chou [:ting] (away)]

I am not sure if I am suitable to review the patch, but I have a few questions: 1) why do we layout a background tab before it is active, is it for pre-render? 2) when do we send SetDocShellIsActive? do we send it only when the tab is moving to foreground? 3) what if we make force paint the interruption point instead?

Comment 7

[Mike Conley (:mconley) (:⚙️)]

(In reply to Ting-Yu Chou [:ting] from comment #6)
> I am not sure if I am suitable to review the patch, but I have a few
> questions: 1) why do we layout a background tab before it is active, is it
> for pre-render?

I asked around in #content about this. There are a number of reasons - primarily, we want to make sure that layout is up to date even if the tab is not visible so that things like script that are running in the background tab can query for layout information properly, but also so that when the tab _does_ finally become active, we don't have to flush everything all at once.

> 2) when do we send SetDocShellIsActive? do we send it only
> when the tab is moving to foreground?

Yes, we send this whenever the user moves a tab into the foreground to become visible.

> 3) what if we make force paint the
> interruption point instead?

I guess we could do that, but I'm not sure how it'd work, really. Force Paint works by interrupting JavaScript from the ProcessHangMonitor thread... are you suggesting we somehow have some kind of bool between ProcessHangMonitor and PuppetWidget where of the ProcessHangMonitor receives the directive to force paint, it takes a lock on and flips the bool, and then when PuppetWidget wants to report possible input so that we can interrupt layout, it takes a lock and checks that bool? Something along those lines?

Or am I misunderstanding?

Comment 8

[Ting-Yu Chou [:ting] (away)]

(In reply to Mike Conley (:mconley) from comment #7)
> > 3) what if we make force paint the
> > interruption point instead?
> 
> I guess we could do that, but I'm not sure how it'd work, really. Force
> Paint works by interrupting JavaScript from the ProcessHangMonitor thread...
> are you suggesting we somehow have some kind of bool between
> ProcessHangMonitor and PuppetWidget where of the ProcessHangMonitor receives
> the directive to force paint, it takes a lock on and flips the bool, and
> then when PuppetWidget wants to report possible input so that we can
> interrupt layout, it takes a lock and checks that bool? Something along
> those lines?
> 
> Or am I misunderstanding?

No, you're right. I was just thinking if this is about force painting doesn't really "force" the paint, we should make it the interruption point instead of SetDocShellIsActive.

Sorry, but I still don't feel I am the right person to review this, I don't understand this part clearly like, there should be two SetDocShellIsActive when switch tab, will the foreground tab's reflow be interrupted by that, too?

Comment 9

[Ting-Yu Chou [:ting] (away)]

:bz, could you review this?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8871968 [details]
Bug 1360946 - Consider a SetDocShellIsActive message a 'pending input event' so it can interrupt reflow.

https://reviewboard.mozilla.org/r/143500/#review151092

r=me

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/73eefb81f8e1
Consider a SetDocShellIsActive message a 'pending input event' so it can interrupt reflow. r=bz

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/73eefb81f8e1