535436 - perf regression on JSA* benchmark

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect)

Description

[Rob Campbell [:rc] (:robcee)]

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

Comment 1

[Boris Zbarsky [:bzbarsky]]

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 2

[Boris Zbarsky [:bzbarsky]]

OK, the most recent speedup was from bug 530255.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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

[Boris Zbarsky [:bzbarsky]]

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

[Boris Zbarsky [:bzbarsky]]

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.  :(

Comment 6

[Rob Campbell [:rc] (:robcee)]

range on tracemonkey is between: 

http://hg.mozilla.org/tracemonkey/rev/5639c3f2e85b
- and -
http://hg.mozilla.org/tracemonkey/rev/66235f4c4eea

Comment 7

[Boris Zbarsky [:bzbarsky]]

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

[sschallerl]

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.

Comment 9

[Rob Campbell [:rc] (:robcee)]

Attachment: JSAStarBenchmark archive from 12-16-09

just taking a snapshot in case the test changes at the URL.

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

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

[Boris Zbarsky [:bzbarsky]]

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

[Boris Zbarsky [:bzbarsky]]

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

[Boris Zbarsky [:bzbarsky]]

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.  :(

Comment 14

[Rob Campbell [:rc] (:robcee)]

(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.

Comment 15

[Rob Campbell [:rc] (:robcee)]

range should be somewhere between 08-01-09 and 09-01-09, I guess.

Comment 16

[Rob Campbell [:rc] (:robcee)]

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

[Boris Zbarsky [:bzbarsky]]

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

[Boris Zbarsky [:bzbarsky]]

Yep.  Based on local testing, that's exactly what happened....

Comment 19

[David Anderson [:dvander] - inactive, e-mail if emergency]

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

[Boris Zbarsky [:bzbarsky]]

Is that something we can do for 1.9.2 (or one of its minor updates)?

Comment 21

[David Anderson [:dvander] - inactive, e-mail if emergency]

Yes.

Comment 22

[Mike Beltzner [:beltzner, not reading bugmail]]

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.

Comment 23

[Helder "Lthere" Magalhães]

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

[Boris Zbarsky [:bzbarsky]]

Note that the new version actually does NOT trace well on tip and t-m.  I filed bug 536564.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 26

[Joshua Mitchell (Inactive)]

Issue is resolved - clearing old keywords - qa-wanted clean-up