1501300 - 0.16 - 2.62% tp5o (linux64, windows10-64, windows7-32) regression on push 6d6b5c10bba56bc286ae66cf9d72948a6a03d200 (Thu Oct 18 2018)

Status: RESOLVED WONTFIX

(Testing :: General, defect)

Description

[Ionuț Goldan [:igoldan]]

Talos has detected a Firefox performance regression from push:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=6d6b5c10bba56bc286ae66cf9d72948a6a03d200

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

Regressions:

  3%  tp5o windows10-64 pgo e10s stylo     117.22 -> 120.29
  2%  tp5o windows7-32 pgo e10s stylo      116.52 -> 118.60
  0%  tp5o linux64 pgo e10s stylo          126.85 -> 127.04


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

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

Comment 1

[Ionuț Goldan [:igoldan]]

Paolo, I blocked both bugs from the patch. If only one is related, please remove the other one from the "Blocks" list.

Comment 2

[Ionuț Goldan [:igoldan]]

Here are the Gecko profiles for tp5o on Windows 10:

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

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

Comment 3

[Brian Grinstead [:bgrins]]

I wonder if removing the CE (and/or platform impl) for progressmeter would restore some of this? Doubt it would affect the same test but would be worth checking.

Comment 4

[:Paolo Amadini]

These are the only HTML progress elements we have in the browser window now:

https://searchfox.org/mozilla-central/rev/a7f4d3ba4fbfe3efbde832869f1d672fce7122f6/browser/base/content/popup-notifications.inc#66
https://searchfox.org/mozilla-central/rev/a7f4d3ba4fbfe3efbde832869f1d672fce7122f6/browser/components/downloads/content/downloadsPanel.inc.xul#131

They are both hidden by default, and I don't see code from nsProgressFrame or HTMLProgressElement in the profiles. Is it possible that this is still affecting performance in some way, or is it improbable for hidden HTML elements?

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Hidden elements are pretty unlikely to cause perfomance issues... Specially if they aren't CE or something like that... This is indeed a bit bizarre.

How has this only affected tp5o? Doesn't that measure just content performance?

Comment 6

[:Paolo Amadini]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #5)
> How has this only affected tp5o? Doesn't that measure just content
> performance?

In fact, it might be a regression unrelated to these changes, or only loosely related. The only thing I thought was that we did additional background work for these hidden elements while the content pages were processed, but this doesn't seem to be the case.

Anyways, I'll run tests to better understand if this may actually affect these measures at all.

Comment 7

[:Paolo Amadini]

Baseline with regression:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=b1bc255dbd4d9d4919070f6df826b71dbc1cd279

Test replacing <html:progress> with <hbox>:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=d6827e7dd924640426900e25891812d51f12b700

Comment 8

[:Paolo Amadini]

Another hypothesis is that this may trip PGO, making some code paths seem more likely than others? I'm not sure if there's much we can or should do in that case.

Comment 9

[Brian Grinstead [:bgrins]]

(In reply to :Paolo Amadini from comment #7)
> Baseline with regression:
>  
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=b1bc255dbd4d9d4919070f6df826b71dbc1cd279
> 
> Test replacing <html:progress> with <hbox>:
>  
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=d6827e7dd924640426900e25891812d51f12b700

Those pushes don't have PGO builds, which would help to validate comment 8

Comment 10

[:Paolo Amadini]

Ionuț, what do you think of this result?

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=b1bc255dbd4d9d4919070f6df826b71dbc1cd279&newProject=try&newRevision=d6827e7dd924640426900e25891812d51f12b700&framework=1

Comment 11

[Ionuț Goldan [:igoldan]]

(In reply to :Paolo Amadini from comment #10)
> Ionuț, what do you think of this result?
> 
> https://treeherder.mozilla.org/perf.html#/
> compare?originalProject=try&originalRevision=b1bc255dbd4d9d4919070f6df826b71d
> bc1cd279&newProject=try&newRevision=d6827e7dd924640426900e25891812d51f12b700&
> framework=1

I don't see any perf win on the affected platforms.

Comment 12

[Brian Grinstead [:bgrins]]

(In reply to Ionuț Goldan [:igoldan], Performance Sheriffing from comment #11)
> (In reply to :Paolo Amadini from comment #10)
> > Ionuț, what do you think of this result?
> > 
> > https://treeherder.mozilla.org/perf.html#/
> > compare?originalProject=try&originalRevision=b1bc255dbd4d9d4919070f6df826b71d
> > bc1cd279&newProject=try&newRevision=d6827e7dd924640426900e25891812d51f12b700&
> > framework=1
> 
> I don't see any perf win on the affected platforms.

Those don't include PGO, which is the only thing that regressed. Can you do a new push with PGO included?

Comment 13

[Ionuț Goldan [:igoldan]]

Somehow, the PGO builds I've issued for Windows 32/64bit and Linux 64bit have failed.
:jmaher do you have any clue of why this happened?

Comment 14

[Ionuț Goldan [:igoldan]]

(In reply to Ionuț Goldan [:igoldan], Performance Sheriffing from comment #13)
> Somehow, the PGO builds I've issued for Windows 32/64bit and Linux 64bit
> have failed.
> :jmaher do you have any clue of why this happened?

I'm referring to Paolo's compare view link from comment 10.

Comment 15

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

the failure to build with --enable-lto was a problem I saw while trying to bisect some other changes [2].

:glandium- do you know the history of --enable-lto and why around oct 24-25 it was broken [1]

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=b1bc255dbd4d9d4919070f6df826b71dbc1cd279&selectedJob=208586052
[2] https://treeherder.mozilla.org/#/jobs?repo=try&author=jmaher%40mozilla.com&searchStr=4.0%2Cbuild&fromchange=8476a48c0ba7598d8abe2583a8f1e2c4b50bbbea&tochange=21bed18a839fe3d90f846ae9c8f3110d39dbc25a

Comment 16

[Mike Hommey [:glandium]]

Artifact builds were broken when we landed the enabling of LTO because of mozconfig subtleties. You can get around this by not using artifact builds.

Comment 17

[Ionuț Goldan [:igoldan]]

I repushed to Try Paolo's patch from comment 10. Again, I don't see any kind of perf wins there.

Comment 18

[Ionuț Goldan [:igoldan]]

The Try push is here: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=abb7cff1debb&newProject=try&newRevision=b698ec7e2e7d&framework=1&filter=tp5o%20pgo

Comment 19

[Brian Grinstead [:bgrins]]

Given Comment 5 / Comment 7 / Comment 17 I don't know where else we can go with this. If even replacing the html:progress elements with hbox makes no difference then the element itself isn't slowing anything down. If anything the html:progress element should be faster than either the XBL or CE version of xul:progressmeter (since there's no JS running), and certainly an hbox should. So if that's not moving the numbers it makes me skeptical that it's the introduction of the elements themselves. Is it possible removing xul:progressmeter is somehow just tripping up PGO as Paolo suggests in Comment 10? And if so, is this a wontfix or are there further steps we can take?

Comment 20

[Ionuț Goldan [:igoldan]]

(In reply to Brian Grinstead [:bgrins] from comment #19)
> Is it possible removing xul:progressmeter is somehow just tripping up PGO as Paolo suggests in
> Comment 10?

I'm not the expert to answer this. :jmaher can you ni? someone who can shed some light on this question?

> And if so, is this a wontfix or are there further steps we can take?

If only the PGO build was tripped up, then we can WONTFIX this bug.

Comment 21

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

I think we should mark as wontfix- being pgo only there is a bit of a black box that we have very delicate controls over.