Closed Bug 1363438 Opened 2 years ago Closed 2 years ago

15% regression in several component speedometer tests around April 24

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED WORKSFORME
Tracking Status
firefox53 --- unaffected
firefox54 --- unaffected
firefox55 --- affected
firefox56 --- affected

People

(Reporter: kbrosnan, Unassigned, NeedInfo)

References

(Blocks 1 open bug)

Details

(Keywords: regression, regressionwindow-wanted, Whiteboard: [qf:p1] [pi-monitoring])

[Tracking Requested - why for this release]:

Examples of tests that show this regression. 

Angular2-TypeScript-TodoMVC-Adding100Items-sync - https://arewefastyet.com/#machine=17&view=single&suite=speedometer-misc&subtest=Angular2-TypeScript-TodoMVC-Adding100Items-sync

AngularJS-TodoMVC-Adding100Items-async - https://arewefastyet.com/#machine=17&view=single&suite=speedometer-misc&subtest=AngularJS-TodoMVC-Adding100Items-async

AngularJS-TodoMVC-CompletingAllItems-async - https://arewefastyet.com/#machine=17&view=single&suite=speedometer-misc&subtest=AngularJS-TodoMVC-CompletingAllItems-async

Elm-TodoMVC-Adding100Items-sync - https://arewefastyet.com/#machine=17&view=single&suite=speedometer-misc&subtest=Elm-TodoMVC-Adding100Items-sync

AWFY suggests a regression range of https://hg.mozilla.org/integration/mozilla-inbound/log?rev=6efe7915f97b389c51b85bb2e1e45004f33ca680%3A%3A0f97f76c0b34673f2bf31c96eb1285416836f565%20and%20%216efe7915f97b389c51b85bb2e1e45004f33ca680

There are two core changes that seem possible that they introduced the regression. They are Bug 1359415 - move threadsafety checks inside nsAutoOwningThread and  Bug 1358275 - Preliminary cleanup: refactor nsTextFragment::UpdateBidiFlag()
Whiteboard: [qf:investigate] → [qf:p1]
Makes sense to track this for 55. Who can check if 54 is affected?
Once we get the regressing bug we will know. It is unlikely that 54 is affected but I wanted to be cautious.
Flags: needinfo?(nfroyd)
Flags: needinfo?(jfkthame)
Certainly possible; we moved threadsafety checks for AddRef/Release out-of-line.

54 can't be affected, because none of the changesets landed in 54.
Flags: needinfo?(nfroyd)
It's possible the change to nsTextFragment::UpdateBidiFlag in bug 1358275 could cause a regression, given that it took the loop out-of-line, though I didn't think this was such a hot codepath that it would matter.

Can we get a profile of a regressing test run, to see if either this or the thread-safety stuff from bug 1359415 shows up at significant levels?
Flags: needinfo?(jfkthame)
I will email you a link to the tests that you can profile, Jonathan.
Filtering for stacks including "nsTextFragment" on a profile from running the full set of speedometer tests shows me a total of only 4ms (and half of that is in the destructor), so it doesn't look like the nsTextFragment::UpdateBidiFlag change can be behind this.
The overall numbers for the Speedometer test do not show the regression. Only some tests do, for example speedometer-misc-Elm-TodoMVC-Adding100Items-sync shows the regression clearly.  https://arewefastyet.com/#machine=17&view=single&suite=speedometer-misc&subtest=Elm-TodoMVC-Adding100Items-sync

Do we have some documentation about running individual speedometer tests in a similar manner to AWFY?
So I found https://arewefastyet.com/schedule.php which when logged in allows you to trigger AWFY runs based on builds from our source trees or try. I am going to update my inbound tree, push to try for builds and see if this works as expected.
Kevin, Benjamin Bouvier has taken over the maintenance of AWFY, CCing him here in case you have questions from him.  :-)
No longer blocks: TimeToFirstPaint_FB
Kevin, did you get a chance to bisect and find the offending bug here?
Flags: needinfo?(kbrosnan)
I've not yet. I'll see what I can do on Tuesday when I get back or ask for help. I have a set of steps that should be able to find the regression. Try -> AWFS schedule -> wait for results, might try pushing several changesets as there seems to be a few hour lag on AWFY scheduled tests.
Flags: needinfo?(kbrosnan)
Great, thanks.
Whiteboard: [qf:p1] → [qf:p1] [monitoring]
Setting fix-optional for 55 since we're going to beta soon and there's no assignee.
Kevin, did you finish your bisection attempts here?
Flags: needinfo?(kbrosnan)
I was under the impression that we cannot ship with any regressions wrt Quantum release criteria, leading up to 57. Is that an incorrect assumption? Tagged as blocking 55 unless someone strongly disagrees.
I may be misreading the AWFY charts, but I see a performance improvement, not a regression, on April 24 in the AWFY graphs linked from comment 0. We had a lot of AWFY configuration changes for Speedometer V2 around that time. Given that those measurements are now two months old and we've made big performance improvements [1] lately, can we just resolve this bug as WFM?

[1] https://arewefastyet.com/#machine=36&view=single&suite=speedometer-misc&subtest=score
Yes.
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → WORKSFORME
Whiteboard: [qf:p1] [monitoring] → [qf:p1] [pi-monitoring]
You need to log in before you can comment on or make changes to this bug.