1520850 - Investigate: Force paint if there is long running JS and vsync is pending?

Status: RESOLVED WONTFIX

(Core :: Layout, enhancement, P3)

Description

[Olli Pettay [:smaug][[e-mail]]]

(Not sure about the component)
The idea here is to figure out if we could force paint if there is long running script and we have done initial reflow yet but not first contentful paint.

The code could reuse the tab switching force painting code.

Comment 1

[Jim Porter (:squib)]

:smaug, is there anywhere I should look at for figuring out how to tell when we've done the initial reflow but haven't painted? I've looked over bug 1279086, but it's not quite clear to me how I'd detect if we should force paint in this case. If you have any sites in mind that you think this might help with, that would help too!

Feel free to redirect this NI to anyone else you think might be better to ask, of course. :)

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

(commented on IRC)

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Attachment: slow_script_flushing.html

Maybe this could work as a testcase

Comment 5

[Jim Porter (:squib)]

Attachment: Non-working patch for discussion

The logging in this patch tells me I had a reflow but no contentful paint for every page, which might be true (I don't know enough about when the initial reflow happens to say), but it's definitely not useful! Is there something obvious I'm missing here?

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9046847 [details] [diff] [review]
Non-working patch for discussion

 
>+  win->GetExtantDoc()->GetShell()->PostReflowCallback(
>+    new TestingCallback{win->GetExtantDoc()}
>+  );
I guess this should check only the very top level content document.
So, perhaps win->GetExtantDoc()->GetTopLevelContentDocument()
See https://searchfox.org/mozilla-central/rev/2a6f3dde00801374d3b2a704232de54a132af389/dom/base/Document.cpp#11172

And the callback gets probably called on most of the documents, since it is possibly set very early, so, even before initial reflow or so.

Could you check here whether the document has presshell (GetShell()) and that has prescontext and not first contentful paint and that FramesReflowedCount() return > 0?
Only in such case we should even try to pain.

Though, I guess we need some new flag. Each time refreshdriver runs, it should clear some flag or counter on prescontext, so we count only reflows since then:
we don't want to try to paint all the time, only if something may have changed.

Comment 7

[Jim Porter (:squib)]

Attachment: Use a different strategy (seems to work)

This patch uses a different strategy than before and seems to work for the demo page in this bug at least. The general idea is this:

• Wait until we hit the InterruptCallback for JS so we know we're in a long-running script
• Check whether we had a contentful paint yet (for the demo page, this is false)
• Check how many reflows we've had (for the demo page, this is typically >2000 in my tests)
• (Probably not needed, since it's covered by the previous bullet) check if we still have NS_FRAME_FIRST_REFLOW on the primary frame (for the demo page, this is false)

:smaug, I just wanted to do a quick sanity check to make sure that this roughly what you were thinking. If this looks about right, I'll tweak the debug printfs to only log "bad" pages, and then I can start testing out different scheduling strategies.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

FramesReflowedCount is of course frames which have been reflowed, not reflow count as such.
I'd expect in the test page to be only one full reflow as such.
But yeah, there are reflows without painting, and then, could we actually force paint.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9050851 [details] [diff] [review]
Use a different strategy (seems to work)

Have you tested whether normal web pages trigger this case,
reflows without first contentful paint.

Comment 10

[Jim Porter (:squib)]

:smaug, So far, I haven't found any pages that trigger this, since most pages don't hit InterruptScript during load. That's probably a good thing, since it means that we shouldn't be getting too many false positives. The demo does trigger this case, but of course it's possible that this method would have false negatives...

I'll keep working on this and try to get a patch up that actually does the force-painting to see how it works.

Comment 11

[Jim Porter (:squib)]

Attachment: (Attempt to) identify sites where this bug would improve things

This patch logs any pages that would be improved by fixing this bug. It correctly identifies attachment 9042782 [details] as something that would be improved. However, I tested all the TP6 sites on mobile, and none of them were logs as possibly-fixed. A few (Reddit and AllRecipes) had long JS during load, but this happened after the first contentful paint.

Do you have any ideas here? Maybe we could be more aggressive in considering what counts as "long-running JS", or maybe it just means that this isn't a significant problem in practice. If we should be more aggressive, do you have any pointers for how I'd do that in a patch?

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Ok, if this would help only rarely, perhaps we shouldn't spend more time on this right now.

Comment 13

[Jim Porter (:squib)]

Unassigning this from myself...

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

and I guess given the results of the investigation, this is wontfix at least for now.