Closed
Bug 1393742
Opened 7 years ago
Closed 7 years ago
9.21 - 10.7% tpaint (windows10-64) regression on push 80a0c38278154ddf519ee8ff9e7239e938540b37 (Thu Aug 24 2017)
Categories
(Core :: DOM: Security, defect, P3)
Core
DOM: Security
Tracking
()
VERIFIED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox55 | --- | unaffected |
firefox56 | --- | unaffected |
firefox57 | --- | verified |
People
(Reporter: igoldan, Assigned: ckerschb)
References
Details
(Keywords: perf, regression, talos-regression, Whiteboard: [domsecurity-backlog1])
Attachments
(1 file)
2.43 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Updated•7 years ago
|
Component: Untriaged → DOM: Security
Product: Firefox → Core
Reporter | ||
Comment 1•7 years ago
|
||
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•7 years 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)
Comment 3•7 years ago
|
||
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)
Comment 4•7 years ago
|
||
oh, 34% regression on linux64, possibly this is just a 64 bit issue?
Assignee | ||
Comment 5•7 years 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•7 years 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?
Comment 7•7 years ago
|
||
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•7 years ago
|
||
Let's check if using a postmessage handler changes things here again.
Attachment #8901709 -
Flags: review?(bugs)
Updated•7 years ago
|
Attachment #8901709 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 9•7 years 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•7 years 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
Comment 11•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6d8af9855103
Comment 12•7 years ago
|
||
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•7 years 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)
Updated•7 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: needinfo?(jmaher)
Resolution: --- → FIXED
Updated•7 years ago
|
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.
Description
•