1745720 - 81.79% speedometer Preact-TodoMVC/DeletingItems/Sync (Windows) regression on Tue November 23 2021

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Andra Esanu (needinfo me)]

Perfherder has detected a browsertime performance regression from push 0fb0990124cfd4d05e81e3a78971b10525ef5c31. As author of one of the patches included in that push, we need your help to address this regression.

Regressions:

Ratio	Test	Platform	Options	Absolute values (old vs new)
82%	speedometer Preact-TodoMVC/DeletingItems/Sync	windows10-64-shippable-qr	webrender	1.56 -> 2.83

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests. Please follow our guide to handling regression bugs and let us know your plans within 3 business days, or the offending patch(es) will be backed out in accordance with our regression policy.

For more information on performance sheriffing please see our FAQ.

Comment 1

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

Set release status flags based on info from the regressing bug 1739929

Comment 2

[Jan de Mooij [:jandem]]

Gijs, does this regression range make sense?

Comment 3

[:Gijs (he/him)]

(In reply to Jan de Mooij [:jandem] from comment #2)

Gijs, does this regression range make sense?

It makes no logical sense to me. I think the only web-observable change here is related to handling untrusted (not user-generated) click events, which would now load and hit a JSWindowActor that wouldn't be hit before. But the graph seems pretty clear: https://treeherder.mozilla.org/perfherder/graphs?highlightAlerts=1&highlightChangelogData=1&highlightCommonAlerts=0&selected=3390936,1449691014&series=autoland,3390936,1,13&timerange=2592000&zoom=1637604825305,1637721810887,1.127073246101928,3.2286931054701213 .

Does this speedometer test use untrusted click events? I don't know how to find out - I looked at https://firefox-source-docs.mozilla.org/testing/perfdocs/raptor.html#speedometer-b but it provides pretty much no meaningful information.

Comment 4

[:Gijs (he/him)]

(but even if it does - why would it regress only this subtest, and/or why only this platform?)

Comment 5

[Mayank Bansal]

quite a few have regressed :
Angular2-TypeScript-TodoMVC
Angular2-TypeScript-TodoMVC/CompletingAllItems
Angular2-TypeScript-TodoMVC/CompletingAllItems/Sync
Angular2-TypeScript-TodoMVC/DeletingItems
Angular2-TypeScript-TodoMVC/DeletingItems/Sync
AngularJS-TodoMVC/CompletingAllItems
AngularJS-TodoMVC/CompletingAllItems/Sync
BackboneJS-TodoMVC
BackboneJS-TodoMVC/CompletingAllItems
BackboneJS-TodoMVC/CompletingAllItems/Sync
BackboneJS-TodoMVC/DeletingAllItems/Sync
Elm-TodoMVC/DeletingItems/Sync (maybe)
EmberJS-TodoMVC/CompletingAllItems/Sync
etc.

Comment 6

[:Gijs (he/him)]

(I also once again have to ask why it is taking so long for perf regressions to be reported - the regressing change fixes google link handling compatibility and is already in beta, and hearing about such a clear perf regression 3 weeks later is really unfortunate - perhaps at the time, backing it out and finding an alternative fix could have been considered, but it's probably too late for that at this point)

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Just some random thoughts, this may be expensive in my experience:
https://searchfox.org/mozilla-central/rev/21a9b72545da06681db97c4b3a2a6be761f4aae5/toolkit/modules/BrowserUtils.jsm#167-169,174

Perhaps, adding chrome only UI event may make this faster.

And if href is null, only when button is 1, the created objects after calling BrowserUtils.hrefAndLinkNodeForClickEvent is used. I.e., in the other cases which are not the target of the case of bug 1739929 wastes the cost of unnecessary object creations.

Anyway, untrusted events are required only when clicking in a link so that it should do nothing as far as possible in the other cases (anyway, optimizing here would affect benchmark score is interesting to me).

Comment 8

[:Gijs (he/him)]

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #7)

Just some random thoughts, this may be expensive in my experience:
https://searchfox.org/mozilla-central/rev/21a9b72545da06681db97c4b3a2a6be761f4aae5/toolkit/modules/BrowserUtils.jsm#167-169,174

I don't think we would call that in this test (though I haven't verified it) - the events from the test are untrusted and have no user gesture, so https://searchfox.org/mozilla-central/rev/71befbc9bc348d33f0c7307dc48e3cb36ba78650/browser/actors/ClickHandlerChild.jsm#67-70,72 should bail out before we hit it, right?

Anyway, untrusted events are required only when clicking in a link so that it should do nothing as far as possible in the other cases (anyway, optimizing here would affect benchmark score is interesting to me).

It'd be better to move all of this into C++ from a general performance perspective, for a number of reasons, but we don't really have time to do that for 96.

Can someone clarify how bad this regression is, realistically speaking? It seems like this is a peculiar kind of microbenchmark so it doesn't seem representative of the real world, but perhaps my understanding there is wrong, or perhaps we care even though it's unrealistic?

Comment 9

[:Gijs (he/him)]

Can someone clarify how to get the values in the subtest when running the online version at https://browserbench.org/Speedometer2.0/ ? Clicking "test details" just shows "runs", but not the per-subtest values, it would seem.

I also tried to run with raptor, and that failed with:

Missing browsertime files...attempting to install
OSError(8, '%1 is not a valid Win32 application', None, 193, None)

Comment 10

[Jan de Mooij [:jandem]]

(In reply to :Gijs (he/him) from comment #9)

Can someone clarify how to get the values in the subtest when running the online version at https://browserbench.org/Speedometer2.0/ ? Clicking "test details" just shows "runs", but not the per-subtest values, it would seem.

Try the interactive runner: https://browserbench.org/Speedometer2.0/InteractiveRunner.html

Comment 11

[:Gijs (he/him)]

https://treeherder.mozilla.org/jobs?repo=try&revision=a982a750b553b3caa150cfcf51411e68896c6fb1

Comment 12

[:Gijs (he/him)]

(In reply to :Gijs (he/him) from comment #11)

https://treeherder.mozilla.org/jobs?repo=try&revision=a982a750b553b3caa150cfcf51411e68896c6fb1

I've confirmed locally that this fixes the perf regression, and isn't entirely pointless in that it'll avoid creating the window actor in fission for all but Real Link Clicks (tm), which maybe/hopefully saves us some memory. And maybe/hopefully this makes it easier to move the logic entirely into C++ at a later point (might be easier if we fix where places gets its link data (bug 1742894) + making it easier for C++ to open new tabs/windows, first).

However, there's a risk of regressions if there are cases where we don't hit the link handling in Element while we currently have click/auxclick events that hit the frontend code. I don't think this should be possible but I'm not certain. Hopefully the trypush helps smoke those out.

Comment 13

[:Gijs (he/him)]

Attachment: Bug 1745720 - use 'chromelinkclick' command event to avoid instantiating ClickHandlerChild except for link clicks, r?smaug!,mconley!

Instead of relying on untrusted click/auxclick events anywhere
instantiating the actor, and then having to look for links, after this
patch we'll only instantiate the actor for actual link clicks. This
patch moves to using a chrome-only CustomEvent dispatched from the
link click post-visitor to accomplish that. In future we should
probably move both this and the middle-click-to-paste handling into
DOM code (or, for the latter, remove it) but this is a less invasive
solution.

This also moves the middle-click-to-paste handling into its own
listener. It needs to listen to page events in general (not just
links) but is disabled everywhere by default, so registering an
actor for everyone doesn't seem like a good trade-off. To avoid
duplicating all the logic (we do need to avoid doing middle-click
navigation based on the clipboard when clicking on links!), as
well as keeping patch size down, the actual control flow goes
through the click handler actor still.

Comment 14

[Jan de Mooij [:jandem]]

Clearing NI. Thanks for working on this, Gijs!

Comment 15

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to :Gijs (he/him) from comment #6)

(I also once again have to ask why it is taking so long for perf regressions to be reported - the regressing change fixes google link handling compatibility and is already in beta, and hearing about such a clear perf regression 3 weeks later is really unfortunate - perhaps at the time, backing it out and finding an alternative fix could have been considered, but it's probably too late for that at this point)

In this case it's because the overall Speedometer result was not impacted (or not enough to generate an alert). We are not currently alerting for any subtests on this suite, and it was noticed by the reporter of bug 1745630 based on manual inspection of the results on arewefastyet.com. In response to that bug, I asked the sheriffs to create a manual alert and open a bug for investigation. Speaking with :denispal, we may be reviewing the way we summarise Speedometer, which could improve our detection of subtest regressions, or we may enable reporting for a selection of subtests.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/26a792be5ec3
use 'chromelinkclick' command event to avoid instantiating ClickHandlerChild except for link clicks, r=smaug,mconley

Comment 17

[Atila Butkovits]

Backed out for causing failures complaining about Event.cpp

Backout link: https://hg.mozilla.org/integration/autoland/rev/ffcf1ba6b92bf2ea100c1622a09bc278207d8cb0

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=26a792be5ec35f69881393f138cc5496d7994717

Failure log:
-https://treeherder.mozilla.org/logviewer?job_id=363947537&repo=autoland&lineNumber=4084
-https://treeherder.mozilla.org/logviewer?job_id=363945793&repo=autoland&lineNumber=8554
-https://treeherder.mozilla.org/logviewer?job_id=363945699&repo=autoland&lineNumber=2500
-https://treeherder.mozilla.org/logviewer?job_id=363946388&repo=autoland&lineNumber=5507

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fd3872532e71
use 'chromelinkclick' command event to avoid instantiating ClickHandlerChild except for link clicks, r=smaug,mconley

Comment 19

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/fd3872532e71

Comment 20

[:Gijs (he/him)]

https://treeherder.mozilla.org/perfherder/graphs?highlightAlerts=1&highlightChangelogData=1&highlightCommonAlerts=0&series=mozilla-central,3412569,1,13&timerange=5184000 suggests that this is genuinely fixed. \o/

Comment 21

[Jan de Mooij [:jandem]]

Great, thanks for fixing this! :)

Comment 22

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

The patch landed in nightly and beta is affected.
:Gijs, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 23

[:Gijs (he/him)]

Given that we ended up shipping the perf regression in 96, and given that this reworks the way in which we handle clicks in a non-trivial fashion, and given that the perf regression was somewhat theoretical (ie to do with untrusted-click events, when "real" user use would have caused trusted click events, which always had this overhead), even if the fix also improves perf for "real" click events that aren't on links, I think we should have this bake on 98 rather than increase risk for 97.