Talos has detected a Firefox performance regression from push:

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

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

Regressions:

6% ts_paint_webext windows10-64-shippable opt e10s stylo 324.08 -> 343.83
4% ts_paint_webext windows10-64-shippable opt e10s stylo 322.83 -> 336.92

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

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/TestEngineering/Performance/Talos

For information on reproducing and debugging the regression, either on try or locally, see: https://wiki.mozilla.org/TestEngineering/Performance/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/TestEngineering/Performance/Talos/RegressionBugsHandling

Alexandru, looking some information about webext talos tests. Do you know what webext tests are about and why those only were regressed?

(In reply to alexander :surkov (:asurkov) from comment #3)

Alexandru, looking some information about webext talos tests. Do you know what webext tests are about and why those only were regressed?

The alert summary update in the meantime. From what I can see, now even the old ts_paint regressed, not only its webext variant.
For more details about these 2 types of tests, check https://wiki.mozilla.org/TestEngineering/Performance/Talos/Tests

The patch doesn't show perf improvements [1], but anyway it's something nice to have. The graph looks somewhat weird though, I'll rerun it.

[1] https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=ee074afe8d0745b892489206e84d44ff61ab6684&newProject=try&newRevision=0edd8deb7525a648e7901f269654c33a3b1fde14&framework=1&showOnlyConfident=1

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

(In reply to alexander :surkov (:asurkov) from comment #3)

Alexandru, looking some information about webext talos tests. Do you know what webext tests are about and why those only were regressed?

The alert summary update in the meantime. From what I can see, now even the old ts_paint regressed, not only its webext variant.

Now it makes sense. Is there a chance to record perf profiles to make comparison of them, to see where time goes?

(In reply to alexander :surkov (:asurkov) from comment #7)

Now it makes sense. Is there a chance to record perf profiles to make comparison of them, to see where time goes?

I requested some profiles on Windows 10 & Windows 7. Let's wait for their results.

This also regressed on awsy:
== Change summary for alert #22874 (as of Wed, 28 Aug 2019 10:55:47 GMT) ==

Regressions:

3% Heap Unclassified windows7-32-shippable opt 37,764,748.33 -> 38,815,016.82
3% Heap Unclassified windows7-32-shippable opt 37,833,635.69 -> 38,874,482.74

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

This reverts removals like https://hg.mozilla.org/mozilla-central/rev/bd7cd9d7b6ad#l8.12

This will allow us to share the fragment across all consumers instead of only applying the styles
to elements matching .closest("BMB_bookmarksPopup").

Depends on D43844

This will avoid parsing the markup strings into fragments for every consumer.

Depends on D43845

Depends on D43846

I made a set of changes to move inline styles out of the Shadow DOM and back into css files, then to share a fragment across all places-popup instances which seems promising on talos: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=aa4648a59ba33bb88fac04d18022909f76b54833&newProject=try&newRevision=3501970b96dd3a99eafe4edb9dc867b336e3ef9d&framework=1&hideUncertain=1.

In the meantime, reset the [part] attribute to itself after appending into the Shadow Root.

Depends on D43847

The numbers on these patches look good: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=aa4648a59ba33bb88fac04d18022909f76b54833&newProject=try&newRevision=b2dd06f6a7b3d196aae21ffaac4e7ff903977dbe&showOnlyImportant=1.

The patches themselves are pretty low risk and should be OK to land in 70. The most obvious potential regressions here are mostly styling (padding, specifically) so should be upliftable.

(In reply to Brian Grinstead [:bgrins] from comment #16)

The numbers on these patches look good: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=aa4648a59ba33bb88fac04d18022909f76b54833&newProject=try&newRevision=b2dd06f6a7b3d196aae21ffaac4e7ff903977dbe&showOnlyImportant=1.

The patches themselves are pretty low risk and should be OK to land in 70. The most obvious potential regressions here are mostly styling (padding, specifically) so should be upliftable.

Actually, as Marco mentioned in https://phabricator.services.mozilla.com/D43845#1333059 it may be better to hold wait to land it until after the merge to get a chance to confirm there aren't regressions.

https://hg.mozilla.org/mozilla-central/rev/cf727244feab
https://hg.mozilla.org/mozilla-central/rev/3f54e52c52eb
https://hg.mozilla.org/mozilla-central/rev/c90add9d9f7f
https://hg.mozilla.org/mozilla-central/rev/f6703db1f552

== Change summary for alert #22940 (as of Tue, 03 Sep 2019 10:32:34 GMT) ==

Improvements:

3% ts_paint_webext windows10-64-shippable opt e10s stylo 338.62 -> 328.33
3% ts_paint windows7-32-shippable opt e10s stylo 342.92 -> 332.58

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

Brian, this seems a bit risky for beta uplift. What do you think?
Vicky, can we accept this regression for 70 and wait for the improvement in 69 or do you think it's important to try to get this into 70?

(In reply to Liz Henry (:lizzard) from comment #21)

Brian, this seems a bit risky for beta uplift. What do you think?
Vicky, can we accept this regression for 70 and wait for the improvement in 69 or do you think it's important to try to get this into 70?

All things equal I'd prefer to let it ride the train, but would be willing to request the uplift if it's a priority for Vicky.

The things that make an uplift not super risky are:
a) the changes are mainly targeted to a non-default popup (the bookmarks popup which you'd have to drag in from Customize mode).
b) regressions are likely to be only visual in nature and not functional.

But the thing that does make it risky is that it's just hard to track down all potential CSS / visual regressions (for instance bug 1578569). So we might get into a state where we end up needing to uplift additional CSS patches over time as this gets more eyes on it.

I'm ok with letting this ride the trains to get fixed in 71 so as to not add risk to the 70 release.

Did the awsy regression also get resolved?