Closed
Bug 1412208
Opened 7 years ago
Closed 7 years ago
5.92 - 5.93% Stylo Gecko_nsCSSParser_ParseSheet_Bench / Stylo Servo_DeclarationBlock_GetPropertyById_Bench (osx-10-10) regression on push a3785ec9a48c8c76dd98b5c5140283f8ac44c851 (Thu Oct 26 2017)
Categories
(Core :: JavaScript: GC, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox57 | --- | unaffected |
firefox58 | + | fixed |
People
(Reporter: igoldan, Assigned: emilio)
References
Details
(Keywords: perf, regression)
We have detected a platform microbenchmarks regression from push: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=a3785ec9a48c8c76dd98b5c5140283f8ac44c851 As author of one of the patches included in that push, we need your help to address this regression. Regressions: 6% Stylo Servo_DeclarationBlock_GetPropertyById_Bench osx-10-10 opt 203,094.50 -> 215,144.58 6% Stylo Gecko_nsCSSParser_ParseSheet_Bench osx-10-10 opt 65,489.00 -> 69,368.83 You can find links to graphs and comparison views for each of the above tests at: https://treeherder.mozilla.org/perf.html#/alerts?id=10181 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 jobs in a pushlog format. To learn more about the regressing test(s), please see: https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Automated_Performance_Testing_and_Sheriffing/Platform_Microbenchmarks
Reporter | ||
Updated•7 years ago
|
Component: Untriaged → JavaScript: GC
Product: Firefox → Core
Reporter | ||
Comment 1•7 years ago
|
||
:pbone Can you look over these perf regressions and state whether we can fix them? Or should we backout/accept them?
Flags: needinfo?(pbone)
Comment 2•7 years ago
|
||
The push in question is expected to cause some performance regressions. It's nightly-only temporary code. I guess it would have been nice if the commit message explained that. :(
Flags: needinfo?(pbone)
Reporter | ||
Comment 3•7 years ago
|
||
No worries, then. I will mark this as WONTFIX when the backout happens.
Comment 4•7 years ago
|
||
Yes, that's correct, we were expecting a regression and I'd prefer not to back it out yet. If the regression is too severe then I can make a to reduce the regression by allowing FromData to be inlined again. Although the assertion code is nightly-only making FromData out of line isn't nightly-only. Ionut: what's the normal procedure when committing something like this when we expect it to cause a performance regression but want it to land anyway? A note in the commit message like you suggested? How can I help you next time? Thanks.
Flags: needinfo?(igoldan)
Reporter | ||
Comment 5•7 years ago
|
||
(In reply to Paul Bone [:pbone] from comment #4) > Ionut: what's the normal procedure when committing something like this when > we expect it to cause a performance regression but want it to land anyway? > A note in the commit message like you suggested? How can I help you next > time? > > Thanks. You can leave a comment in the bug you want to land. Just mention you expect some perf regressions. You don't have to mention which specific ones. I'll figure out. And then needinfo? me. Thank you so much for considering this.
Flags: needinfo?(igoldan)
Comment 6•7 years ago
|
||
(In reply to Ionuț Goldan [:igoldan], Performance Sheriffing from comment #5) > (In reply to Paul Bone [:pbone] from comment #4) > > Ionut: what's the normal procedure when committing something like this when > > we expect it to cause a performance regression but want it to land anyway? > > A note in the commit message like you suggested? How can I help you next > > time? > > > > Thanks. > You can leave a comment in the bug you want to land. Just mention you expect > some perf regressions. You don't have to mention which specific ones. I'll > figure out. > And then needinfo? me. Thanks, yeah, in this case I didn't know which tests would be affected, so it was still helpful/interesting to read this bug. I don't imagine this will happen often. > Thank you so much for considering this. No worries. Just don't want anyone to worry/panic, including me ;-) Cheers.
Comment 7•7 years ago
|
||
"we don't intend to ship a release with this" -> setting P1. (That doesn't mean anyone has to drop everything and work on this, only that further action is needed this cycle.)
Comment 8•7 years ago
|
||
Do we have an idea of when someone can get to this (since the Nightly window is closing)?
Flags: needinfo?(jorendorff)
Assignee | ||
Comment 10•7 years ago
|
||
This was backed out on inbound today: https://hg.mozilla.org/integration/mozilla-inbound/rev/23f86a1ac424
Reporter | ||
Updated•7 years ago
|
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Updated•6 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•