906811 - don't enter favor perf mode for document loads in background tabs

Status: RESOLVED INVALID

(Core :: DOM: Navigation, defect)

Description

[Timothy Nikkel (:tnikkel)]

Attachment: patch

docshell makes us enter favor perf mode (process gecko events in preference to os events like painting) for 2 seconds whenever we start a document load, thus trading interactivity for (hopefully) a quicker overall page load. This trade off is of dubious value. We do this for document loads in any tab, including background tabs. Trading off interactivity of a foreground tab for page load performance in a background tab seems like a clearly bad idea to me.

Comment 1

[Timothy Nikkel (:tnikkel)]

This of course doesn't account for a tab becoming foreground after it starts loading but before the 2 seconds of favor perf mode would be up, but I think that's okay.

Comment 2

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

What if we just removed the whole performance favoring hint?
Do we have any data whether it actually affects to performance in a good way?

Comment 3

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

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

This would anyhow lead easily to the case where gNumberOfDocumentsLoading
becomes < 0, and then never go back to 0.

(Wouldn't surprise me if that might happen occasionally even without the patch.)

Comment 4

[Avi Halachmi (:avih)]

(In reply to Olli Pettay [:smaug] from comment #2)
> What if we just removed the whole performance favoring hint?

tn wanted to do it in steps, starting with bg tabs, and if it sticks, he'll remove it from the fg tab as well. Also, IMO, since it might cause regressions and possibly other issues, I think we should no-op the code using a pref, and rip it out later once we're happy with the result - at least for the fg tab case.

> Do we have any data whether it actually affects to performance in a good way?

tn did re-ran talos many times with the perf hint disabled: http://perf.snarkfest.net/compare-talos/index.html?oldRevs=4134d0810e32&newRev=b7637f90cdf8&submit=true

tn (from irc):
> favor perf seems to give us a 1-2% win on tp (ie removing it
> regresses b 1-2%) but is a big tp responsiveness win (hard
> to quantify because it has very high variance)

Comment 5

[Timothy Nikkel (:tnikkel)]

(In reply to Olli Pettay [:smaug] from comment #3)
> This would anyhow lead easily to the case where gNumberOfDocumentsLoading
> becomes < 0, and then never go back to 0.

Maybe I need coffee but I'm not seeing how this patch changes the values that gNumberOfDocumentsLoading takes.

Comment 6

[Timothy Nikkel (:tnikkel)]

(In reply to Olli Pettay [:smaug] from comment #2)
> What if we just removed the whole performance favoring hint?

That's the idea, but that looks like it causes a regressing on tp so I figured we could just do a simple first step that shouldn't be controversial at all and that might lead to the momentum needed to make the change once we see how it works out.

> Do we have any data whether it actually affects to performance in a good way?

It's hard to measure the responsiveness win exactly but it looks like it regresses tp on Windows and Linux by 1-2% and by 10-20% on mac (which is surprising why it is so different). So this is why we can rip it out wholesale but maybe we can take a small step in that direction and get some data to see if this makes things better or worse or no change.

Comment 7

[Timothy Nikkel (:tnikkel)]

(In reply to Timothy Nikkel (:tn) from comment #6)
> It's hard to measure the responsiveness win exactly but it looks like it
> regresses tp on Windows and Linux by 1-2% and by 10-20% on mac (which is
> surprising why it is so different).

Link for mac/linux numbers: http://perf.snarkfest.net/compare-talos/index.html?oldRevs=fe9f18b31705&newRev=c7814d0a7c92&submit=true

Comment 8

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

The patch would change the number of true values passed to
http://mxr.mozilla.org/mozilla-central/source/widget/xpwidgets/nsBaseAppShell.cpp?rev=c4d279b10128#182
but not false values.

And yes, nsBaseAppShell.cpp is super error prone, and I think I figured out today that it is causing
bug 822096.

Comment 9

[Timothy Nikkel (:tnikkel)]

(In reply to Olli Pettay [:smaug] from comment #8)
> The patch would change the number of true values passed to
> http://mxr.mozilla.org/mozilla-central/source/widget/xpwidgets/
> nsBaseAppShell.cpp?rev=c4d279b10128#182
> but not false values.

Ah, I forgot that appshell kept a counter and was just thinking it was a simple on/off switch controlled by passing true/false. I can make a slightly more complicated patch that keeps track of this so we at least don't introduce any more mismatches of FavorPerf calls.

Comment 10

[Timothy Nikkel (:tnikkel)]

Attachment: patch v2

Comment 11

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

Comment on attachment 795751 [details] [diff] [review]
patch v2

This doesn't work if there are two tabs loading simultaneously.
ActiveTab (at) starts, ++gNumberOfDocumentsLoading and FavorPerformanceHint(true) are called,
BackgroundTab (bt) start,  ++gNumberOfDocumentsLoading is called, 
at stops and --gNumberOfDocumentsLoading is called.
But since gNumberOfDocumentsLoading is now 1,
FavorPerformanceHint(false) isn't called.
bt stops,  gNumberOfDocumentsLoading gets to 0, but since 
mTurnOffFavorPerfMode is false, FavorPerformanceHint(false) isn't called.

Comment 12

[Timothy Nikkel (:tnikkel)]

Attachment: patch v3

I should probably actually put some thought into this.

Comment 13

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

Comment on attachment 796172 [details] [diff] [review]
patch v3

Sorry, still not happy :)
Since ContentSink also uses FavorPerformanceHint, this may actually lead to
worse performance if we end up calling FavorPerformanceHint(true) too often.

Could you just keep the counter and have mTurnOffFavorPerfMode.
only increase counter for active tabs, and then also set mTurnOffFavorPerfMode.
Then if mTurnOffFavorPerfMode is true when we're supposed to decrease the counter, decrease it and if 0, FavorPerformanceHint(false).

(And the whole FavorPerformanceHint is such a big mess. Well, not so big, since the amount of code involved is actually quite small.)

Comment 14

[Timothy Nikkel (:tnikkel)]

Attachment: patch v4

Right, good point. Such a bad api.

Comment 15

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3a1f27080f9c

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Backed out for causing various mochitest and Jetpack failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/404c12dddf28

https://tbpl.mozilla.org/php/getParsedLog.php?id=27142492&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=27142435&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=27142312&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=27142433&tree=Mozilla-Inbound'
https://tbpl.mozilla.org/php/getParsedLog.php?id=27141705&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=27141628&tree=Mozilla-Inbound

Comment 17

[Avi Halachmi (:avih)]

This system appears to be extremely fix/tamper-resistant ;)

Maybe it's time to tackle it head on and just find out what breaks whenever someone just breaths at it.

Comment 18

[Timothy Nikkel (:tnikkel)]

Just completely turning off favor perf in docshell is green on try (https://tbpl.mozilla.org/?tree=Try&rev=ccd795627d32) where the patch in this bug is not, which is interesting.

Comment 19

[Timothy Nikkel (:tnikkel)]

Interestingly the patch from bug 822096 actually fixes the orange caused by this patch.
https://tbpl.mozilla.org/?tree=Try&rev=63beaa0901bb

Comment 20

[Florian Bender]

Bug 930793 removed favor perf mode completely (afaics) – so I guess this bug is invalid / a dupe, then?

Comment 21

[Timothy Nikkel (:tnikkel)]

Yeah.

Comment 22

[Timothy Nikkel (:tnikkel)]

Attachment: unbitrot