930793 - Remove favor performance mode

Status: RESOLVED WONTFIX

(Core :: Widget, defect)

Description

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

I can see some use for the perf hint on content processes, but hopefully we could
kill it elsewhere.

Comment 1

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

Attachment: patch

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

No idea what all this will break :)
But I think we should just do this (after fixing tons of tests I guess).

Comment 2

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

Attachment: Performance hint only in the content processes

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

Comment 3

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

Comment on attachment 822830 [details] [diff] [review]
Performance hint only in the content processes

>+++ b/dom/tests/mochitest/general/test_vibrator.html
>@@ -52,17 +52,17 @@ function testSuccesses() {
>   expectSuccess(1000);
>   expectSuccess(1000.1);
>   expectSuccess([0, 0, 0]);
>   expectSuccess(['1000', 1000]);
>   expectSuccess([1000, 1000]);
>   expectSuccess([1000, 1000.1]);
> 
>   // The following loop shouldn't cause us to crash.  See bug 701716.
>-  for (var i = 0; i < 10000; i++) {
>+  for (var i = 0; i < 1000; i++) {
>     navigator.vibrate([100, 100]);
>   }
So we end up posting a runnable for each these, and in case we're not in perf mode we
handle them slower than with perf mode.

>+++ b/dom/tests/mochitest/pointerlock/file_screenClientXYConst.html
>@@ -46,19 +46,19 @@ https://bugzilla.mozilla.org/show_bug.cg
>         is(unLockedCoords.screenY, lockedCoords.screenY,
>            "screenY should be equal to where the mouse was originaly locked");
>       }
> 
>       function moveUnlocked(e) {
>         var firstCall = !unLockedCoords;
>         if (!firstCall) {
>           todo(false, "mousemove is fired twice.");
>+        } else {
>+          isUnlocked = !document.mozPointerLockElement;
>         }
>-
>-        isUnlocked = !document.mozPointerLockElement;
I think the issue here is just racy test. Test expects gecko-only events, but there can be also native events.

But let's see what the tryserver tells.

Comment 4

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

Attachment: Max 15ms for os events

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

Comment 5

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

Attachment: Max 10ms for os events

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

Comment 6

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

And the browser_Troubleshoot.js failure is something that happened on m-i too but was fixed there
after I had updated my tree.

Comment 7

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

Attachment: only_content_process_favor_perf_hint_5.diff

Trying to reduce tp regression by allowing a bit more gecko event processing.
Haven't tested yet how this affects to responsiveness.

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

Comment 8

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

Comment on attachment 822901 [details] [diff] [review]
only_content_process_favor_perf_hint_5.diff

Doesn't seem to affect to anything and this was a test anyway.

Comment 9

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

different kind of approach coming.

Comment 10

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

Attachment: remove_favor_perf_2.diff

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

Locally this behaves well on Linux, but no idea about tp numbers.

Added also some code for windows since it isn't necessarily using omtc.
Other platforms should use it, I was told, so no need for the Update.

Comment 11

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

(But I don't have windows dev environment, so no idea whether the code even compiles there :) 
 If it does compile, will test trybuilds)


Matt, this is the bug I was talking on #gfx. Do the nsWindow changes look even close to reasonable?

Comment 12

[Avi Halachmi (:avih)]

Comment on attachment 831896 [details] [diff] [review]
remove_favor_perf_2.diff

Review of attachment 831896 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/xpwidgets/nsBaseAppShell.cpp
@@ +253,5 @@
>    mProcessedGeckoEvents = false;
>  
> +  if (start - mLastNativeEventTime > limit) {
> +    mLastNativeEventTime = start;
> +    DoProcessNextNativeEvent(false, recursionDepth);

Didn't you want to use the if part rather than the else? It looks like the else part which you kept is the perf mode where native events are throttled...

Comment 13

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

No, because this way I can keep good tp results, and the fix to RefreshDriver handling
ensures user visible jank stays still ok.

Comment 14

[Avi Halachmi (:avih)]

tp results are not an absolute benchmark to not regress IMO. We do expect some tp regressions because more work will happen before the page is loaded (e.g. painting it few times maybe, handling user inputs if they occur, etc). This is by design - perf mode throttle native events, and we want to un-throttle them.

The part which you kept is clearly throttling native events (forcing |limit| to pass before handling them again), of which OnPaint is one, BTW (on windows), and not handling OnPaint will not update the screen. Same goes for mouse inputs I'd think, so if indeed it's throttled, then it has a potential to hurt responsiveness compared to "non-perf" mode before the patch.

You also modified limit from 20 to 10ms, so maybe 10ms between handling native events is OK, and maybe it isn't. As you mentioned on IRC, we don't currently have a framework to measure responsiveness from input event to paint (hopefully bug 935680 will help with that).

I'm just saying we should understand how that limit condition affects behavior, preferably at the code level together with some actual hands-on tests, and especially since to me it looks like it's a regression compared to "normal" mode before the patch.

Comment 15

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

tp is not an absolute benchmark, but it is a benchmark.

Also, I didn't actually say we don't have framework to measure from input to
paint, but from invalidate to paint. Currently there are cases when we're processing input and
Invalidating, but we just don't let OS to paint. And we have no tools to detect that.

In practice the patch shouldn't regress behavior, since we force the Update, so user actually
sees something happening on the screen. That is the (or a) real jank we have now, UI not updating.

I'm about to upload a patch which tries to process a native event whenever gecko event is processed.

Comment 16

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

Attachment: remove_favor_perf_3.diff (always process native events)

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

Comment 17

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

Attachment: remove_favor_perf_4.diff (Same as v2 but with possible Windows fix)

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

Comment 18

[Matt Woodrow (:mattwoodrow)]

(In reply to Olli Pettay [:smaug] from comment #11)
> (But I don't have windows dev environment, so no idea whether the code even
> compiles there :) 
>  If it does compile, will test trybuilds)
> 
> 
> Matt, this is the bug I was talking on #gfx. Do the nsWindow changes look
> even close to reasonable?

If it works! :)

Looks sane to me anyway.

Comment 19

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

Seems to work and fixes bug 822096 and bug 880036.

https://tbpl.mozilla.org/?tree=Try&rev=cf7ea2fa86f6
v3 + some test fixes.

Comment 20

[Avi Halachmi (:avih)]

So the original code had 2 modes at nsBaseAppShell::OnProcessNextEvent(..) :

Normal mode:
- On each entry, as long as there are more native events pending, they are processed in a loop of DoProcessNextNativeEvent for up to limit ms.

Perf mode:
- On each entry, call DoProcessNextNativeEvent once, but only if limit ms have elapsed since the last native event was processed.


In patch v2 from comment 10, it's exactly like perf mode before the patches (but with the Update() calls).

In patch v3, it's like perf mode, but DoProcessNextNativeEvent is called once on each entry rather than only if limit ms have elapsed (also with the Update() calls).


Now, the Update() calls help with flushing, but they don't help increase native events processing.

To me it still seems that normal mode before the patch can process much more native events than patch v3 (unless a typical single call to DoProcessNextNativeEvent lasts more than limit ms - which I think is unlikely).

So I think there are two potential results for this:

1. It'll paint more than in perf mode before the patch, because of the added Update() calls and extra processing of Native events (each entry rather than only after limit ms elapsed).

2. Compared to normal mode, it'll potentially paint less and native events might queue up, making the browser less responsive on cases where many native events are queued, because it calls DoProcessNextNativeEvent only once on each entry rather than in a loop of upto limit ms on each entry.

Comment 21

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

(In reply to Avi Halachmi (:avih) from comment #20)

> Now, the Update() calls help with flushing, but they don't help increase
> native events processing.
Yes they do. Calling Update() explicitly makes non-OMTC platforms to update the
screen asap, instead of waiting native event loop to update UI later.


> To me it still seems that normal mode before the patch can process much more
> native events than patch v3 (unless a typical single call to
> DoProcessNextNativeEvent lasts more than limit ms - which I think is
> unlikely)
there aren't usually many pending native events.


> 
> So I think there are two potential results for this:
> 
> 1. It'll paint more than in perf mode before the patch, because of the added
> Update() calls and extra processing of Native events (each entry rather than
> only after limit ms elapsed).
yes

> 
> 2. Compared to normal mode, it'll potentially paint less and native events
> might queue up, making the browser less responsive on cases where many
> native events are queued, because it calls DoProcessNextNativeEvent only
> once on each entry rather than in a loop of upto limit ms on each entry.
No, it won't paint less, since we Invalidate based on RefreshDriver.
Native events are possibly processed slower _if_ we don't have many Gecko events.
But even then, we want the UI to update. So processing tons of native events, but not showing
the results to the user is effectively jank.

Comment 22

[Avi Halachmi (:avih)]

(In reply to Olli Pettay [:smaug] from comment #21)
> > To me it still seems that normal mode before the patch can process much more
> > native events than patch v3 (unless a typical single call to
> > DoProcessNextNativeEvent lasts more than limit ms - which I think is
> > unlikely)
> there aren't usually many pending native events.

So your patch not only removes perf mode, but also modifies the behavior of normal mode to possibly process less native events (compared to gecko events). If we have good data or understanding to support also this change, then fine, otherwise, it has a potential to regress (not on tp tests which never measured in normal mode).

Comment 23

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

We usually have lots of Gecko events, so we end up processing lots of native events to with v3.
And if we don't have Gecko events, we're idle from Gecko point of view and just process native events.

Just calling DoProcessNextNativeEvent(false, recursionDepth); always is almost enough
to fix the jank-y scrolling I get (bug 822096), but not enough to fix bug 880036 properly.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

If the main reason to process native events more is to make painting happen in a timely manner, OMTC will make that unnecessary. And we could make it less necessary without OMTC, by calling UpdateWindow() at the end of nsRefreshDriver::Tick on Windows.

Comment 25

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

The patch makes us call UpdateWindow() at the end of Tick. 
And AFAIK OMTC is not very close to be enabled on linux, so need to fix bug 822096
and bug 880036 even without OMTC.


I prefer (v3) attachment 832162 [details] [diff] [review] which gives good compromise between gecko and native events and
forces timely updates on non-omtc platforms. That is the patch I'll ask review for once I've
fixed bug 931641 (and possibly some other racy tests).

Comment 26

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

Attachment: v3.1

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

Comment 27

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

Comment on attachment 8340525 [details] [diff] [review]
v3.1

Actually, I think asking review for this now is ok.
I'll need to still look at some random failures on linux, but they all
seem to be racy tests.

Because the patch needs to go in as one, splitting it doesn't make much sense.
Favor-perf-mode removal is just removing code, and reviewer should see the
big picture here anyway.
The basic idea is to remove favor-perf-mode and always try to process one native
event per Gecko event. And to get visual updates fast enough, call Update() from refresh driver. Update() should be in general no-op with omtc.

The patch regresses tp a bit but helps with tp_responsiveness, and 
bug 822096 and bug 880036 are fixed.

This all is scary enough that it should go in early in a cycle.

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8340525 [details] [diff] [review]
v3.1

Review of attachment 8340525 [details] [diff] [review]:
-----------------------------------------------------------------

Can't the code to update the window during the refresh driver tick be factored out into its own patch?

Comment 29

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

I can do that.

(not sure it is useful, since I wouldn't land it without the other stuff.)

Comment 30

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

Attachment: RefreshDriver to Update

Comment 31

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

Attachment: remove favor perf, 3.1

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8341298 [details] [diff] [review]
RefreshDriver to Update

Review of attachment 8341298 [details] [diff] [review]:
-----------------------------------------------------------------

::: view/public/nsViewManager.h
@@ +305,5 @@
>    /**
>     * Flush the accumulated dirty region to the widget and update widget
>     * geometry.
>     */
> +  void ProcessPendingUpdates(bool aTryToSyncUpdate);

Please make this a flags word or an enum so we don't have to decipher what ProcessPendingUpdates(true) means.

::: widget/cocoa/nsChildView.h
@@ +469,5 @@
>  
>    virtual int32_t         RoundsWidgetCoordinatesTo() MOZ_OVERRIDE;
>  
>    NS_IMETHOD              Invalidate(const nsIntRect &aRect);
> +  NS_IMETHOD              Update();

just make this virtual void.

Comment 33

[Avi Halachmi (:avih)]

Comment on attachment 8341300 [details] [diff] [review]
remove favor perf, 3.1

Review of attachment 8341300 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/xpwidgets/nsBaseAppShell.cpp
@@ +9,5 @@
>  #include "nsThreadUtils.h"
>  #include "nsIObserverService.h"
>  #include "nsServiceManagerUtils.h"
>  #include "mozilla/Services.h"
> +#include "nsBaseWidget.h"

Still needed?

Comment 34

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

Attachment: 3.1.1

Comment 35

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

Attachment: v3.1.2

Right kind of NS_OBJC_END_TRY_ABORT*

Comment 36

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/347d5704cbe1

Comment 37

[Ryan VanderMeulen [:RyanVM]]

Backed out for frequent Linux mochitest-bc failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/54293b840bf4

https://tbpl.mozilla.org/php/getParsedLog.php?id=33046746&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=33033795&tree=Mozilla-Inbound

Comment 38

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

https://tbpl.mozilla.org/php/getParsedLog.php?id=33033795&tree=Mozilla-Inbound is a nice case
where gdk after resize returns first the correct data and then for some reason the old data.

Comment 39

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/80b64a63b524

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/80b64a63b524

Comment 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Hooray!

Comment 42

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

Attachment: patch for backout

...because trying to fix Android 2.2 regression revealed that we have racy
b2g reftests and what not.

Comment 43

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

Comment on attachment 8368173 [details] [diff] [review]
patch for backout

backout, not backup

Comment 44

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

http://hg.mozilla.org/integration/mozilla-inbound/rev/89f9304ff4ba

I'll reland early next week (and continue fixing the racy b2g tests before that :/ ).

Comment 45

[Daniel Holbert [:dholbert]]

(In reply to Olli Pettay [:smaug] from comment #44)
> I'll reland early next week (and continue fixing the racy b2g tests before
> that :/ ).

Did this end up relanding? (or maybe the racy b2g tests were racier than anticipated?)

Comment 46

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

I haven't found time to fix b2g reftest framework, not some more tests.

But I will..

Comment 47

[Avi Halachmi (:avih)]

Note that bug 959281 and bug 996848 also touch heavily the events processing system. It'd probably be wise to not try landing them in parallel.

Comment 48

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

Attachment: 2015-v1

Trying to get this in again

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

Comment 49

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

Attachment: 2015-v2

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

Comment 50

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/1b8f36a7ee32

crossing fingers

Comment 51

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1b8f36a7ee32

Comment 52

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/a5a7035ebf21

Comment 53

[Bob Clary [:bc] (inactive)]

This also appeared on Autophone Throbber stop for local-blank. The backout confirms it.
http://phonedash.mozilla.org/#/org.mozilla.fennec/throbberstop/local-blank/norejected/2015-04-12/2015-04-13/notcached/errorbars/standarderror/notry

Did not appear/was not significant  for local-twitter or webappstartup though.

Comment 54

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

Attachment: 2015-v3

Some new tryserserver  try: -b o -p linux,linux64,macosx64,win32,win64 -u none -t all pushes
without the patch, clean m-i: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ab7f32def79
with the patch, same m-i + patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ce8adeefcea

Comment 55

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

Attachment: 2015-v3.1

https://treeherder.mozilla.org/#/jobs?repo=try&revision=033c5f3e23b1
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5231b5e4ee26

Comment 56

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

Looks like any new native event processing during window open regresses tpaint on linux
(and in fact if native event processing is reduced from what it is now on m-c, we get even better results from that test).

An issue seems to be that we get more OnExposeEvent calls, so, more painting.

Comment 57

[Avi Halachmi (:avih)]

(In reply to Olli Pettay [:smaug] from comment #55)
> Created attachment 8627464 [details] [diff] [review]
> 2015-v3.1
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=033c5f3e23b1
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=5231b5e4ee26

After smaug retriggers those tests 5 more times, we ended up with this compare-view:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=033c5f3e23b1&newProject=try&newRevision=5231b5e4ee26

It shows some relatively small ( < 5% ) improvements and regressions, and one supposedly meaningful (25%) tpaint regression on linux only.

smaug thinks this is not an actual regression but rather a weakness of tpaint which got exposed by the patch of this bug (where tpaint measured the first content paint which possibly happened with the first chrome paint before the patch, but now chrome paint happens earlier, but tpaint still measures the first content paint). That's bug 1179377.

Comment 58

[Avi Halachmi (:avih)]

IMO we should try to land this and accept the regressions (and improvements) as they manifest at comment 57 (several up to 5% improvements and regressions).

Including the tpaint linux regression if it ends up to be real (which currently seems like not a real regression).

Comment 59

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

Well, it is a regression, but somewhat expected one. We don't block Gecko from interacting with the
OS level stuff so much with the patch, so we end up processing certain things faster, which may lead to process Gecko internal things slower.
But the tpaint itself does indeed seem to be a bit too racy, and measuring a bit wrong things.

Comment 60

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

try: -b do -p all -u all -t all
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b2f5a9d7d4e

Comment 61

[Bob Clary [:bc] (inactive)]

Note that phonedash urls have changed with the addition of the new mean vs min display, you need to append /mean to the end of the url. 

The new is:

http://phonedash.mozilla.org/#/org.mozilla.fennec/throbberstop/local-blank/norejected/2015-04-12/2015-04-13/notcached/errorbars/standarderror/notry/mean

Comment 62

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

Attachment: WIP: Bug 930793, Remove favor performance mode, (year 2022 edition)

Comment 63

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 64

[Suhaib Mujahid [:suhaib]]

Sorry, there was a problem with the detection of inactive users. I'm reverting the change.

Comment 65

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

I think I've actually found a use case for performance mode. Need to just remove first all the old callers and then tweak the API.
So, marking this wontfix and will open new bugs