451673 - TM: Tracing fails for prime number generation testcase

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Mike Schroepfer]

Attachment: Original testcase

See attached testcases.  First version was a straight translation from a flash and sliverlight version of the code.  It revealed the (thanks to bz) following issues:

1) Math.ceil is not traced
2) bool == bool not trace

So I build a second version to work around this (second testcase) and it still falls off trace.

Comment 1

[Mike Schroepfer]

Attachment: Removed use of bools and math.ceil - still fails

Comment 2

[Mike Schroepfer]

Attachment: With spurious continue removed - much faster incorrect results..

Bz noted that !(int) was not currently traced either so the move to ints hit us in a different way.  We were both confused as to the utility of the continue so we just removed that and got very fast runtimes but incorrect results.  Turning off JIT gets us correct results...

Comment 3

[Mike Schroepfer]

Hrm restarting the browser gets me correct results now...

Comment 4

[Boris Zbarsky [:bzbarsky]]

I still see the bogus results.  Furthermore, if I add a printf to js_dmod, I see that we're only hitting it with b == 3 starting after some point.  That pretty much explains why we get 300k primes (at least that's what I see here).

Interestingly, the point at which things go sour is exactly the point at which we start calling js_Math_floor!  That is, up until i get to be 401 or so we don't call js_Math_floor at all (presumably that part isn't jitted yet?).  After that we start calling it, and that's precisely when the calls to js_dmod start happening only for b == 3.  Presumably we're screwing up the limit somehow; not sure how.  I did confirm that js_Math_floor is returning the right values.

Comment 5

[Boris Zbarsky [:bzbarsky]]

gal and I debugged this a bit.  It looks like j and limit are doubles as far as the outer loop is concerned, and ints as far as the inner loop is concerned.  So the inner loop is not looking in the right registers when it goes to do the loop test, and we end up only traversing it once.

gal says:

<gal> ok, thats simply not handled
<gal> ok, so I guess what we need here is to flag that slot for the inner tree
      in the oracle as "don't demote" and then flush the inner tree
<gal> both will be rebuilt, inner with double this time, and all will be good
<gal> can you post the most minimal testcase you can come up with?
<gal> I will try to fix and test with that and then add to trace-tests

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Minimal-ish js shell correctness testcase, nothing to do with primes.

Comment 7

[Andreas Gal :gal]

Fixed but this triggers a perf regression now (same we hit in fannkuch and fasta, trying to grow inner tree). Investigating.

http://hg.mozilla.org/index.cgi/tracemonkey/rev/2f39c79606d9

Comment 8

[Andreas Gal :gal]

Fixed fasta, but fannkuch is still dog slow.

http://hg.mozilla.org/index.cgi/tracemonkey/

Investigating further.

Comment 9

[Andreas Gal :gal]

Thats what you get for posting to bugzilla after 1am. The hg link this time for real:

http://hg.mozilla.org/index.cgi/tracemonkey/rev/c0e568db2d39

Comment 10

[Andreas Gal :gal]

bz, could you test this again?

Comment 11

[Boris Zbarsky [:bzbarsky]]

We now give the right answer, but the speedup over non-jit is <2x.  I tried rewriting the testcase to not use a break, but that doesn't seem to help.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: JS shell testcase for counting primes without explicit break

Comment 13

[Andreas Gal :gal]

stepping over outer loop edges via trackLoopEdges() causes a bad tail explosion for this case (700+ traces). Without it we don't complete a functional outer tree. Investigating.

Comment 14

[Brendan Eich [:brendan]]

Attachment: patch

Comment 15

[Brendan Eich [:brendan]]

Patch landed on TM, will let it ride along into MC later.

http://hg.mozilla.org/tracemonkey/rev/d02034acc88c

/be

Comment 16

[Boris Zbarsky [:bzbarsky]]

So... as of tracemonkey tip this morning:

1)  The JS shell testcase from comment 12 is 1.3 seconds with jit, 3.2 seconds
    without.  Reasonable, but not great.  Unoptimized C is order of 0.17 seconds
    with int math, 0.2 seconds with double math.

2)  The testcase in comment 2 is 15.1 seconds with jit, 3.2 seconds without.  So
    a 4x performance _regression_ with jit on.

I'll try to attach a JS shell version of that testcase.

Can we do something about adding these various micro-tests to some sort of TM performance suite, since we seem to keep improving some tests at the expense of others?

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: JS shell testcase version of attachment from comment 2

Some more data with jit on:

Attachment in comment 0 takes about 1.6 seconds
Attachment in comment 1 takes about 5 seconds.

Comment 18

[Boris Zbarsky [:bzbarsky]]

I would have thought that the patch Brendan landed for bug 454027 would help here, but it doesn't seem to.

Comment 19

[Andreas Gal :gal]

bz, can you confirm that this is fixed and then close?

Comment 20

[Boris Zbarsky [:bzbarsky]]

It's not fixed; see comment 16 item 2.  Unless you think we should spin things out into separate bugs?  It seems like this one testcase + variations has hit quite a number of issues; that's why I started filing bugs it depends on for the "real work".

I would love to have a way to write a regression test for the performance issue here, by the way.

Comment 21

[Boris Zbarsky [:bzbarsky]]

I guess this bug got hijacked and mutated, then the mutated bug fixed. :(  The original testases are still quite broken (worse than when this bug was filed).

I have filed bug 455751 and bug 455753 on the issues in comment 16, but really, the best way to treat bugs like this is to treat them as trackers and file separate bugs for each step we take towards making the testcase faster...  As things stand, we'll have to retest the original testcase in this bug once we fix the various dependencies anyway.

Fixed, I guess.

Note that comment 17 is no longer correct: the testcase from comment 1 now takes the same 15 seconds as the testcase from comment 2.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Someone who actually knows whether we have sufficient tests for this should toggle in-testsuite as needed.

Comment 23

[Bob Clary [:bc] (inactive)]

test already in js1_8_1/trace/trace-test.js

Comment 24

[Mike Schroepfer]

Just an update - on the original testcase:

Chrome: 1.055s
Sliverlight: 1.05s
Today's nightly: 1.24s
Flash: 2.35s

Comment 25

[Boris Zbarsky [:bzbarsky]]

Bob, trace-test is just testing correctness.  This was a performance bug, so ideally we'd add the test to a performance test suite too....

Comment 26

[Bob Clary [:bc] (inactive)]

in-testsuite? for performance test.

Comment 27

[Bob Clary [:bc] (inactive)]

http://hg.mozilla.org/tracemonkey/rev/3ce8178cec14
/cvsroot/mozilla/js/tests/js1_8_1/trace/regress-451673.js,v  <--  regress-451673.js
initial revision: 1.1