Status

()

defect
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

({fixed1.9.1})

Other Branch
Points:
---
Bug Flags:
wanted1.9.1 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(2 attachments)

This would eliminate a linear search in js_DeepBail.
Assignee: general → jorendorff
Posted 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]
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
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.

/be
Just in case anyone's curious what the assertion looks like.
http://hg.mozilla.org/tracemonkey/rev/3ad791b6a1b4
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/3ad791b6a1b4
Status: NEW → RESOLVED
Closed: 10 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.