1.07 KB, text/html
1.08 KB, text/html
1.04 KB, text/html
189 bytes, text/plain
558 bytes, text/plain
2.66 KB, patch
|Details | Diff | Splinter Review|
Created attachment 335001 [details] 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.
Created attachment 335005 [details] 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...
Summary: Tracing fails for prime number generation testcase → TM: Tracing fails for prime number generation testcase
Hrm restarting the browser gets me correct results now...
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.
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
Created attachment 335104 [details] Minimal-ish js shell correctness testcase, nothing to do with primes.
Depends on: 451785
Depends on: 451787
Depends on: 451788
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
Fixed fasta, but fannkuch is still dog slow. http://hg.mozilla.org/index.cgi/tracemonkey/ Investigating further.
Status: NEW → ASSIGNED
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
bz, could you test this again?
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.
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.
Patch landed on TM, will let it ride along into MC later. http://hg.mozilla.org/tracemonkey/rev/d02034acc88c /be
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?
Attachment #335104 - Attachment description: Minimal-ish js shell testcase → Minimal-ish js shell correctness testcase, nothing to do with primes.
Created attachment 336658 [details] 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.
I would have thought that the patch Brendan landed for bug 454027 would help here, but it doesn't seem to.
Attachment #336609 - Flags: review?(gal) → review+
bz, can you confirm that this is fixed and then close?
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.
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.
Someone who actually knows whether we have sufficient tests for this should toggle in-testsuite as needed.
test already in js1_8_1/trace/trace-test.js
Just an update - on the original testcase: Chrome: 1.055s Sliverlight: 1.05s Today's nightly: 1.24s Flash: 2.35s
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....
in-testsuite? for performance test.
Flags: in-testsuite+ → in-testsuite?
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
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.