1508944 - 2.23 - 3.47% tp5o / tp5o_webext (linux64, linux64-qr, osx-10-10, windows10-64, windows10-64-qr, windows7-32) regression on push 209ef31926beef24da82d91223a89d26c712eccc (Tue Nov 20 2018)

Status: VERIFIED FIXED

(Firefox :: Protections UI, defect, P2)

Description

[Ionuț Goldan [:igoldan]]

Talos has detected a Firefox performance regression from push:

https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=fd9a2224eb523e00beccac04e958719eae6b478a&tochange=209ef31926beef24da82d91223a89d26c712eccc

As author of one of the patches included in that push, we need your help to address this regression.

Regressions:

  3%  tp5o windows10-64-qr opt e10s stylo         129.63 -> 134.14
  3%  tp5o windows10-64 opt e10s stylo            133.47 -> 137.43
  3%  tp5o osx-10-10 opt e10s stylo               229.17 -> 235.77
  3%  tp5o_webext windows10-64 opt e10s stylo     196.32 -> 201.94
  3%  tp5o windows10-64 pgo e10s stylo            119.80 -> 123.10
  2%  tp5o_webext windows7-32 pgo e10s stylo      174.54 -> 178.72
  2%  tp5o_webext windows7-32 opt e10s stylo      193.78 -> 198.21
  2%  tp5o linux64 pgo e10s stylo                 127.14 -> 130.01
  2%  tp5o linux64 opt e10s stylo                 141.44 -> 144.60
  2%  tp5o_webext linux64-qr opt e10s stylo       213.89 -> 218.65


You can find links to graphs and comparison views for each of the above tests at: https://treeherder.mozilla.org/perf.html#/alerts?id=17702

On the page above you can see an alert for each affected platform as well as a link to a graph showing the history of scores for this test. There is also a link to a treeherder page showing the Talos jobs in a pushlog format.

To learn more about the regressing test(s), please see: https://wiki.mozilla.org/Performance_sheriffing/Talos/Tests

For information on reproducing and debugging the regression, either on try or locally, see: https://wiki.mozilla.org/Performance_sheriffing/Talos/Running

*** Please let us know your plans within 3 business days, or the offending patch(es) will be backed out! ***

Our wiki page outlines the common responses and expectations: https://wiki.mozilla.org/Performance_sheriffing/Talos/RegressionBugsHandling

Comment 1

[Ionuț Goldan [:igoldan]]

I'll soon post the Gecko profiles.

Comment 2

[Ionuț Goldan [:igoldan]]

Here are the Gecko profiles for tp5o on Windows 10 OPT builds:

before: https://perf-html.io/from-url/https%3A%2F%2Fqueue.taskcluster.net%2Fv1%2Ftask%2FDm0-3wexQkGKte2UQOWxlg%2Fruns%2F0%2Fartifacts%2Fpublic%2Ftest_info%2Fprofile_tp5o.zip

after: https://perf-html.io/from-url/https%3A%2F%2Fqueue.taskcluster.net%2Fv1%2Ftask%2FBEAtzpSQTRCEkE0ooXi2Zw%2Fruns%2F0%2Fartifacts%2Fpublic%2Ftest_info%2Fprofile_tp5o.zip


For tp5o on Linux 64bit OPT builds:

before: https://perf-html.io/from-url/https%3A%2F%2Fqueue.taskcluster.net%2Fv1%2Ftask%2FF8VGP4oCRPeSUAkUjxJIYw%2Fruns%2F0%2Fartifacts%2Fpublic%2Ftest_info%2Fprofile_tp5o.zip

after: https://perf-html.io/from-url/https%3A%2F%2Fqueue.taskcluster.net%2Fv1%2Ftask%2FckbG-840SbGxarE_NvFL3g%2Fruns%2F0%2Fartifacts%2Fpublic%2Ftest_info%2Fprofile_tp5o.zip


For tp5o on OS X:

after: https://perf-html.io/from-url/https%3A%2F%2Fqueue.taskcluster.net%2Fv1%2Ftask%2FQHDqWRFKT8eTRG8C4zhjPg%2Fruns%2F0%2Fartifacts%2Fpublic%2Ftest_info%2Fprofile_tp5o.zip

before: https://perf-html.io/from-url/https%3A%2F%2Fqueue.taskcluster.net%2Fv1%2Ftask%2FW84l9lDVRBuUPRJzGF0vVw%2Fruns%2F0%2Fartifacts%2Fpublic%2Ftest_info%2Fprofile_tp5o.zip

Comment 3

[Ionuț Goldan [:igoldan]]

(In reply to Ionuț Goldan [:igoldan], Performance Sheriffing from comment #2)

> For tp5o on OS X:
> 
> after:
> https://perf-html.io/from-url/https%3A%2F%2Fqueue.taskcluster.
> net%2Fv1%2Ftask%2FQHDqWRFKT8eTRG8C4zhjPg%2Fruns%2F0%2Fartifacts%2Fpublic%2Fte
> st_info%2Fprofile_tp5o.zip
> 
> before:
> https://perf-html.io/from-url/https%3A%2F%2Fqueue.taskcluster.
> net%2Fv1%2Ftask%2FW84l9lDVRBuUPRJzGF0vVw%2Fruns%2F0%2Fartifacts%2Fpublic%2Fte
> st_info%2Fprofile_tp5o.zip

Oh, sorry. Got this wrong. For OS X, the correct profiles are these:

before: https://perf-html.io/from-url/https%3A%2F%2Fqueue.taskcluster.net%2Fv1%2Ftask%2FQHDqWRFKT8eTRG8C4zhjPg%2Fruns%2F0%2Fartifacts%2Fpublic%2Ftest_info%2Fprofile_tp5o.zip

after: https://perf-html.io/from-url/https%3A%2F%2Fqueue.taskcluster.net%2Fv1%2Ftask%2FW84l9lDVRBuUPRJzGF0vVw%2Fruns%2F0%2Fartifacts%2Fpublic%2Ftest_info%2Fprofile_tp5o.zip

Comment 4

[(no longer active)]

Thanks for the report.  This is expected in the sense that we are now doing more work, but it's also a huge regression which I'm not in a rush to just accept.  I'm discussing how to deal with this with my colleagues.  Maintaining the needinfo.

Comment 5

[(no longer active)]

Attachment: Bug 1508944 - Avoid dispatching the OnSecurityChange notification repeatedly when nothing has changed

Comment 6

[(no longer active)]

The patch attached is my attempt at fixing this regression without backing out the offending patch itself.  The approach is simple: if the top-level page already has the STATE_COOKIES_LOADED flag, just record the flag on our document and don't notify anything.  I came up with this approach when I noticed the content processes in the profiles after the change experiencing additional time running the OnSecurityChange callback coming from the nsGlobalWindowOuter::NotifyContentBlockingState() call site.

I pushed a couple of try pushes to verify that the regression does indeed go away:

Before: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d8d4fdcedd7ae7d0996ddbf5040113c72d33da73
After: https://treeherder.mozilla.org/#/jobs?repo=try&revision=59cf9a5ac928b010047b7c70eaf8f87a363e7fb3

Talos compare: https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=59cf9a5ac928b010047b7c70eaf8f87a363e7fb3&oldRevision=d8d4fdcedd7ae7d0996ddbf5040113c72d33da73

Comment 7

[(no longer active)]

OK, the first approach was promising:

<https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=d8d4fdcedd7ae7d0996ddbf5040113c72d33da73&newProject=try&newRevision=59cf9a5ac928b010047b7c70eaf8f87a363e7fb3&framework=1&showOnlyImportant=1>

But this wasn't completely recovering the performance we lost due to my original landing.  Then I decided to even go one step further and apply the same idea to everything that this function does and that indeed recovers all of the lost perf, and indeed buys a little bit more here or there too:

<https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=d8d4fdcedd7ae7d0996ddbf5040113c72d33da73&newProject=try&newRevision=f8ae453e84a881aa24c5fc922b9e71e3e082b407&framework=1&showOnlyImportant=1>

The updated patch is ready for review now.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d8cec61d53e8
Avoid dispatching the OnSecurityChange notification repeatedly when nothing has changed r=baku

Comment 9

[Noemi Erli[:noemi_erli]]

Backed out changeset d8cec61d53e8 (Bug 1508944) for failures in browser_trackingUI_trackers_subview.js

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&selectedJob=213443638&revision=d8cec61d53e80441e8b44eb02c0c914595d4c8ab

Failure log:https://treeherder.mozilla.org/logviewer.html#?job_id=213443638&repo=autoland&lineNumber=5235

Backout: https://hg.mozilla.org/integration/autoland/rev/77623171e512127f109569942fd4ae8468d2cf60

Comment 10

[(no longer active)]

Increased the timeout from 500ms to 1s, seems to make the test pass...

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8ab07484e939
Avoid dispatching the OnSecurityChange notification repeatedly when nothing has changed r=baku

Comment 12

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/8ab07484e939

Comment 13

[Joel Maher ( :jmaher ) (UTC -8)]

perf alerts are in:
== Change summary for alert #17780 (as of Fri, 23 Nov 2018 23:58:06 GMT) ==

Improvements:

  6%  tsvg_static windows7-32 pgo e10s stylo      45.61 -> 43.02
  3%  tp5o windows7-32 opt e10s stylo             135.05 -> 130.70
  3%  tp5o windows10-64-qr opt e10s stylo         133.55 -> 129.44
  3%  tp5o_webext windows7-32 pgo e10s stylo      178.56 -> 173.46
  3%  tp5o windows7-32 pgo e10s stylo             121.51 -> 118.21
  3%  tp5o windows10-64 opt e10s stylo            137.12 -> 133.52
  2%  tp5o_webext windows10-64 pgo e10s stylo     180.69 -> 176.47
  2%  tp5o_webext windows10-64 opt e10s stylo     201.43 -> 196.90

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=17780

Comment 14

[Johann Hofmann [:johannh]]

👍

But why did tsvg_static improve?

Comment 15

[Joel Maher ( :jmaher ) (UTC -8)]

oh, that is marked as "invalid", it is a noisy test affected by lower/higher volumes of pushes.