Bug 631951 added the -a flag to the shell, to request the methodjit to always be used. But it's not on by default. So our jit-test coverage has gone down. Two steps are needed to fix: - js/src/Makefile.in needs to run --jitflags=m,j,mj,mjp,am,amj,amjp,amd. ("d" puts JM in debug mode, which disables tracing and greatly changes how compilation occurs; dvander says md isn't necessary if we have amd) - If a test has |mjit-always| and --jitflags contains 'a', currently they'll cancel each other out. This should not happen.

I just tried adding an abort to finishThisUp(), which basically means that the JS engine will abort if the method JIT is invoked at all. With a --jitflags=m run of jit_test.py, I got 319 failures out of 1067 tests. That means for 748 of the tests the method JIT isn't being run! Our collective asses are hanging out. Nominating as a hardblocker, even though it's NPOTB, because such a glaring hole in our testing is asking for trouble.

This should be fixed immediately, but I don't see why this should block FF4. There is no indication that any of those 319 tests actually break the method jit. If you find any concrete failures, please file and nominate those. Recommending blocking-.

(In reply to comment #2) > There is no indication that any of those 319 tests actually break the method > jit. It's the 748 that don't invoke the method JIT that I'm worried about!

Sure. Fix it. Test it. If anything breaks and its serious we might have to block on it.

Pretty simple fix. I'm requesting 2.0 approval in advance to avoid some inevitable back-and-forth -- dmandelin has approval powers. The good news is that testBug634590.js is the only failure, for all the configurations involving -a and -m, which bug 636820 already covers: FAILURES: -a -m /home/njn/moz/ws7/js/src/jit-test/tests/basic/testBug634590.js -a -m -j /home/njn/moz/ws7/js/src/jit-test/tests/basic/testBug634590.js -a -m -j -p /home/njn/moz/ws7/js/src/jit-test/tests/basic/testBug634590.js -a -m -d /home/njn/moz/ws7/js/src/jit-test/tests/basic/testBug634590.js

Comment on attachment 515539 [details] [diff] [review] patch pre-approved to land with a=beltzner once it passes review, not before!

Comment on attachment 515539 [details] [diff] [review] patch > def get_test_cmd(path, jitflags, lib_dir): > libdir_var = lib_dir > if not libdir_var.endswith('/'): > libdir_var += '/' > expr = "const platform=%r; const libdir=%r;"%(sys.platform, libdir_var) >- return [ JS ] + jitflags + [ '-e', expr, '-f', os.path.join(lib_dir, 'prolog.js'), >+ # We may have specified '-a' or '-d' twice: once via --jitflags, once >+ # via the "|jit-test|" line. Remove dups because they are toggles. >+ jitflags_no_dups = [] >+ [jitflags_no_dups.append(i) for i in jitflags if not jitflags_no_dups.count(i)] >+ return [ JS ] + jitflags_no_dups + [ '-e', expr, '-f', os.path.join(lib_dir, 'prolog.js'), > '-f', path ] This can be better, e.g.: + return ([ JS ] + list(set(jitflags)) + + [ '-e', expr, '-f', os.path.join(lib_dir, 'prolog.js'), '-f', path ]) (In some parallel universes, I would prefer to have a class for building toggle flag options but that is overkill for something that is likely about to change anyway.) r=me with that version or similarly elegant dup elimination. I tried the patch locally and I got some failures on testBug634590, bug540187, and bug540243. I'm not sure if that's real or what, but of course this can land and stick only if we're still all green with it.

I applied the patch from bug 637390 as well, since it has r+ and is NPOTB -- it just increases the testing coverage a bit more by including interpreter-only runs. http://hg.mozilla.org/tracemonkey/rev/f85299200bd3