Closed Bug 517973 Opened 15 years ago Closed 15 years ago

remove deep abort and the stack thereof

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- beta1-fixed

People

(Reporter: luke, Assigned: luke)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 1 obsolete file)

Attached patch v.1 (obsolete) — Splinter Review
The logic concerning deep abort and the stack of deep aborted recorders is somewhat tricky, and confounded by the way it touches all over the code (bug bug 517329 is an example).  Some of this complexity is essential, viz., the problem of what to do when we reenter the interpreter from recording code, an executing trace, or the interpreter.  However, I think we can make it a bit simpler by removing the "wasDeepAborted" state and the stack of deep aborted recorders, and just calling js_AbortRecording whenever we would have called deepAbort().

So, the initial worry is "but what about all the dangling pointers to recorders on the stack?"  Well, we already need to check |wasDeepAborted| immediately after any call in the recorder that may reenter the interpreter (since the recorder state is now moot), so this just changes checks of |wasDeepAborted| to TRACE_RECORDER(cx) != NULL.  It is a little more delicate, because need need to check *immediately* after (lest we access member variables of a deleted recorder), but the situation arises in the same way.  This delicacy is also somewhat good: valgrind will now immediately catch uses of an aborted recorder as invalid memory accesses instead of much later if/when the recorder/interpreter state discord manifests itself in an error a la bug 463829 bug 467857 bug 500108.

Another benefit is that, since recorders die immediately, the TraceRecorder::fragment pointer cannot become invalid and so does not need to be null-ed on JIT reset, and hence there is no fragment == NULL state to watch out for.

There is also a deepAbort() from the GC, however, Andreas says this was from when the recorder was GC-unsafe.  We should be safe now, so we can keep recording and only worry about shapes getting changed.  However, this is already handled by the needFlush flag, so no problem.

Initial work attached.
lw++ this bug is awesome. 

anything to cut down on the number of lifecycle states in the monitor+recorder.
Amen, share it! Ready for review?

/be
You betcha. Try-server's fine too.
Attachment #401919 - Attachment is obsolete: true
Attachment #401997 - Flags: review?
Attachment #401997 - Flags: review? → review?(dvander)
Comment on attachment 401997 [details] [diff] [review]
now with less 'wasRootFragment'


Glad to see this stuff go - especially wasRootFragment and the unnecessary extra Assembler checks that added to error handling confusion.

>+    void pushThisOntoAbortStack();
>+    void popThisFromAbortStack();
>+    void onJITFlush();

Dead code, r=me with that gone.
Attachment #401997 - Flags: review?(dvander) → review+
does this get the same perf wins as bug 517329?
(In reply to comment #5)
*sigh* It seems my measured performance improvement was a fluke or mistake.  See bug 517329 comment 12.  I did test though and this patch correctly blacklists the loops from bug 517329.
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/f3ea03af0e28
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Flags: wanted1.9.2+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: