1393742 - 9.21 - 10.7% tpaint (windows10-64) regression on push 80a0c38278154ddf519ee8ff9e7239e938540b37 (Thu Aug 24 2017)

Status: VERIFIED FIXED

(Core :: DOM: Security, defect, P3)

Description

[Ionuț Goldan [:igoldan]]

Talos has detected a Firefox performance regression from push:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=80a0c38278154ddf519ee8ff9e7239e938540b37

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

Regressions:

 11%  tpaint summary windows10-64 pgo e10s     218.04 -> 241.38
  9%  tpaint summary windows10-64 opt e10s     275.73 -> 301.12


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

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

Christoph, I see you are the owner of bug 1393350. Looks like this is something we should accept, but I still want to know if you find something strange about these regressions. If not, I'll resolve this to WONTFIX.

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Ionuț Goldan [:igoldan], Performance Sheriffing from comment #1)
> Christoph, I see you are the owner of bug 1393350. Looks like this is
> something we should accept, but I still want to know if you find something
> strange about these regressions. If not, I'll resolve this to WONTFIX.

We didn't modify the test itself, we just load what was formerly a data: URI from an external file now to comply with the new data: URI inheritance model. I can obviously try to help get any issues resolved, but I can't tell if those are real regressions and if we have to resolve them. Jmaher is probably the better person to ask, I'll change the ni? but will continue to watch this bug.

Comment 3

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

I am curious why this is tpaint only and why this is windows 10 only?  is there something about the data:URI changes that affect windows specifically or 64 bit only?

if we can understand why this is happening for win10 only that would make me happier to say "lets accept this as a test change"

Comment 4

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

oh, 34% regression on linux64, possibly this is just a 64 bit issue?

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Joel Maher ( :jmaher) (UTC-5) from comment #3)
> I am curious why this is tpaint only and why this is windows 10 only?  is
> there something about the data:URI changes that affect windows specifically
> or 64 bit only?

There is nothing platform specific for the data: URI changes we performed. But 34% regression on Linux really sounds like a lot. In particular, this change only loads html from an external file instead of using an inline data: URI. I would imagine it's rather something in the test itself than really a performance regression caused by the Gecko changes. Possibly this is the new baseline? Alternatively we could rewrite the test yet again, go back to using a data: URI an just replace |window.opener.childIsOpen()| with some postMessge handler to bypass the origin restrictions. Do you want me to try that?

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Loading a file takes of course more time than data: url which is all in memory from the beginning.
I wonder if the test could be changed to use data: again but with some postMessages or so?

Comment 7

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

I don't mind the change in the test values as a new baseline, I would just rather we have a better explanation for it.

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1393742_tpaint_postmessage.patch

Let's check if using a postmessage handler changes things here again.

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

I am marking this one as a 'leave-open' so we can have a look at perf numbers with that change applied.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d8af9855103
Rewrite test tpaint.html to rely on data: URI and postmessage handler to avoit perf regression. r=smaug

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/6d8af9855103

Comment 12

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

I see perf improvements:
== Change summary for alert #9077 (as of August 28 2017 13:48 UTC) ==

Improvements:

 26%  tpaint summary linux64 pgo e10s     285.39 -> 209.93
 26%  tpaint summary linux64 opt e10s     315.17 -> 234.39
 20%  tpaint summary osx-10-10 opt e10s   418.62 -> 335.70
  8%  tpaint summary windows10-64 opt e10s304.73 -> 281.42

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

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Joel Maher ( :jmaher) (UTC-5) from comment #12)
> I see perf improvements:

That looks way better. Are you fine with marking that bug as fixed? If so, please go ahead.