TM: Tracing fails for prime number generation testcase

RESOLVED FIXED in mozilla1.9.1b1

Status

()

Core
JavaScript Engine
P2
normal
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: Mike Schroepfer, Assigned: gal)

Tracking

({testcase})

unspecified
mozilla1.9.1b1
testcase
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.9.1 ?
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments)

(Reporter)

Description

10 years ago
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.
(Reporter)

Comment 1

10 years ago
Created attachment 335002 [details]
Removed use of bools and math.ceil - still fails
(Reporter)

Comment 2

10 years ago
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
(Reporter)

Comment 3

10 years ago
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.

Updated

10 years ago
Depends on: 451782
(Assignee)

Comment 7

10 years ago
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
(Assignee)

Comment 8

10 years ago
Fixed fasta, but fannkuch is still dog slow.

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

Investigating further.
Status: NEW → ASSIGNED
(Assignee)

Comment 9

10 years ago
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
(Assignee)

Updated

10 years ago
Assignee: general → gal
Status: ASSIGNED → NEW
(Assignee)

Comment 10

10 years ago
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.
Flags: in-testsuite?
Created attachment 336057 [details]
JS shell testcase for counting primes without explicit break
(Assignee)

Comment 13

10 years ago
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.
Flags: wanted1.9.1?

Updated

10 years ago
Priority: -- → P2
Target Milestone: --- → mozilla1.9.1b1
(Assignee)

Updated

10 years ago
Attachment #336609 - Flags: review?(gal) → review+
(Assignee)

Comment 19

10 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Depends on: 455753, 455751
Resolution: --- → FIXED
Someone who actually knows whether we have sufficient tests for this should toggle in-testsuite as needed.

Updated

9 years ago
Keywords: testcase

Comment 23

9 years ago
test already in js1_8_1/trace/trace-test.js
Flags: in-testsuite?
Flags: in-testsuite+
Flags: in-litmus-
(Reporter)

Comment 24

9 years ago
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....

Comment 26

9 years ago
in-testsuite? for performance test.
Flags: in-testsuite+ → in-testsuite?

Comment 27

9 years ago
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.