517973 - remove deep abort and the stack thereof

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

Attachment: v.1

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.

Comment 1

[Graydon Hoare :graydon]

lw++ this bug is awesome. 

anything to cut down on the number of lifecycle states in the monitor+recorder.

Comment 2

[Brendan Eich [:brendan]]

Amen, share it! Ready for review?

/be

Comment 3

[Luke Wagner [:luke]]

Attachment: now with less 'wasRootFragment'

You betcha. Try-server's fine too.

Comment 4

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

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.

Comment 5

[Robert Sayre]

does this get the same perf wins as bug 517329?

Comment 6

[Luke Wagner [:luke]]

(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.

Comment 7

[Luke Wagner [:luke]]

http://hg.mozilla.org/tracemonkey/rev/f3ea03af0e28

Comment 8

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/f3ea03af0e28

Comment 10

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/53da0fcec6fa