Closed Bug 601009 Opened 14 years ago Closed 14 years ago

TM: allow for guards that always exit

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: n.nethercote, Assigned: n.nethercote)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file)

Bug 600779 changes jstracer.cpp to account for jumps that are always taken, thus avoiding the "Constantly taken branch detected" assertion failure.  The aim of this bug is similar, to account for guards that always exit, in order to avoid the "Constantly false guard detected" assertion failure.

If this can be done in a general way, it should fix bug 596823.  It's not yet clear to me if this can be done in a general way, but I'll give it a try.
The general solution:

- guard() must work out for itself if the guard will always exit (which is easy);

- guard() must return a RecordingStatus (or AbortableRecordingStatus?  I haven't worked out the difference between them), and all callers of guard() should check it.  Unfortunately there are more than 100 guard() calls in jstracer.cpp and propagating status results through all those calls is a big task.  And for a lot of the guards the comparison will never be always-true, eg. any comparison where one of the operands is a load.

So I'm not sure what to do.  Playing whack-a-mole with these assertions as they pop up isn't much fun.
Actually, here's what I'll do.  I'll add an alternative version of guard() that returns a RecordingStatus.  It doesn't need to be used everywhere immediately, it can be incorporated as needed.  For example, bug 596823 hits this assertion in relation to makeNumberInt32(), so I can change makeNumberInt32() to use the alternative guard().  Only a few places need to be changed.  If we hit the assertion again later, we can rinse and repeat.

It's still whack-a-mole, but at least we'll be able to use the same whack every time a new mole pops up.
> It's still whack-a-mole, but at least we'll be able to use the same whack every
> time a new mole pops up.

The good thing about considering each case as it appears is that sometimes you don't want to abort recording.  Eg. see bug 570663.
This patch:

- Changes guard() to return a RecordingStatus.  However, in most places it 
  doesn't need to be consulted.  So guard() now has an extra argument (which
  defaults to false);  if it's true, the caller promises to check the return
  value.  If it's false and the guard is optimized to always exit, we'll 
  assert.
  
  So this provides a general mechanism for dealing with 
  optimized-to-always-exit guards, while allowing for it to be deployed 
  incrementally as needed.  (Deploying it fully would be overkill and a huge
  pain.)  If, in the future, an unhandled optimized-to-always-exit guard 
  pops up, guard() will assert and there's a nice big comment explaining how
  to handle it.
  
- Uses this new behaviour of guard() for makeNumberInt32(), which can be 
  determined to alway fail (causing an exit) if the input is a fractional 
  immediate.  
  
- Moves the second overloading of guard() next to the first one.

- Adds code to detect the out-of-bounds case for String.charAt(), like the 
  code already present String.charCodeAt().  (Stolen from the draft patch in
  bug 596823.)

- Adds a trace-test based on the one from bug 596823.

Nb: I used RecordingStatus rather than AbortableRecordingStatus as the 
return type for guard() and makeNumberInt32().  I wasn't sure about this, so
this should be checked by the reviewer.
Attachment #481121 - Flags: review?(gal)
Attachment #481121 - Flags: review?(gal) → review?(dmandelin)
Blocks: 596823
Blocks: 600127
Comment on attachment 481121 [details] [diff] [review]
patch (against TM 54871:c44177df3ee3)

Looks good to me. IIUC, AbortableRecordingStatus is needed only if the function can re-enter the interpreter, which can't happen in guard() because it doesn't execute any JS.

I did find the usage of this new API a little confusing (and the old one too). It is probably partially because I haven't looked at this stuff in a while, but it would nice to lay it out clearly in the top comment. Also, checkForAbort confused me, since I expect a parameter name to be a command to the caller, not the callee. Maybe abortIfAlwaysExits?

The comment could go something like:

---
Callers shouldn't generate guards that always exit, because traces that always exit don't help us run fast. This function can handle an always-exit guard in two ways, controlled by abortIfAlwaysExits:

abortIfAlwaysExits == false: This is the default. If the guard will always exit, we assert (in debug builds) as a signal that we are generating bad traces. In this mode, the caller must not generate an always-exit guard. The return value will always be RECORD_CONTINUE, so the caller shouldn't check it.

abortIfAlwaysExits == true:  If the guard will always exit, we abort recording. This can be used when the caller doesn't know ahead of time whether the guard will always fail. In this mode, the caller must check the return value.
---

Anyway, just a thought. (And by the way, "conditon" is a typo in the original comment.)
Attachment #481121 - Flags: review?(dmandelin) → review+
(In reply to comment #5)
>
> Maybe abortIfAlwaysExits?

Fair enough, will do.

> The comment could go something like:

Nice!  I'll incorporate it.  Thanks.
Pushed with the suggested changes:

http://hg.mozilla.org/tracemonkey/rev/ee28a2d63d3c
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/ee28a2d63d3c
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.