Closed Bug 1360277 Opened 3 years ago Closed 3 years ago

tp5 regression in November on windows 8

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED WONTFIX

People

(Reporter: jmaher, Assigned: evilpie)

References

Details

(Whiteboard: [qf:p1])

this is a regression that was overlooked in our automatic alerts due to a lot of change (regressions, fixes, etc.) in a really short time window.  So we didn't get an alert.  In looking back on tp5 over time, we see on November 23rd there is a regression that if fixed would give us a nice perf win.

here is a zoomed in perfherder graph:
https://treeherder.mozilla.org/perf.html#/graphs?timerange=31536000&series=%5Bmozilla-inbound,b68e2b084272409d7def3928a55baf0e00f3888a,1,1%5D&zoom=1477624922893.967,1481255587440.585,233.18839695142663,294.05796216881794

On this graph I had to hunt around and see that the increase happens here (since perfherder doesn't seem to show revision/alert history much past 4 months old):
https://treeherder.mozilla.org/api/project/mozilla-inbound/resultset/147293/

typically I would retrigger this revision and some before/after to prove this, unfortunately I don't have the ability to retrigger since we don't keep artifacts >3 months.

As we are focusing on windows 64 bit for Firefox 57, this is something we should put some attention on.
Whiteboard: [qf]
Tom can you please take a look at this changeset to see if it may have caused the regression?
Assignee: nobody → evilpies
Flags: needinfo?(evilpies)
Whiteboard: [qf] → [qf:p1]
Sure, I think it's possible, but maybe unlikely? What is perfherder actually running and can somebody test if we hit MappedArgumentsObject::obj_defineProperty. We should not hit this for assignments like `arguments[n] = 1`, but we might in some other cases.
Flags: needinfo?(evilpies)
I ran `./mach talos-test --suite tp5o --tppagecycles 2` and for every page-load I observed about 5 calls to that function, that should be insignificant. Maybe we disabled some other IC other optimization by adding this hook?
a lot has changed since November 8th in the product- maybe there are no quick wins on this regression.
Joel, even though we don't keep artifacts around, can we create try builds with the before/after changesets at the time and re-run the tests to validate the jump?
I have attempted some pushes to try, if the builds work I will post the links here.  A lot has changed since November, so I am lacking confidence in this, but enough confidence that I think a try push is worth a go :)
oh, this is a light day for load on the trees!

Here is what I see:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=b1bc2c004763&newProject=try&newRevision=afd1637b8e0520d31295a16e17ba91a065767ed0&framework=1&showOnlyImportant=0

That is a 4% tp5o e10s regression on win8; so given that backing that out on the tip didn't produce an improvement, then I see little value in pursuing this avenue- it is good to know that this did cause a specific regression in the past, possible there are lessons there.
I just realized I didn't backout the whole change, there was some change to freezing as well. I am going to do another push with a complete backout.
Any more data from the complete backout pushes Tom?
Apart from one outlier I don't see any improvements.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → WONTFIX
Agreed.
You need to log in before you can comment on or make changes to this bug.