Closed Bug 488874 Opened 12 years ago Closed 12 years ago

Change tm.onTrace to tm.tracecx


(Core :: JavaScript Engine, defect)

Other Branch
Not set





(Reporter: jorendorff, Assigned: jorendorff)


(Keywords: fixed1.9.1, Whiteboard: fixed-in-tracemonkey)


(2 files)

This would eliminate a linear search in js_DeepBail.
Assignee: general → jorendorff
Attached patch v1Splinter Review
Should have been done in bug 480301.  Better late than never!
Attachment #374101 - Flags: review?(brendan)
Attachment #374101 - Flags: review?(brendan) → review+
Comment on attachment 374101 [details] [diff] [review]

>+     * The context currently executing JIT-compiled code on this thread, or
>+     * NULL if none. Among other things, this can in certain cases prevent
>+     * last-ditch garbage collection and suppress calls to
>+     * JS_ReportOutOfMemory.

Nit: s/garbage collection/GC when in Rome and for prettier wrapping.

>     uintN                   prohibitFlush;
>     JSBool                  needFlush;
>     /*
>      * reservedObjects is a linked list (via fslots[0]) of preallocated JSObjects.
>      * The JIT uses this to ensure that leaving a trace tree can't fail.
>      */
>+    JSPackedBool            useReservedObjects;

Make needFlush a JSPackedBool too. Could pack prohibitFlush if we can bound tree nesting by 2^16 -- not a big deal, just wondering if there is a tree nesting limit other than memory limits (code cache size, no doubt).

>+ * Reconstruct the JS stack and clear cx->tracecx. We must be currently
>+ * executing a _FAIL builtin from trace on cx or another context on the same
>+ * thread. The machine code for the trace remains on the C stack when
>+ * js_DeepBail returns.

Nit: s/executing/in/ for better wrapping and conciseness.

>+    tm->prohibitFlush++;

Is there an assertion to add to LeaveTree after the if (bailed) {...} that prohibitFlush is zero? Or something to handle the questions begged by the asymmetry between the ++ and the --?

Looks great, this is wanted1.9.1+, I hope.

Flags: wanted1.9.1?
I think we want this. Carrying a diff against tm and m-c is a small pain in merge overhead but bigger in cognitive load. And this tightens up the code, requiring less reasoning. Especially if we can get that assertion I'm hoping for.

Just in case anyone's curious what the assertion looks like.
Whiteboard: fixed-in-tracemonkey
Closed: 12 years ago
Flags: wanted1.9.1? → wanted1.9.1+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.