Closed Bug 1412208 Opened 3 years ago Closed 3 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)

55 Branch
defect

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
Component: Untriaged → JavaScript: GC
Product: Firefox → Core
:pbone Can you look over these perf regressions and state whether we can fix them? Or should we backout/accept them?
Flags: needinfo?(pbone)
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)
No worries, then. I will mark this as WONTFIX when the backout happens.
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)
(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)
(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.
"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.)
Priority: -- → P1
Do we have an idea of when someone can get to this (since the Nightly window is closing)?
Flags: needinfo?(jorendorff)
Tracking so we make sure to follow up on this.
:+1:
Flags: needinfo?(jorendorff)
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Assignee: nobody → emilio
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.