Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Slow down parsing of web pages in background tabs.

RESOLVED FIXED

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
a year ago

People

(Reporter: jst, Assigned: smaug)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs, {perf})

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Snappy:p1])

Attachments

(4 attachments, 3 obsolete attachments)

(Reporter)

Description

5 years ago
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

5 years ago
> - 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...
(Reporter)

Comment 2

5 years ago
(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.

Updated

5 years ago
Whiteboard: [Snappy] → [Snappy:p1]
Keywords: perf
(Assignee)

Comment 3

5 years ago
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.
Whoa. Those tiny tasks are *really* tiny. No wonder there are lots of memmoves and runnables.
(Assignee)

Comment 5

5 years ago
Created attachment 609823 [details] [diff] [review]
WIP2

Still just testing various things... based on about:jank it is 
often reflow which takes times.
Attachment #609758 - Attachment is obsolete: true
(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

5 years ago
> 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

5 years ago
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

5 years ago
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....
(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

5 years ago
I filed a new bug on the user switching : no 750785
https://bugzilla.mozilla.org/show_bug.cgi?id=750785
(Assignee)

Comment 12

5 years ago
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.
Attachment #609823 - Attachment is obsolete: true
(Assignee)

Comment 13

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=d3932a054ba5
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/opettay@mozilla.com-d3932a054ba5/
(Assignee)

Comment 14

5 years ago
Created attachment 625966 [details] [diff] [review]
single flush timer for background tabs

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

Comment 15

5 years ago
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?
Attachment #625966 - Flags: review?(hsivonen)
(Assignee)

Comment 16

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

Comment 17

5 years ago
(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.
(Assignee)

Comment 18

5 years ago
Created attachment 626344 [details] [diff] [review]
patch

Reuse gBackgroundFlushList more often.
Attachment #625966 - Attachment is obsolete: true
Attachment #625966 - Flags: review?(hsivonen)
Attachment #626344 - Flags: review?(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?
Attachment #626344 - Flags: review?(hsivonen) → review+
(Assignee)

Comment 20

5 years ago
(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.
(Assignee)

Comment 21

5 years ago
Changing the summary, and I'll file followups for other parts of this bug.
Summary: Slow down background tabs. → Slow down parsing of web pages in background tabs.
(Assignee)

Updated

5 years ago
Blocks: 757812
(Assignee)

Updated

5 years ago
Blocks: 757813
(Assignee)

Comment 22

5 years ago
Created attachment 626406 [details] [diff] [review]
patch
(Assignee)

Updated

5 years ago
Assignee: anygregor → bugs
(Assignee)

Comment 23

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

Feedback welcome. The patch shouldn't do miracles, only slow down background processing a tiny bit.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 25

5 years ago
Huh. I had pushed the patch several times to try.
Chances are that those tests where already flaky and introducing slack in the timing just made it more probable that they fail.
(Assignee)

Comment 27

5 years ago
Yeah, that is what I've planned to investigate.
(Assignee)

Comment 28

5 years ago
browser_tab_dragdrop.js is certainly buggy.
(Assignee)

Updated

5 years ago
Depends on: 760424
(Assignee)

Comment 29

5 years ago
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

Updated

5 years ago
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 30

5 years ago
I think I finally found the bug in browser_tabfocus.js.
It is racy.
(Assignee)

Updated

5 years ago
Depends on: 764540
(Assignee)

Comment 31

5 years ago
Created attachment 632842 [details] [diff] [review]
up-to-date
(Assignee)

Comment 32

5 years ago
https://hg.mozilla.org/mozilla-central/rev/e24844e280a0
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Depends on: 765041
No longer depends on: 765041
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.
Depends on: 766546

Updated

a year ago
Depends on: 1282215

Updated

a year ago
Depends on: 1293252
You need to log in before you can comment on or make changes to this bug.