734015 - Slow down parsing of web pages in background tabs.

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Johnny Stenback (:jst)]

We already slow down firing of timeouts in background tabs, and we're working on improving that, and timeout firing in general in bug 715376. But there's plenty of things that we do in background tabs to slow down execution. Some ideas that have been floated are:

- Slow down parsing, i.e. delay feeding data from the parser to the main thread
- Slow down async reflows
- Slow down script evaluation, i.e. delay invocation of external scripts

There's probably more, but that seems like a good start. Feel free to propose additional things we could look into here. And this should probably act as a meta bug for this work, with specific bugs for the various items we'll end up working on here.

Comment 1

[Boris Zbarsky [:bzbarsky]]

> - Slow down parsing, i.e. delay feeding data from the parser to the main thread

That effectively means fewer longer main-thread events instead of more shorter ones, right?

> - Slow down async reflows

We already do; background tab refresh drivers are throttled.

> - Slow down script evaluation, i.e. delay invocation of external scripts

This is an interesting one...

Comment 2

[Johnny Stenback (:jst)]

(In reply to Boris Zbarsky (:bz) from comment #1)
> > - Slow down parsing, i.e. delay feeding data from the parser to the main thread
> 
> That effectively means fewer longer main-thread events instead of more
> shorter ones, right?

I think this would need to mean more short main-thread events. We want to keep the building of the DOM and layout thereof in background tabs from interfering with foreground tabs, at the expense of pageload times for background tabs being longer. Whether that means the parser feeds the main thread slower, or the main thread taking smaller bites off of the incoming data from the parser I dont't know, but one of the two. Speculative loading should probably continue to happen at the same pace though.

Comment 3

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

Attachment: WIP, doesn't work well

I started to hack something, just to try out various things.
This patch doesn't feel right. My guess is that we end up interrupting 
parser way too often, and that leads to tons of memmoves in parser and also
way too many runnables.

Perhaps parser should use a timer, not runnable when handling a parsing interrupt.

Comment 4

[Henri Sivonen (:hsivonen)]

Whoa. Those tiny tasks are *really* tiny. No wonder there are lots of memmoves and runnables.

Comment 5

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

Attachment: WIP2

Still just testing various things... based on about:jank it is 
often reflow which takes times.

Comment 6

[Henri Sivonen (:hsivonen)]

(In reply to Olli Pettay [:smaug] from comment #5)
> Still just testing various things... based on about:jank it is 
> often reflow which takes times.

Sorry I forgot to mention that. (A couple of years ago at least painting was also terribly expensive on Mac. In the frontmost tab at least but that's not a concern here.)

Now that incremental reflow exists, I wonder if reflows in background tabs could be inhibited altogether (when JS isn't forcing a synchronous reflow).

Comment 7

[Boris Zbarsky [:bzbarsky]]

> I wonder if reflows in background tabs could be inhibited altogether (when JS isn't
> forcing a synchronous reflow).

It'd take some hacking.

Right not reflows in a background tab are throttled (when not being forced from JS) to an initial delay of 1000ms, then doubling every time the refresh timer fires.

Comment 8

[AM]

Could this slow down be applied with multiple logins/fast switching.
With several users on a Mac several copies of firefox can be active.

I often see these background copies of firefox dominating the cpu usage-
leading to overheating (fans) and general slowness for all users

Comment 9

[Boris Zbarsky [:bzbarsky]]

Please file a separate bug on that?  When the user account changes, if we can detect that we should set all our stuff as inactive....

Comment 10

[Gregor Wagner [:gwagner]]

(In reply to AM from comment #8)
> Could this slow down be applied with multiple logins/fast switching.
> With several users on a Mac several copies of firefox can be active.
> 
> I often see these background copies of firefox dominating the cpu usage-
> leading to overheating (fans) and general slowness for all users

That's an interesting use-case and we should fix it. But I agree with Boris, this bug is about the multi-tab and not about the multi-instance problem.

Comment 11

[AM]

I filed a new bug on the user switching : no 750785
https://bugzilla.mozilla.org/show_bug.cgi?id=750785

Comment 12

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

Attachment: user a timer for background parsing, decrease reflow interrupt timer

This is simpler, hard to say how much this helps.
This machine is too fast for testing.

Comment 13

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

https://tbpl.mozilla.org/?tree=Try&rev=d3932a054ba5
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-d3932a054ba5/

Comment 14

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

Attachment: single flush timer for background tabs

https://tbpl.mozilla.org/?tree=Try&rev=df36296fdf1f

Comment 15

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

Comment on attachment 625966 [details] [diff] [review]
single flush timer for background tabs

This seems to help in certain cases. A bad, artificial case where this helps quite
a lot is loading http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp in several bg tabs.
But in general the effect isn't anything dramatic, at least not on this machine.

!NS_IsMainThread() check isn't needed in ContinueInterruptedParsingAsync.
The number 50 is magic. Not too high, not too low. Could be tweaked.
50 works still well with huge pages like HTML spec. The page loading doesn't get
really slow.

This is something where we need feedback. Does it really help with responsiveness?
Is it annoying to slow down bg tab loading?

Comment 16

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

(I need to try the patch on a slow Windows laptop)

Comment 17

[(dormant account)]

(In reply to Olli Pettay [:smaug] from comment #15)
> This is something where we need feedback. Does it really help with
> responsiveness?
> Is it annoying to slow down bg tab loading?

Hopefully bug 710935 will land soonish to help us test magic constants like this.

Comment 18

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

Attachment: patch

Reuse gBackgroundFlushList more often.

Comment 19

[Henri Sivonen (:hsivonen)]

Comment on attachment 626344 [details] [diff] [review]
patch

r+ if you add a comment explaining the magic number 50 to spare ourselves or people who come after us from Bugzilla and version control archaeology to figure out why the magic number is what it is.

Thank you for working on this.

I'm slightly curious, though: Why do background executors get inserted to the linked list only from ContinueInterruptedParsing and not also from nsHtml5StreamParser-queued nsHtml5ExecutorFlusher::Run? Also, is it intentional that an executor doesn't get removed from the linked list if it switches back to the foreground tab?

Comment 20

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

(In reply to Henri Sivonen (:hsivonen) from comment #19)
> I'm slightly curious, though: Why do background executors get inserted to
> the linked list only from ContinueInterruptedParsing and not also from
> nsHtml5StreamParser-queued nsHtml5ExecutorFlusher::Run?
That flusher runs already usually based on a timer tick, and I didn't bother to change the behavior,
except to check if the executor will be run anyway.
I basically wanted to focus on the case where constructing DOM may block main-thread too long.

> Also, is it
> intentional that an executor doesn't get removed from the linked list if it
> switches back to the foreground tab?
That is intentional. No need to complicate the code. The executor will be removed from the list
soon enough anyway.

Comment 21

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

Changing the summary, and I'll file followups for other parts of this bug.

Comment 22

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

Attachment: patch

Comment 23

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

https://hg.mozilla.org/mozilla-central/rev/8cf563a575fd

Feedback welcome. The patch shouldn't do miracles, only slow down background processing a tiny bit.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Unfortunately, this had to be backed out due to causing random mochitest-other orange.
https://hg.mozilla.org/mozilla-central/rev/d499dc65cdab

https://tbpl.mozilla.org/php/getParsedLog.php?id=11998664&tree=Firefox
https://tbpl.mozilla.org/php/getParsedLog.php?id=11990720&tree=Firefox
https://tbpl.mozilla.org/php/getParsedLog.php?id=12000852&tree=Firefox
https://tbpl.mozilla.org/php/getParsedLog.php?id=12002778&tree=Firefox
https://tbpl.mozilla.org/php/getParsedLog.php?id=11998674&tree=Firefox
https://tbpl.mozilla.org/php/getParsedLog.php?id=12002315&tree=Firefox

Comment 25

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

Huh. I had pushed the patch several times to try.

Comment 26

[Henri Sivonen (:hsivonen)]

Chances are that those tests where already flaky and introducing slack in the timing just made it more probable that they fail.

Comment 27

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

Yeah, that is what I've planned to investigate.

Comment 28

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

browser_tab_dragdrop.js is certainly buggy.

Comment 29

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

I *think* the editor/libeditor/base/tests/test_selection_move_commands.xul failure isn't related.
There are existing similar bugs open.

Still investigating browser_tabfocus.js

Comment 30

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

I think I finally found the bug in browser_tabfocus.js.
It is racy.

Comment 31

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

Attachment: up-to-date

Comment 32

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

https://hg.mozilla.org/mozilla-central/rev/e24844e280a0

Comment 33

[Geoff Brown [:gbrown]]

Android S1/S2 testing *may* be showing a regression in startup time caused by this patch. Infrastructure problems have severely reduced S1/S2 capacity, so all I have to go on is data from one phone, but that appears to show a 52 ms increase in startup time with this check-in. It's not a big regression, and very limited evidence, but I thought I would mention it.