If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

VERIFIED FIXED in Firefox 57

Status

()

Core
DOM: Security
P3
normal
VERIFIED FIXED
25 days ago
18 days ago

People

(Reporter: igoldan, Assigned: ckerschb)

Tracking

(Blocks: 1 bug, {perf, regression, talos-regression})

unspecified
mozilla57
perf, regression, talos-regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 unaffected, firefox57 verified)

Details

(Whiteboard: [domsecurity-backlog1])

Attachments

(1 attachment)

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
Component: Untriaged → DOM: Security
Product: Firefox → Core
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.
Flags: needinfo?(ckerschb)
(Assignee)

Comment 2

25 days ago
(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.
Flags: needinfo?(ckerschb) → needinfo?(jmaher)
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"
Flags: needinfo?(jmaher) → needinfo?(ckerschb)
oh, 34% regression on linux64, possibly this is just a 64 bit issue?
(Assignee)

Comment 5

24 days ago
(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?
Flags: needinfo?(ckerschb) → needinfo?(jmaher)

Comment 6

24 days ago
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?
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.
Flags: needinfo?(jmaher)
(Assignee)

Comment 8

22 days ago
Created attachment 8901709 [details] [diff] [review]
bug_1393742_tpaint_postmessage.patch

Let's check if using a postmessage handler changes things here again.
Attachment #8901709 - Flags: review?(bugs)

Updated

22 days ago
Attachment #8901709 - Flags: review?(bugs) → review+
(Assignee)

Comment 9

22 days ago
I am marking this one as a 'leave-open' so we can have a look at perf numbers with that change applied.
Assignee: nobody → ckerschb
Status: NEW → ASSIGNED
Keywords: checkin-needed, leave-open
Priority: -- → P3
Whiteboard: [domsecurity-backlog1]

Comment 10

22 days ago
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6d8af9855103
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
(Assignee)

Comment 13

20 days ago
(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.
Flags: needinfo?(jmaher)
Status: ASSIGNED → RESOLVED
Last Resolved: 20 days ago
Flags: needinfo?(jmaher)
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
status-firefox55: --- → unaffected
status-firefox56: --- → unaffected
status-firefox57: --- → verified
status-firefox-esr52: --- → unaffected
Keywords: leave-open
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.