Closed
Bug 535436
Opened 15 years ago
Closed 13 years ago
perf regression on JSA* benchmark
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
WORKSFORME
People
(Reporter: rcampbell, Unassigned)
References
()
Details
(Keywords: perf, regression, Whiteboard: [regression range wanted on tracemonkey][3.6.x])
Attachments
(1 file)
(deleted),
application/octet-stream
|
Details |
seeing 1-2s slow-down (depending on machine) from Firefox 3.5.5 to Firefox 3.6 nightlies. Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2b6pre) Gecko/20091215 Namoroka/3.6b6pre ID:20091215033611 mozilla-central (recent nightly) is around 270ms on my machine vs. 7500ms for 3.6. Boris did some bisection and found the changeset containing the perf-win: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=65c1582465ef&tochange=d040e38c4107
Flags: blocking1.9.2?
Reporter | ||
Updated•15 years ago
|
Keywords: perf,
regression
![]() |
||
Comment 1•15 years ago
|
||
To be clear, that changeset contains a perf win from 1600ms to 300ms on my machine. There was _another_ perf win before that sometime, presumably.
![]() |
||
Comment 3•15 years ago
|
||
The previous speedup is in http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=38754465ffde&tochange=3079370d6597 Sadly, that's a huge (255 changesets) tracemonkey merge. :(
![]() |
||
Comment 4•15 years ago
|
||
And given the later result, I would have expected one of dmandelin's *NAME fixes from that merge to be relevant, but they landed on 1.9.2...
![]() |
||
Comment 5•15 years ago
|
||
Oh, and on t-m right now the time is spent 80% in jit-generated code. The rest is unboxing doubles and Array.push, plus some minor bits. It's still slower than Chrome (comparable to their first run, then 50% slower), but 30% faster than Safari. So speeding this up more (not really the subject of this bug) would require some more careful looking. :(
Reporter | ||
Comment 6•15 years ago
|
||
range on tracemonkey is between: http://hg.mozilla.org/tracemonkey/rev/5639c3f2e85b - and - http://hg.mozilla.org/tracemonkey/rev/66235f4c4eea
![]() |
||
Comment 7•15 years ago
|
||
So http://hg.mozilla.org/tracemonkey/pushloghtml?fromchange=5639c3f2e85b&tochange=66235f4c4eea My money is sadly on bug 471214 for fixing the perf regression...
Comment 8•15 years ago
|
||
hi, i'm the author of the a-star implementation (aka hack), demo and benchmark. i just wanted to add that i'm still working on it and the js code will change, thus making before-after comparisons invalid (i haven't set up a public rep yet). if there are any questions regarding the implementation itself, feel free to contact me.
Reporter | ||
Comment 9•15 years ago
|
||
just taking a snapshot in case the test changes at the URL.
Comment 10•15 years ago
|
||
I have no idea how to make a blocking decision on this, but since I don't think we're tracking that particular benchmark, my default inclination is not to block. I'm also pretty sure that the patch Boris points to here can't land on 1.9.2 ...
![]() |
||
Comment 11•15 years ago
|
||
The questions are: 1) Are we ok with this regression to slower-than-interp performance? 2) Is the regression the result of some sort of mis-merge? If the answer to #2 is "no", then we probably don't want to block no matter what.
![]() |
||
Comment 12•15 years ago
|
||
And the other question is whether the regression is something that would hit other things too, or not likely. I think we're over-focused on benchmarks, and our performance suffers in practice as a result, and web developers notice that. To be clear, the 7500 number robcee is seeing is about 3x slower than interp on the same testcase.
![]() |
||
Comment 13•15 years ago
|
||
OK, via local testing I just confirmed that bug 471214 is in fact what took this from 3x slower than interp to 2x faster than interp... robcee, do you think you can figure out when this became slower than 3.5 on t-m? But it does look like we shouldn't block on this. :(
Depends on: 471214
Reporter | ||
Comment 14•15 years ago
|
||
(In reply to comment #13) > robcee, do you think you can figure out when this became slower than 3.5 on > t-m? But it does look like we shouldn't block on this. :( I should have time to do that this weekend. I understand the non-blocking decision but it is a little painful to ship with a perf regression, even it if is a somewhat synthetic benchmark. I could see this style of code appearing in some real-world applications so I do think it's worth fixing.
Reporter | ||
Updated•15 years ago
|
Keywords: regressionwindow-wanted
Reporter | ||
Comment 15•15 years ago
|
||
range should be somewhere between 08-01-09 and 09-01-09, I guess.
Whiteboard: [regression range wanted on tracemonkey]
Reporter | ||
Comment 16•15 years ago
|
||
interesting! 08-27 - 3084ms - http://hg.mozilla.org/tracemonkey/rev/429fe0f0a0e0 08-28 - 11519ms - http://hg.mozilla.org/tracemonkey/rev/10380ffe4d49 08-29 - 8684ms - http://hg.mozilla.org/tracemonkey/rev/d8a9a1803ea5 Looks like there was a big jump on the 28th, then maybe some backouts or damage control on the 29th and 30th, but this never came back down below the 7100ms I saw on the 31st. the URL is the changeset from about:buildconfig. http://hg.mozilla.org/tracemonkey/pushloghtml?fromchange=429fe0f0a0e0&tochange=10380ffe4d49
![]() |
||
Comment 17•15 years ago
|
||
Hrm. I wonder whether landing bug 505591 made us try to trace more stuff but fail to run the traces due to the function cloning thing....
![]() |
||
Comment 18•15 years ago
|
||
Yep. Based on local testing, that's exactly what happened....
We could do a quick heuristic here. For example, giving trees a "fannoutCount" variable and for certain, specific guards, incrementing that number and blacklisting when it reaches some limit. We'd just have to add a new ExitType that acts like a BRANCH_EXIT but increments this counter. I did something like this for recursion and it worked well - you just have to make sure it doesn't negatively affect code with similar behavior that did trace well.
![]() |
||
Comment 20•15 years ago
|
||
Is that something we can do for 1.9.2 (or one of its minor updates)?
Yes.
Comment 22•15 years ago
|
||
Adding [3.6.x] to the whiteboard which I'm using to track branch fixes for things, and marking blocking1.9.2- based on previous comments.
Flags: wanted1.9.2+
Flags: blocking1.9.2?
Flags: blocking1.9.2-
Whiteboard: [regression range wanted on tracemonkey] → [regression range wanted on tracemonkey][3.6.x]
Comment 23•15 years ago
|
||
According to the author regarding benchmark updates [1], the bug's URL should probably be updated to: http://jsastar.tapirpirates.net/original_tracemonkeybench/JSAStarBenchmark.html [1] http://jsastar.tapirpirates.net/original_tracemonkeybench/
![]() |
||
Comment 24•15 years ago
|
||
Note that the new version actually does NOT trace well on tip and t-m. I filed bug 536564.
Comment 25•13 years ago
|
||
Bug 576687 was also filed on this same version of the JSA* benchmark, and performance is competitive. Bug 576688 remains open for performance issues with the "New" JSA* benchmark.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → WORKSFORME
Comment 26•10 years ago
|
||
Issue is resolved - clearing old keywords - qa-wanted clean-up
Keywords: regressionwindow-wanted
You need to log in
before you can comment on or make changes to this bug.
Description
•