621096 - Update jit-tests since HOTLOOPS has gone up.

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Paul Biggar]

I pushed a change really which broke some things, despite the fact that it passed jit-tests. Looking one level up, there is a problem that many of our jit-tests are outdated: they were written when HOTLOOPS was 2 - it is now 8 - so many of our tests no longer trace. They all need to be fixed.

Comment 1

[Paul Biggar]

Attachment: Script to assist updating tests

This is the script I used to process the tests. It moves through test cases, presenting information and prompting for action. For each test case, it shows:

 - the author
 - the revision in which it was added
 - the value of HOTLOOPS when it was added
 - the commit message in which it was added
 - all the lines in the files containing for-loop headers (or the entire file if there aren't any)
 - the tracer stats


It gives options to:

 - edit the file
 - replace for-loop less-than operands with RUNLOOPS (ie 'i<9' -> 'i<RUNLOOPS').
 - move between files
 - replace the expected output with the actual output (for when the output is based on the length of the loop, which is all the time)
 - hg revert the file

Comment 2

[Paul Biggar]

Attachment: jit-tests diff

This changes nearly all tests to use RUNLOOP, rather than its old value. I did this with the script I posted above, and paid very little attention to the tests themselves, except to see if it fit into one of the 6 or so patterns I recognized.

In general, the rules I used were:

 - If the test was written when HOTLOOP was 2 or 4:
   - if it contained a for-loop which iterates less-than 8 times, and the tracer wasnt active, then I replaced the loop count with RUNLOOP to make the tracer run
     - sometimes I used RUNLOOP+1 or RUNLOOP+2, if it meant that monitor:exits and monitor:triggered went from 0 to 1
   - if it used an array of length similar to the old HOTLOOP, I made the array longer, repeating the contents twice or three times, to make the tracer run
   - if I couldnt make the tracer run, I added a comment to the file (to make it show in the diff), and left it alone otherwise

 - If the test used a loop counter of 10-ish, I switched it to RUNLOOP/RUNLOOP+1

 - in nearly every case, I fixed the output automatically, without paying attention to whether that was the right thing to do. In some cases, this replaces the right output with the wrong output. Again, this needs review.

- I made some notes when I couldnt get things to trace with any loop counter

- there were various edge cases, but a review of the diff shows me I largely messed with for-loops.


Because of how I wrote the script to help here, I accidentally removed trailing whitespace and windows line endings from changed tests - I'll call this a bonus. (I excluded this from the diff, for clarity)

This needs review, I'm not sure what to do here: the patch is 280K (8000 lines). I could split it up based on the author of the test, and ask for reviews individually.

I intend to measure coverage of the tracer before and after this change.

Comment 3

[Nicholas Nethercote [inactive]]

There's a similar bug already open on this issue, but I can't find it :/

Comment 4

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

I tried to do something like this myself, but I feel like it's a losing battle. Here are the problems I ran into:

1. Many of the tests check some actual output against a string of expected output values. We can make the test independent of HOTLOOP by replacing fixed loop constants with RUNLOOP, but it's much harder to get the value of |expected| to contain the right number of values. It looks like your patch just hard codes for HOTLOOP==8 in these cases, which isn't really solving the real problem.

2. Although checkStats is off right now, we'd like to turn it on. And when we do, changing the number of loop iters will screw up some of the checkStats numbers as well.

That's why I would rather allow the user to provide a value for HOTLOOP. Then we can run with HOTLOOP==2, and everything will pretty much work as it's supposed to. Or we can try different values of HOTLOOP and make sure nothing breaks (although checkStats has to be off in this case).

That said, I think it's a good idea to make as many tests as possible be HOTLOOP-independent. I'd be in favor of a more conservative change that only affects tests that satisfy the following conditions:
1. The test doesn't use checkStats.
2. You only change for loop bounds, and you leave |expected| values alone.

But I'm sure other people will have their own opinions.

Comment 5

[Nicholas Nethercote [inactive]]

(In reply to comment #3)
> There's a similar bug already open on this issue, but I can't find it :/

Ah, it's bug 601990, and you've already commented on it.  The two bugs have the same root cause, which is that HOTLOOP changed, right?

Comment 6

[Boris Zbarsky [:bzbarsky]]

> 1. The test doesn't use checkStats.

The thing is, if you don't use checkStats you have no way to tell that you actually ended up on trace and hence are testing the right thing...

Comment 7

[Paul Biggar]

(In reply to comment #4)
> 1. Many of the tests check some actual output against a string of expected
> output values. We can make the test independent of HOTLOOP by replacing fixed
> loop constants with RUNLOOP, but it's much harder to get the value of
> |expected| to contain the right number of values. It looks like your patch just
> hard codes for HOTLOOP==8 in these cases, which isn't really solving the real
> problem.

I didn't do this, but we could abstract some of the loop code to replace:

   [x,x,x,x,x,x,x,x]

with

   HOTARRAY(x)

Similarly for repeating sequences: [Number, String, Array] -> HOTARRAY([Number, String, Array])

In the second case, its hard to tell that we're triggering the same behaviour we originally did when the test was written.


> 2. Although checkStats is off right now, we'd like to turn it on. And when we
> do, changing the number of loop iters will screw up some of the checkStats
> numbers as well.

I agree, we need to turn this back on. Perhaps check_stats should check for the presence or absense of a field, rather than specific values. So test 'traces and aborts' or 'traces and doesnt abort'. There are a few tests that trace many times, but this isn't nearly so common as tests that trace just once.



> That's why I would rather allow the user to provide a value for HOTLOOP. Then
> we can run with HOTLOOP==2, and everything will pretty much work as it's
> supposed to. Or we can try different values of HOTLOOP and make sure nothing
> breaks (although checkStats has to be off in this case).

I presume you mean the option would go in spidermonkey, not in jit_tests.py? I like this as a migration step, but don't like adding configuration options to spidermonkey.


 
> That said, I think it's a good idea to make as many tests as possible be
> HOTLOOP-independent. I'd be in favor of a more conservative change that only
> affects tests that satisfy the following conditions:
> 1. The test doesn't use checkStats.
> 2. You only change for loop bounds, and you leave |expected| values alone.

This could be strengthened by adding checkStats calls on these tests, and filing bugs on the tests which don't trace?

Comment 8

[Paul Biggar]

(In reply to comment #4)
> That said, I think it's a good idea to make as many tests as possible be
> HOTLOOP-independent. I'd be in favor of a more conservative change that only
> affects tests that satisfy the following conditions:
> 1. The test doesn't use checkStats.
> 2. You only change for loop bounds, and you leave |expected| values alone.

How about this:

 - let H be the revision in which we changed HOTLOOP from 2
 - in revision H-1, run all the tests with just -j
 - add a checkStats-like call at the end, which tests for binary conditions:
   - does it trace?
   - does the trace abort?
   - etc?

Comment 9

[Paul Biggar]

The only actual bugs this found was bug 632642, and fixed separately, so the remaining benefit from this bug is to improve coverage.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Obsolete with the removal of tracejit.