488874 - Change tm.onTrace to tm.tracecx

Status: RESOLVED FIXED

Description

[Jason Orendorff [:jorendorff]]

This would eliminate a linear search in js_DeepBail.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: v1

Should have been done in bug 480301.  Better late than never!

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 374101 [details] [diff] [review]
v1

>+     * 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.

/be

Comment 3

[Brendan Eich [:brendan]]

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.

/be

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: interdiff v1 to v2

Just in case anyone's curious what the assertion looks like.

Comment 5

[Jason Orendorff [:jorendorff]]

http://hg.mozilla.org/tracemonkey/rev/3ad791b6a1b4

Comment 6

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/3ad791b6a1b4

Comment 7

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e18d7c1e49d3