Closed Bug 1573158 Opened 2 months ago Closed Last month

4.36 - 6.09% ts_paint_webext (windows10-64-shippable) regression on push bd7cd9d7b6ade7bf9fa8441a8686f19c36f2a7e5 (Thu August 8 2019)

Categories

(Toolkit :: XUL Widgets, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla71
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- unaffected
firefox69 --- unaffected
firefox70 --- wontfix
firefox71 --- fixed

People

(Reporter: alexandrui, Assigned: bgrins)

References

(Blocks 1 open bug, Regressed 1 open bug, Regression)

Details

(Keywords: perf, regression, talos-regression)

Attachments

(4 files, 2 obsolete files)

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

Flags: needinfo?(surkov.alexander)
Component: Performance → XUL Widgets
Product: Testing → Toolkit
Version: Version 3 → unspecified

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

Flags: needinfo?(alexandru.ionescu)

(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

Flags: needinfo?(alexandru.ionescu)

(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?

Flags: needinfo?(igoldan)

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

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.

Attachment #9088880 - Attachment description: Bug 1573158 - Target all menupopups within BMB_bookmarksPopup more directly instead of requiring a runtime check in JS → Bug 1573158 - Remove inline styles targeting BMB_bookmarksPopup in places-popup
See Also: → 1577592

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.

Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED

(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.

Attachment #9089162 - Attachment is obsolete: true
Attachment #9086139 - Attachment is obsolete: true
Blocks: 1555497
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cf727244feab
Move menupopup-drop-indicator styles back into browser.css using Shadow Parts and out of inline styles r=mak
https://hg.mozilla.org/integration/autoland/rev/3f54e52c52eb
Remove inline styles targeting BMB_bookmarksPopup in places-popup r=mak
https://hg.mozilla.org/integration/autoland/rev/c90add9d9f7f
Share one fragment across all consumers of places-popup r=surkov
https://hg.mozilla.org/integration/autoland/rev/f6703db1f552
Move Shadow DOM creation into constructor instead of connectedCallback r=surkov
Regressions: 1578569

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

Flags: needinfo?(igoldan)
Target Milestone: mozilla70 → mozilla71

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?

Flags: needinfo?(vchin)
Flags: needinfo?(bgrinstead)

(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.

Flags: needinfo?(bgrinstead)

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?

Flags: needinfo?(vchin)
Regressions: 1583531
You need to log in before you can comment on or make changes to this bug.