Last Comment Bug 734015 - Slow down parsing of web pages in background tabs.
: Slow down parsing of web pages in background tabs.
Status: RESOLVED FIXED
[Snappy:p1]
: perf
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: ---
Assigned To: Olli Pettay [:smaug]
:
: Andrew Overholt [:overholt]
Mentors:
Depends on: 1282215 760424 764540 766546 1293252
Blocks: 757812 757813
  Show dependency treegraph
 
Reported: 2012-03-08 00:24 PST by Johnny Stenback (:jst, jst@mozilla.com)
Modified: 2016-08-10 09:15 PDT (History)
29 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP, doesn't work well (6.99 KB, patch)
2012-03-27 09:48 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
WIP2 (11.67 KB, patch)
2012-03-27 11:58 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
user a timer for background parsing, decrease reflow interrupt timer (7.01 KB, patch)
2012-05-21 16:52 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
single flush timer for background tabs (7.41 KB, patch)
2012-05-22 04:24 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
patch (7.48 KB, patch)
2012-05-22 23:47 PDT, Olli Pettay [:smaug]
hsivonen: review+
Details | Diff | Splinter Review
patch (7.65 KB, patch)
2012-05-23 05:58 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
up-to-date (7.65 KB, patch)
2012-06-13 13:23 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review

Description Johnny Stenback (:jst, jst@mozilla.com) 2012-03-08 00:24:09 PST
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 [:bz] (still a bit busy) 2012-03-08 08:03:08 PST
> - 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, jst@mozilla.com) 2012-03-08 09:11:03 PST
(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] 2012-03-27 09:48:46 PDT
Created attachment 609758 [details] [diff] [review]
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) 2012-03-27 10:51:01 PDT
Whoa. Those tiny tasks are *really* tiny. No wonder there are lots of memmoves and runnables.
Comment 5 Olli Pettay [:smaug] 2012-03-27 11:58:23 PDT
Created attachment 609823 [details] [diff] [review]
WIP2

Still just testing various things... based on about:jank it is 
often reflow which takes times.
Comment 6 Henri Sivonen (:hsivonen) 2012-03-27 22:50:51 PDT
(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 [:bz] (still a bit busy) 2012-03-27 22:55:42 PDT
> 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 2012-05-01 09:46:30 PDT
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 [:bz] (still a bit busy) 2012-05-01 09:50:11 PDT
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] 2012-05-01 11:27:53 PDT
(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 2012-05-12 01:27:08 PDT
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] 2012-05-21 16:52:35 PDT
Created attachment 625822 [details] [diff] [review]
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 14 Olli Pettay [:smaug] 2012-05-22 04:24:08 PDT
Created attachment 625966 [details] [diff] [review]
single flush timer for background tabs

https://tbpl.mozilla.org/?tree=Try&rev=df36296fdf1f
Comment 15 Olli Pettay [:smaug] 2012-05-22 07:13:32 PDT
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] 2012-05-22 07:14:58 PDT
(I need to try the patch on a slow Windows laptop)
Comment 17 (dormant account) 2012-05-22 11:49:43 PDT
(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] 2012-05-22 23:47:10 PDT
Created attachment 626344 [details] [diff] [review]
patch

Reuse gBackgroundFlushList more often.
Comment 19 Henri Sivonen (:hsivonen) 2012-05-23 05:29:12 PDT
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] 2012-05-23 05:43:21 PDT
(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] 2012-05-23 05:51:13 PDT
Changing the summary, and I'll file followups for other parts of this bug.
Comment 22 Olli Pettay [:smaug] 2012-05-23 05:58:04 PDT
Created attachment 626406 [details] [diff] [review]
patch
Comment 23 Olli Pettay [:smaug] 2012-05-23 06:55:16 PDT
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 25 Olli Pettay [:smaug] 2012-05-24 03:19:28 PDT
Huh. I had pushed the patch several times to try.
Comment 26 Henri Sivonen (:hsivonen) 2012-05-29 05:08:53 PDT
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] 2012-05-29 05:39:24 PDT
Yeah, that is what I've planned to investigate.
Comment 28 Olli Pettay [:smaug] 2012-06-01 06:17:43 PDT
browser_tab_dragdrop.js is certainly buggy.
Comment 29 Olli Pettay [:smaug] 2012-06-01 07:17:34 PDT
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] 2012-06-13 13:03:56 PDT
I think I finally found the bug in browser_tabfocus.js.
It is racy.
Comment 31 Olli Pettay [:smaug] 2012-06-13 13:23:38 PDT
Created attachment 632842 [details] [diff] [review]
up-to-date
Comment 32 Olli Pettay [:smaug] 2012-06-14 11:39:20 PDT
https://hg.mozilla.org/mozilla-central/rev/e24844e280a0
Comment 33 Geoff Brown [:gbrown] 2012-06-20 12:13:08 PDT
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.

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