1514731 - 3.3 - 4.61% tp5o_webext (linux64, linux64-qr, windows10-64, windows10-64-qr, windows7-32) regression on push f4ae445c27170b47c330b7e5091063dc43d408ca (Sat Dec 15 2018)

Status: VERIFIED FIXED

(Testing :: General, defect)

Description

[Florin Strugariu [:Bebe]]

Talos has detected a Firefox performance regression from push:

https://hg.mozilla.org/integration/autoland/pushloghtml?changeset=f4ae445c27170b47c330b7e5091063dc43d408ca

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

Regressions:

  5%  tp5o_webext opt e10s stylo     211.61 -> 221.36
  4%  tp5o_webext opt e10s stylo     190.40 -> 198.42
  4%  tp5o_webext pgo e10s stylo     174.53 -> 181.77
  4%  tp5o_webext opt e10s stylo     209.39 -> 217.94
  4%  tp5o_webext pgo e10s stylo     186.77 -> 194.15
  4%  tp5o_webext opt e10s stylo     196.08 -> 203.67
  3%  tp5o_webext pgo e10s stylo     177.64 -> 183.79
  3%  tp5o_webext opt e10s stylo     193.32 -> 199.70


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

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]]

(In reply to Florin Strugariu [:Bebe] from comment #0)
> Regressions:
> 
>   5%  tp5o_webext opt e10s stylo     211.61 -> 221.36
>   4%  tp5o_webext opt e10s stylo     190.40 -> 198.42
>   4%  tp5o_webext pgo e10s stylo     174.53 -> 181.77
>   4%  tp5o_webext opt e10s stylo     209.39 -> 217.94
>   4%  tp5o_webext pgo e10s stylo     186.77 -> 194.15
>   4%  tp5o_webext opt e10s stylo     196.08 -> 203.67
>   3%  tp5o_webext pgo e10s stylo     177.64 -> 183.79
>   3%  tp5o_webext opt e10s stylo     193.32 -> 199.70

Because of a reporting flaw, the regressions don't mention the platforms.
Here are the complete results:

Regressions:

  5%  tp5o_webext linux64-qr opt e10s stylo     211.61 -> 221.36
  4%  tp5o_webext windows10-64-qr opt e10s stylo     190.40 -> 198.42
  4%  tp5o_webext windows7-32 pgo e10s stylo     174.53 -> 181.77
  4%  tp5o_webext linux64 opt e10s stylo     209.39 -> 217.94
  4%  tp5o_webext linux64 pgo e10s stylo     186.77 -> 194.15
  4%  tp5o_webext windows10-64 opt e10s stylo     196.08 -> 203.67
  3%  tp5o_webext windows10-64 pgo e10s stylo     177.64 -> 183.79
  3%  tp5o_webext windows7-32 opt e10s stylo     193.32 -> 199.70

Comment 2

[Ionuț Goldan [:igoldan]]

I'll try to post the Gecko profiles.

Comment 3

[Ionuț Goldan [:igoldan]]

Unfortunately, I didn't have any luck. Looks like it's the same problem experienced here [1] and stated in bug 1514257.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1514257#c0

Comment 4

[Shane Caraveo (:mixedpuppy)]

Does this mean the reporting is inaccurate and not a result of the changes in bug 1437258?  We were looking at it yesterday, trying to even get tp5o_webext to run (non obvious).  It's hard to imagine the changes in that patch causing this level of regression.

Comment 5

[Shane Caraveo (:mixedpuppy)]

Here's a recent run with some minor changes, but even without those changes, the code paths here dont make sense to have caused that much regression.

https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=85cf888dafaa8c1a8373729b0124fce6612947d9

Comment 6

[tossj]

I believe I have found the cause of this regression, and I have a potential solution. I have to check it with :mixedpuppy, but it should be able to be fixed by the end of 2018-12-21.

Comment 7

[Ionuț Goldan [:igoldan]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #4)
> Does this mean the reporting is inaccurate and not a result of the changes
> in bug 1437258?  We were looking at it yesterday, trying to even get
> tp5o_webext to run (non obvious).  It's hard to imagine the changes in that
> patch causing this level of regression.

No, the reporting is accurate. It's just that it was incomplete.

Comment 8

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1514731 - Move to API schema wrappers the webRequestBlocking permission checks for webRequest blocking listeners. r?mixedpuppy!,tossj

This patch includes the following changes:

- move into the API schema wrappers the additional checks needed (in the child process) to ensure that we throw a validation error when
  a webRequest blocking listener is registered by an extension that doesn't have the required webRequestBlocking permission:
  - define a new webRequestBlockingPermissionRequired postprocessor
  - add to web_request.json API schema the `"postprocess": "webRequestBlockingPermissionRequired"` property
    to all the webRequest options types that allows "blocking" as a valid string value (currently
    OnBeforeRequestOptions, OnBeforeSendHeadersOptions, OnHeadersReceivedOptions and OnAuthRequiredOptions)

- add back an additional check for the webRequestBlocking permission in parent/ext-webRequest.js
  (just in case a new webRequest event is added in the future and we forget to add the postprocess
  property, so that we still ensure that an extension without the webRequestBlocking permission
  is not able to register any blocking listener)

- tweak the test_no_webRequestBlocking_error test case to use browser.test.assertThrows to explicitly check
  that the error is thrown and "catchable" by the extension code, and not just logged in the console
  messages, and extend it to run on all the webRequest API events that can be blocking.

Comment 9

[Luca Greco [:rpl] [:luca] [:lgreco]]

As I've been discussing over IRC with :tossj, the patch I just attached seems to give talos results similar to the patch that :tossj was preparing, in :tossj's patch fire.sync was being changed to fire.raw, and I think that @tossj analysis was right and fire.sync was likely the part that was introducing a good chunk of the performance regression detected by the talos (which makes sense because fire.sync clones the data into the extension context, which should also be unnecessary because that data is coming from a parent event and so it has been already cloned in the extension context when the StructuredCloneHolder has been deserialized)  

Nevertheless, we agreed (@tossj and I) that we both liked this alternative strategy a bit more than the one we are currently using and so I proceeded to attach the patch.   

@mixedpuppy as a blocking reviewer on the patch, and @tossj as an additional reviewer (e.g. to double-check that this version is still providing the intended behavior in the exact same way of the current version).

Comment 10

[tossj]

:rpl, I don't have the permissions to accept the phabricator revision, but it is working as intended; the error message is displaying on the extension debugging console when an extension with "blocking" doesn't have the webRequestBlocking permission.

Comment 11

[Luca Greco [:rpl] [:luca] [:lgreco]]

At the following url the results from running talos tests on the attached patch and compare its results against the results on mozilla-central (over the last 2 days):

https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=ff4a893467987e6d7943550c4c7a260e5c2273c6&framework=1&filter=webext&showOnlyComparable=1&showOnlyImportant=1&selectedTimeRange=172800

Comment 12

[Luca Greco [:rpl] [:luca] [:lgreco]]

The related push to try also looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff4a893467987e6d7943550c4c7a260e5c2273c6

Comment 13

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to tossj from comment #10)
> :rpl, I don't have the permissions to accept the phabricator revision, but
> it is working as intended; the error message is displaying on the extension
> debugging console when an extension with "blocking" doesn't have the
> webRequestBlocking permission.

Thanks for double-checking it for me (and also for digging into which part of the original patch may have contributed to the regression, that wasn't an easy task ;-)).

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c9413e3a5702
Move to API schema wrappers the webRequestBlocking permission checks for webRequest blocking listeners. r=mixedpuppy

Comment 15

[Narcis Beleuzu [:NarcisB]]

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

Comment 16

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

I see an improvement:
== Change summary for alert #18383 (as of Thu, 20 Dec 2018 19:51:51 GMT) ==

Improvements:

  3%  tp5o_webext windows7-32 opt e10s stylo     201.24 -> 194.43

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

I assume this is spread across all platforms, I saw windows10 alert on inbound, just not on autoland.

Comment 17

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

here are the other platforms:
== Change summary for alert #18377 (as of Thu, 20 Dec 2018 18:31:18 GMT) ==

Improvements:

  5%  tp5o_webext linux64-qr opt e10s stylo          221.28 -> 210.75
  4%  tp5o_webext windows7-32 pgo e10s stylo         183.35 -> 175.33
  4%  tp5o_webext linux64 pgo e10s stylo             194.43 -> 186.11
  4%  tp5o_webext linux64 opt e10s stylo             217.45 -> 209.13
  4%  tp5o_webext windows10-64 pgo e10s stylo        183.76 -> 177.18
  3%  tp5o_webext windows10-64 opt e10s stylo        203.55 -> 196.61
  3%  tp5o_webext windows10-64-qr opt e10s stylo     198.35 -> 192.17

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