Closed Bug 906811 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Document Navigation, defect)

defect
Not set

Tracking

()

RESOLVED INVALID

People

(Reporter: tnikkel, Assigned: tnikkel)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 3 obsolete files)

Attached patch patch (obsolete) — Splinter Review
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.
Attachment #792342 - Flags: review?(bugs)
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.
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?
Flags: needinfo?(avihpit)
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.)
Attachment #792342 - Flags: review?(bugs) → review-
(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)
Flags: needinfo?(avihpit)
(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.
(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.
(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
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.
Blocks: 880036
(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.
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #792342 - Attachment is obsolete: true
Attachment #795751 - Flags: review?(bugs)
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.
Attachment #795751 - Flags: review?(bugs) → review-
Attached patch patch v3 (obsolete) — Splinter Review
I should probably actually put some thought into this.
Attachment #795751 - Attachment is obsolete: true
Attachment #796172 - Flags: review?(bugs)
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.)
Attachment #796172 - Flags: review?(bugs) → review-
Attached patch patch v4Splinter Review
Right, good point. Such a bad api.
Attachment #796172 - Attachment is obsolete: true
Attachment #796355 - Flags: review?(bugs)
Attachment #796355 - Flags: review?(bugs) → review+
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.
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.
Interestingly the patch from bug 822096 actually fixes the orange caused by this patch.
https://tbpl.mozilla.org/?tree=Try&rev=63beaa0901bb
Bug 930793 removed favor perf mode completely (afaics) – so I guess this bug is invalid / a dupe, then?
Flags: needinfo?(tnikkel)
Yeah.
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: needinfo?(tnikkel)
Resolution: --- → INVALID
Attached patch unbitrotSplinter Review
You need to log in before you can comment on or make changes to this bug.