601990 - Reenable checkStats and fix any regressions this uncovers

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Looks like we just disabled stats checking.  This means that we won't catch cases where we should trace but don't in various simple cases (e.g. bug 601968's test would use this).  Before ship we probably need to fix this and then fix whatever's been broken in the meantime.

Comment 1

[Luke Wagner [:luke]]

Hmm, this is going to take a large amount of time.  Should this really be blocking?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Well, that sort of depends on whether we think there are regressions that need to be fixed or whether it's all just noise, right?

Comment 3

[Jeff Walden [:Waldo]]

We know there have been regressions (jorendorff could point out one).  Without doing this work, we won't know the full extent.  And generally, tracing bugs are quite often type errors that are exploitable, or are very difficult to prove non-exploitable (beyond proof by casual inspection).  That unknown unknown, and the magnitude of the consequences if we are wrong, makes me think this should still block.

Comment 4

[Nicholas Nethercote [inactive]]

(In reply to comment #3)
> And generally, tracing bugs
> are quite often type errors that are exploitable

Can you clarify?  Nanojit has LIR type checker that runs in debug mode.  The type system is simple (int, quad, double) but it does catch real errors.  Or maybe you meant something else?

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to comment #3)
> We know there have been regressions (jorendorff could point out one).  Without
> doing this work, we won't know the full extent.

I'm not sure I understand this bug. Is the purpose to ensure that we're exercising the tracer in our test cases? It seems like we can turn off the profiler to do that. Or is the idea to find performance problems where we're not tracing code that we should be tracing?

Comment 6

[Jeff Walden [:Waldo]]

Yes, I meant things like that.  Your type checker can't find problems if we don't exercise the corresponding code in our tests.  And even then, it only detects type-size differences (mostly) -- still extremely helpful, but not perfect.  LIR type checking is great, but if we're not running the tests that trigger the type errors, it's not going to reveal problems -- problems someone willing to do the work we might end up being too lazy to do could find without a whole lot of effort.

(In reply to comment #5)
> I'm not sure I understand this bug. Is the purpose to ensure that we're
> exercising the tracer in our test cases? It seems like we can turn off the
> profiler to do that. Or is the idea to find performance problems where we're
> not tracing code that we should be tracing?

The problem is our tests implicitly relied on loop-tracing limit values that have since changed, which caused us to disable those stats checks -- checks which ensured we actually were executing to the point of recording or tracing the desired code, or of then executing traced code.

Profiling makes it harder to write robust limit checks, but I don't think it's relevant to the question of whether a test successfully exercises particular code by getting the loop limits exactly right.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Is the purpose to ensure that we're exercising the tracer in our test cases?

Yes.

> It seems like we can turn off the profiler to do that

That's not enough; the point is that right now we have tests that assert as part of the test that the tracer has been exercised and that fail if those asserts are turned back on (said turning on being what this bug is about).  Which means the tracer is not being exercised.  This is true no matter what mode you run the tests in (for one thing, HOTLOOP is no longer 2!).

> Or is the idea to find performance problems 

For tests that assert things about how many times we enter/exit trace, you could detect performance regressions by reenabling stats, yes....  Sadly, I expect some of them are fragile in the sense that the test asserts 1 trace transition but failing in the way of the original bug would only lead to 2 transitions in the test...  So the failures would have to actually be looked at.  Just slightly bumping the numbers to match may well hide real problems.

Comment 8

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

It seems fine to assert that the tracer was exercised, but getting the exact counts right for all time, across various values of HOTLOOP, seems like a trail of tears. Keep in mind we don't assert on whether the Method JIT, or its ICs, were exercised at all, either - and there are still unimplemented opcodes.

What would we do about tests that loop over the contents of an array, and worse, the regression involves some kind of type instability? checkStats will fail, and it won't be clear at all how to make the test exercise its original path again, if that path is even still relevant. I've also seen that adjusting HOTLOOP exposes different bugs (for example, whether it's odd or even, or past a certain threshold).

What might be more useful is letting HOTLOOP be configurable so jit-tests can try different values (say, 2, 3, and default), and then making checkStats only work against one known value.

Having this enhanced coverage would be great, but I'm not sure about blocking a release on the possibility that some tests aren't being run as exactly intended 12+ months ago.

Comment 9

[Nicholas Nethercote [inactive]]

(In reply to comment #8)
> 
> What might be more useful is letting HOTLOOP be configurable so jit-tests can
> try different values (say, 2, 3, and default), and then making checkStats only
> work against one known value.

How about making it configurable in the shell via a command-line argument, and always setting it to 2 for the jit-tests?  It was 2 for a long time, during which time most of these tests were probably written.  I don't see why you'd want to try multiple HOTLOOP values.

Hmm, that would definitely work for --jitflags=j, but will be dicey for --jitflags=jm and definitely won't work for --jitflags=m.  Does it make sense to change things so that we only run checkStats when --jitflags=j?

Comment 10

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

Because different values can expose different bugs. But that might be more useful for fuzzing.

Comment 11

[Jeff Walden [:Waldo]]

Issues like bug 618362 comment 9 are why I (still) think this bug (or some reasonable facsimile in which our existing checkStats-using tests, however massaged, actually run) should block, for what it's worth.

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch to re-enable checkStats

Since this isn't a blocker, I'll just post what I have so far. This patch adds a cmd line param for HOTLOOP. I re-enabled checkStats, but only when HOTLOOP == 2. We get the following test failures in that case:

    -j -m /home/billm/mozilla/tm8/js/src/jit-test/tests/basic/testGCWhileRecording.js
    -j -m /home/billm/mozilla/tm8/js/src/jit-test/tests/basic/testMethodInit.js
    -j -m /home/billm/mozilla/tm8/js/src/jit-test/tests/basic/testMethodOverride.js
    -j -m /home/billm/mozilla/tm8/js/src/jit-test/tests/basic/testNewString.js
    -j -m /home/billm/mozilla/tm8/js/src/jit-test/tests/basic/testTypedArrays.js
    -j -m /home/billm/mozilla/tm8/js/src/jit-test/tests/basic/testUndefinedIncrement.js

None of them seem terribly important, so I'll just leave things as they stand for now.

Comment 13

[Paul Biggar]

I've tried a different approach to a similar problem, on bug 621096. Not quite a dup though.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Obsolete with the removal of tracejit.