Closed
Bug 601009
Opened 14 years ago
Closed 14 years ago
TM: allow for guards that always exit
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
(Whiteboard: fixed-in-tracemonkey)
Attachments
(1 file)
24.78 KB,
patch
|
dmandelin
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•14 years ago
|
||
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.
Assignee | ||
Comment 2•14 years ago
|
||
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.
Assignee | ||
Comment 3•14 years ago
|
||
> 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.
Assignee | ||
Comment 4•14 years ago
|
||
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)
Assignee | ||
Updated•14 years ago
|
Attachment #481121 -
Flags: review?(gal) → review?(dmandelin)
Comment 5•14 years ago
|
||
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+
Assignee | ||
Comment 6•14 years ago
|
||
(In reply to comment #5) > > Maybe abortIfAlwaysExits? Fair enough, will do. > The comment could go something like: Nice! I'll incorporate it. Thanks.
Assignee | ||
Comment 7•14 years ago
|
||
Pushed with the suggested changes: http://hg.mozilla.org/tracemonkey/rev/ee28a2d63d3c
Whiteboard: fixed-in-tracemonkey
Comment 8•14 years ago
|
||
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.
Description
•