Closed Bug 1377242 Opened 7 years ago Closed 7 years ago

2.06 - 2.75% cart / tart (linux64) regression on push ba616afa8125a467295c08418d0dd9e1d69d303b (Wed Jun 28 2017)

Categories

(Firefox :: General, defect)

Product:
Firefox
Component:
General
Version:
53 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 56
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- unaffected
firefox55 --- unaffected
firefox56 --- fixed

People

(Reporter: jmaher, Assigned: bgrins)

References

Details

(Keywords: perf, regression, talos-regression, Whiteboard: [LP_M1]) 

Talos has detected a Firefox performance regression from push:

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

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

Regressions:

  3%  cart summary linux64 opt e10s     21.17 -> 21.76
  2%  cart summary linux64 pgo e10s     18.22 -> 18.61
  2%  tart summary linux64 opt e10s     5.05 -> 5.16


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

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/Buildbot/Talos/Tests

For information on reproducing and debugging the regression, either on try or locally, see: https://wiki.mozilla.org/Buildbot/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/Buildbot/Talos/RegressionBugsHandling
:heycam, I see that you fixed the build breakage (which looks like your first push aused the build breakage).  Unfortunately this regression occurred while the build was broken!  Do you think your code might be related to a linux 64 cart regression?
Flags: needinfo?(cam)
:kats, I see you authored a patch in this range, can you take a look at your patch from bug 1376311 to see if these regressions are related to your code?
Flags: needinfo?(bugmail)
:nechen, I see you authored a patch for bug 1373174, can you comment if your patch might be related to the linux64 cart regressions?
Flags: needinfo?(cnevinchen)
The pushlog in comment doesn't include any of these bugs.
No longer blocks: 1376311
Flags: needinfo?(bugmail)
I'm not sure why bug 1366199 as been referenced in the dependency tree here. It has nothing to do with Firefox and should not affect anything because it was just a version bump.
No longer blocks: 1366199
(But even assuming the range in comment 3 is correct, the webrender_traits change I made would not have affected talos, as webrender is not enabled when talos is run).
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #5)
> The pushlog in comment doesn't include any of these bugs.

That should be "... in comment 0" :)
:kats, it is not trivial to do that right now- you were just too fast- so your needinfo is still valid
Flags: needinfo?(bugmail)
 m_kato@ga2.so-net.ne.jp, I see you authored a patch in bug 1375910, can you comment if this caused a linux64 cart regression?
Flags: needinfo?(m_kato)
:gijs, I see you authored a patch in bug 1373968, can you comment if this is related to a linux64 tart regression.
Flags: needinfo?(gijskruitbosch+bugs)
all bugs referenced here are referenced because they are in a range where the regression occurred- unfortunately we cannot backfill due to a broken build. consider it like winning the lottery :)
:bgrins I see you authored a patch in bug 1366851, can you comment if this could cause the linux64 tart regressions.
Flags: needinfo?(bgrinstead)
Bug 1374999 might make AllChildrenIterators slightly slower.  I figured the flag check in bug 1376647 should negate that, though we still will check this new content property if the flag is set for some other reason (e.g. having anonymous children from XBL bindings).

Bug 1372061 is code that mostly only affect stylo builds, so it doesn't seem relevant to "linux64 opt e10s" or "linux64 pgo e10s".  Apart from the addition of the flag setting that bug 1374999 also happens to use.

Joel, are you able to push a try run just with bug 1374999 backed out?  Or tell me what my try syntax should be?
Flags: needinfo?(cam) → needinfo?(jmaher)
(In reply to Joel Maher ( :jmaher) from comment #9)
> :kats, it is not trivial to do that right now- you were just too fast-

Assuming you were replying to comment 5/comment 8 there.

> so your needinfo is still valid

repeating from comment 7: the webrender_traits change I made would not have affected talos, as webrender is not enabled when talos is run.

(Sorry for the rapid flurry of comments)
Flags: needinfo?(bugmail)
(In reply to Joel Maher ( :jmaher) from comment #10)
>  m_kato@ga2.so-net.ne.jp, I see you authored a patch in bug 1375910, can you
> comment if this caused a linux64 cart regression?

This effect is input.value setter only (to set empty value from non-empty) with focus.  I don't think that this is root cause.
Most situation improves performance by this fix.  Doesn't autoland run talos?
Flags: needinfo?(m_kato)
(In reply to Cameron McCormack (:heycam) from comment #14)
> Bug 1374999 might make AllChildrenIterators slightly slower.  I figured the
> flag check in bug 1376647 should negate that, though we still will check
> this new content property if the flag is set for some other reason (e.g.
> having anonymous children from XBL bindings).
> 
> Bug 1372061 is code that mostly only affect stylo builds, so it doesn't seem
> relevant to "linux64 opt e10s" or "linux64 pgo e10s".  Apart from the
> addition of the flag setting that bug 1374999 also happens to use.
> 
> Joel, are you able to push a try run just with bug 1374999 backed out?  Or
> tell me what my try syntax should be?

How I usually do it is:

* ./mach try -b o -p linux,linux64,macosx64,win32,win64 -u none -t all --rebuild-talos 5
* backout patches
* ./mach try -b o -p linux,linux64,macosx64,win32,win64 -u none -t all --rebuild-talos 5

Load this url, replacing FIRST with the rev in the first push and SECOND with the rev in the second push: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=FIRST&newProject=try&newRevision=SECOND&framework=1&showOnlyImportant=0

I'd be interested if Joel has a better way though
(In reply to Joel Maher ( :jmaher) from comment #11)
> :gijs, I see you authored a patch in bug 1373968, can you comment if this is
> related to a linux64 tart regression.

It could potentially explain a cart regression because it changes the styling of various bits inside customize mode.

I don't see how it would have anything to do with the tart regression though.
My patch is all on Android. I don't think it's related.
Flags: needinfo?(cnevinchen)
(In reply to Joel Maher ( :jmaher) from comment #13)
> :bgrins I see you authored a patch in bug 1366851, can you comment if this
> could cause the linux64 tart regressions.

Maybe?  It's adding a new CSS transform on a toolbar button.  Here's an ongoing try push with it backed out: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=73b5364f54dc18fde08bb1b45d4453060e34cdc3&newProject=try&newRevision=3f08f6aa4fd899001b37590e0887b80630bb3363&framework=1&showOnlyImportant=0
Flags: needinfo?(bgrinstead)
Component: Untriaged → General
Flags: needinfo?(cam)
A backout from 1373968 doesn't apply cleanly to current m-c, so I just used current m-c and also backed out the depending bug 1354145.

m-c with patches from bug 1373968 and bug 1354145 backed out ('baseline'):
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=e30643e0b4f0057ec391bb38268aaecee39e5914


m-c with patches from bug 1354145 backed out (to see if bug 1373968 regressed something compared to the previous patch):
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=1cf9f27e2782969245b0e1871b7e0584302dc4ac


m-c right now (to see if bug 1354145 helped improve things):
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=fae54563bd020c4af17a94ba92c86ef430ad2fbf
(In reply to :Gijs from comment #22)
> A backout from 1373968 doesn't apply cleanly to current m-c, so I just used
> current m-c and also backed out the depending bug 1354145.
> 
> m-c with patches from bug 1373968 and bug 1354145 backed out ('baseline'):
> remote:  
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=e30643e0b4f0057ec391bb38268aaecee39e5914
> 
> 
> m-c with patches from bug 1354145 backed out (to see if bug 1373968
> regressed something compared to the previous patch):
> remote:  
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=1cf9f27e2782969245b0e1871b7e0584302dc4ac
> 
> 
> m-c right now (to see if bug 1354145 helped improve things):
> remote:  
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=fae54563bd020c4af17a94ba92c86ef430ad2fbf


So the cart regression was definitely bug 1373968:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=e30643e0b4f0057ec391bb38268aaecee39e5914&newProject=try&newRevision=1cf9f27e2782969245b0e1871b7e0584302dc4ac&framework=1&showOnlyImportant=0

but bug 1354145 seems to have fixed things beyond the original regression

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=1cf9f27e2782969245b0e1871b7e0584302dc4ac&newProject=try&newRevision=fae54563bd020c4af17a94ba92c86ef430ad2fbf&framework=1&showOnlyImportant=0


Neither of these show a substantial difference on tart, so perhaps that was caused by one of the other bugs here? jmaher, is there still a substantial lasting tart regression in addition to the cart alert?
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Cameron McCormack (:heycam) from comment #23)
> I haven't yet tried a backout of bug 1374999.

Sorry, meant to say I haven't yet tried a backout of bug 1372061.
(In reply to Brian Grinstead [:bgrins] from comment #20)
> (In reply to Joel Maher ( :jmaher) from comment #13)
> > :bgrins I see you authored a patch in bug 1366851, can you comment if this
> > could cause the linux64 tart regressions.
> 
> Maybe?  It's adding a new CSS transform on a toolbar button.  Here's an
> ongoing try push with it backed out:
> https://treeherder.mozilla.org/perf.html#/
> compare?originalProject=try&originalRevision=73b5364f54dc18fde08bb1b45d445306
> 0e34cdc3&newProject=try&newRevision=3f08f6aa4fd899001b37590e0887b80630bb3363&
> framework=1&showOnlyImportant=0

I do see a minor tart improvement, but it has low confidence. So I pushed up a patch for comparison that doesn't use the transform to see if there's an improvement:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=308897cf901b6a01b305cc5303a34148a5ab0392&newProject=try&newRevision=ba5475976365b66bd7e4ec4b5bf179e5714ceb45&framework=1&showOnlyImportant=0
(In reply to Brian Grinstead [:bgrins] from comment #26)
> (In reply to Brian Grinstead [:bgrins] from comment #20)
> > (In reply to Joel Maher ( :jmaher) from comment #13)
> > > :bgrins I see you authored a patch in bug 1366851, can you comment if this
> > > could cause the linux64 tart regressions.
> > 
> > Maybe?  It's adding a new CSS transform on a toolbar button.  Here's an
> > ongoing try push with it backed out:
> > https://treeherder.mozilla.org/perf.html#/
> > compare?originalProject=try&originalRevision=73b5364f54dc18fde08bb1b45d445306
> > 0e34cdc3&newProject=try&newRevision=3f08f6aa4fd899001b37590e0887b80630bb3363&
> > framework=1&showOnlyImportant=0
> 
> I do see a minor tart improvement, but it has low confidence. So I pushed up
> a patch for comparison that doesn't use the transform to see if there's an
> improvement:
> 
> https://treeherder.mozilla.org/perf.html#/
> compare?originalProject=try&originalRevision=308897cf901b6a01b305cc5303a34148
> a5ab0392&newProject=try&newRevision=ba5475976365b66bd7e4ec4b5bf179e5714ceb45&
> framework=1&showOnlyImportant=0

It's still low confidence, but there's a small improvement in tart showing so I went ahead and filed a bug with a patch to not use the css transform: https://bugzilla.mozilla.org/show_bug.cgi?id=1377551
Joel, Bug 1377551 has now landed in e311e58d0ce0. Has there been any improvement in tart since then?
cart changed a lot, but tart looks to have improved- hard to tell as there is a lot of missing data, but the values for tart were reset on July 1st.  Possibly we can close this bug as fixed?
Flags: needinfo?(jmaher)
(In reply to Joel Maher ( :jmaher) (UTC-8) from comment #29)
> cart changed a lot, but tart looks to have improved- hard to tell as there
> is a lot of missing data, but the values for tart were reset on July 1st. 
> Possibly we can close this bug as fixed?

I agree on closing it - I don't think there's anything left to do here
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Assignee: nobody → bgrinstead
status-firefox56: affected → fixed
status-firefox-esr52: --- → unaffected
Depends on: 1377551
Target Milestone: --- → Firefox 56
Flags: needinfo?(cam)
Whiteboard: [LP_M2]
Whiteboard: [LP_M2] → [LP_M1]
You need to log in before you can comment on or make changes to this bug.