This must be fixed at the same time as bug 462027 to avoid regressing for-in loops in 462027 (a two-way dependency, which bugzilla prohibits). The fix in bug 462027 allows certain native functions to bail off trace in midflight. Calls to such functions are followed by a special guard that detects when the native function bailed off trace (and reconstituted the JS stack) but then successfully completed. First I tried having record_JSOP_ITER and record_JSOP_NEXTITER just emit special guards after calls to js_FastValueToIterator and js_FastCallIteratorNext. Using the new stuff in 462027 to fix this problem. This is hairy. See the attached (buggy and incomplete) patch. I think I could use imacros instead, and make js_FastValueToIterator and js_FastCallIteratorNext traceable natives which the imacros simply CALL. Which approach is better?

Try imacros, they are the new hawtness ;-). We use 'em already for JSOP_ITER and JSOP_NEXTITER in the hasIteratorMethod(v) case of JSOP_ITER, and in JSOP_NEXTITER when the iterator object is an instance of js_IteratorClass. /be

Jason, I'm hoping you'll take this one too. /be

This needs to be fixed for beta, so P1.

Very well. I'll try imacros Monday.

Ping me on IRC about anything, let me know if I can help. /be

This applies on top of the patches in bug 462027 and bug 462042. The good news is, it fixes the bug like magic. None of the regrtests hit my "don't reenter except through _FAIL traceable natives" assertion with this patch. The bad news is I had to add two new opcodes to do it, JSOP_CALLBUILTIN and JSOP_CHECKHOLE. They are used only by the new imacros. It all feels fairly kludgey to me; advice welcome.

Comment on attachment 356855 [details] [diff] [review] v1 >+ BEGIN_CASE(JSOP_CALLBUILTIN) >+ { Nit: no unnecessary braces in opcode interpreter cases. >+ obj = js_GetBuiltinFunction(cx, GET_INDEX(regs.pc)); >+ if (!obj) >+ goto error; >+ rval = FETCH_OPND(-1); >+ STORE_OPND(-1, OBJECT_TO_JSVAL(obj)); >+ PUSH_OPND(rval); This is a total nit, but it might be less stressful for anyone worried about rooting to see rval always referenced by live stack: push rval before store at -2. >+ } >+ END_CASE(JSOP_CALLBUILTIN) >+ >+ BEGIN_CASE(JSOP_CHECKHOLE) >+ { >+ rval = FETCH_OPND(-1); >+ PUSH_OPND(BOOLEAN_TO_JSVAL(rval != JSVAL_HOLE)); >+ } >+ END_CASE(JSOP_CHECKHOLE) You avoid adding this op by coding hole;stricteq in the imacro. >+/* Builtin functions. */ >+#define BUILTIN_FUNCTION_FastObjectToIterator 0 >+#define BUILTIN_FUNCTION_FastCallIteratorNext 1 Couple of nits: BUILTIN_ seems enough, all builtins are functions; "Fast" seems unnecessary now too. Would an enum be better? C++ might require casts to jsbytecode of imacro_asm.js.in, yay. >+/* Functions for use with JSOP_CALLBUILTIN. */ >+ >+static JSBool >+FastObjectToIterator(JSContext *cx, uintN argc, jsval *vp) >+{ >+ jsval *argv = JS_ARGV(cx, vp); >+ JS_ASSERT(JSVAL_IS_INT(argv[0])); >+ return js_ValueToIterator(cx, JSVAL_TO_INT(argv[0]), &JS_RVAL(cx, vp)); >+} Is this function necessary? I mean, yeah, it is for the tn stuff in full, but is there a lower level of macro to use that avoids the non-FASTCALL JSFastNative peer? It doesn't seem to be used. Separate and lower priority bug at most. >+static JSBool >+FastCallIteratorNext(JSContext *cx, uintN argc, jsval *vp) >+{ >+ return js_CallIteratorNext(cx, JS_THIS_OBJECT(cx, vp), &JS_RVAL(cx, vp)); >+} Ditto. >+JSObject * >+js_GetBuiltinFunction(JSContext *cx, uintN index) >+{ >+ JSRuntime *rt = cx->runtime; >+ JSObject *f = rt->builtinFunctions[index]; >+ if (!f) { Nit: canonical name is obj or funobj not f. +JS_REQUIRES_STACK bool >+TraceRecorder::record_JSOP_CHECKHOLE() >+{ >+ jsval& v = stackval(-1); >+ >+ if (JSVAL_TAG(v) == JSVAL_BOOLEAN) >+ stack(0, lir->ins2(LIR_eq, get(&v), lir->insImm(JSVAL_TO_BOOLEAN(JSVAL_HOLE)))); >+ else >+ stack(0, INS_CONST(JS_TRUE)); >+ return true; >+} Yeah, trace type inducing a constant strongly suggests this should be recoded using existing ops JSVAL_HOLE and JSVAL_STRICTEQ. Looks great otherwise -- I never liked the resumeAfter and record_IteratorNextComplete hacks, this is nice. I'd even hope for more use of JSOP_CALLBUILTIN, so it can stay. ;-) /be

Assertion failure: tm->reservedDoublePoolPtr > tm->reservedDoublePool also occurs in browser with js1_6/extensions/regress-455464-04.js on 1.9.1-tracemonkey, mozilla-central.

Thanks for the review. The right sequence to replace JSOP_CHECKHOLE turns out to be: dup; hole; strictne.

Need a dup, right. > >+/* Functions for use with JSOP_CALLBUILTIN. */ > >+ > >+static JSBool > >+FastObjectToIterator(JSContext *cx, uintN argc, jsval *vp) > >+{ > >+ jsval *argv = JS_ARGV(cx, vp); > >+ JS_ASSERT(JSVAL_IS_INT(argv[0])); > >+ return js_ValueToIterator(cx, JSVAL_TO_INT(argv[0]), &JS_RVAL(cx, vp)); > >+} > > Is this function necessary? I mean, yeah, it is for the tn stuff in full, but > is there a lower level of macro to use that avoids the non-FASTCALL > JSFastNative peer? It doesn't seem to be used. Separate and lower priority bug > at most. Any thoughts? I didn't know if this was missed. I'll review the v2 patch in a moment. > >+ JSObject *f = rt->builtinFunctions[index]; > >+ if (!f) { > > Nit: canonical name is obj or funobj not f. This too, but obviously a minor nit. /be

Comment on attachment 357045 [details] [diff] [review] v2 > * JSOP_NEXTITER stores the next iterated value in the top of stack slot which > * was allocated by JSOP_ITER and pushes true, or stores JSVAL_HOLE and pushes >- * false. It is followed immediately by JSOP_IFNE{,X}. >+ * false. It is followed immediately by JSOP_IFNE{,X}. (JSOP_NEXTITER has >+ * JOF_TMPSLOT2 because there is an imacro implementation of it that uses an >+ * extra stack slot.) Could you try writing imacros.jsasm source instead of adding TMPSLOT2 here and extending my first hand-crafted imacros in jstracer.cpp? Then there's no stack budget magic. I'll help if imacro_asm.js.in needs extending. /be

(In reply to comment #13) > > >+/* Functions for use with JSOP_CALLBUILTIN. */ > > >+ > > >+static JSBool > > >+FastObjectToIterator(JSContext *cx, uintN argc, jsval *vp) > > >+{ [...] > > > > Is this function necessary? [...] Sorry I didn't respond to this earlier. The boring JSFastNative version is called from the interpreter when recording. > > Nit: canonical name is obj or funobj not f. > > This too, but obviously a minor nit. An oversight, corrected in v4. > Could you try writing imacros.jsasm source instead of adding TMPSLOT2 here and > extending my first hand-crafted imacros in jstracer.cpp? You're right, that was a lot more fun. :)

I think comment 0 is wrong about the mutual dependency. I think I can land this before bug 462027. If I can, I should; they're both significant enough changes to warrant separate Tinderbox spins. Transposed patches to come, if it works out for me.

Transposed. This is the same as v4 except many lines of context changed.

Comment on attachment 357835 [details] [diff] [review] v5 >+ if (info.flags.indexOf("JOF_UINT16") >= 0) { >+ if (/^\(/.test(op.imm1)) >+ return '(_ & 0xff00) >> 8, (_ & 0xff)'.replace(/_/g, op.imm1); Nit: 4-space indentation needed. >+ let m = /(?:(\w+):)?\s*(\.?\w+)(?:\s+(\w+|\([^)]*\)))?(?:\s+([\w-]+|\([^)]*\)))?(?:\s*(?:#.*))?$/.exec(a[i]); I should have used this old chestnut in global code: let line_regexp_parts = [ "^(?:(\w+):)?", "\s*(\.?\w+)", "(?:\s+(\w+|\([^)]*\)))?", "(?:\s+([\w-]+|\([^)]*\)))?", "(?:\s*(?:#.*))?$" ]; let line_regexp = new RegExp(line_regexp_parts.join("")); Also not the missing ^ in first part -- I think that's needed to avoid skipping arbitrary junk at the front (duh on me for forgetting). > error: > if (fp->imacpc && cx->throwing) { > // To keep things simple, we hard-code imacro exception handlers here. > if (*fp->imacpc == JSOP_NEXTITER) { >- JS_ASSERT(*regs.pc == JSOP_CALL); >+ JS_ASSERT(*regs.pc == JSOP_CALL || *regs.pc == JSOP_DUP); Asked over IRC, seems JSOP_DUP should not be allowed by this assertion. r=me with nit and assert fixes. line_regexp at your discretion. /be

s/Also not/Also note/ /be

It turns out that pc can point to JSOP_DUP there due to bug 474854. (It hadn't occurred to me that that behavior is a bug, but it is.) js1_8/extensions/regress-455973.js triggers that.

Backed out. Suspected of causing leaks. Possible mechanism: rt->builtinFunctions could be entraining the global object via __parent__. The comment says null parent, but I think js_NewFunction is assigning a parent anyway.

Sorry, I should have seen that. Solution alternatives: 1. Call js_NewObjectWithGivenProto first and pass that funobj into js_NewFunction. 2. STOBJ_CLEAR_PROTO(funobj) after the js_NewFunction call returns. Alternative 2 is strictly less code. /be

Confirmed to leak and backout out (again, after re-landing it). http://hg.mozilla.org/tracemonkey/rev/89a99401da84

Fix probable leak and a possible thread safety issue.

Sorry, that interdiff is a bit too hasty; the actual v6 patch adds another line: STOBJ_CLEAR_PROTO(funobj); + STOBJ_CLEAR_PARENT(funobj); Debugger tells me that js_NewFunction chooses both a proto and a parent for you.

Comment on attachment 358927 [details] [diff] [review] v6 >+JSObject * >+js_GetBuiltinFunction(JSContext *cx, uintN index) >+{ >+ JSRuntime *rt = cx->runtime; >+ JS_LOCK_GC(rt); >+ JSObject *funobj = rt->builtinFunctions[index]; >+ if (!funobj) { >+ /* Use NULL parent and atom. Builtin functions never escape to scripts. */ >+ JSFunction *fun = js_NewFunction(cx, >+ NULL, >+ (JSNative) builtinFunctionInfo[index].tn, >+ builtinFunctionInfo[index].nargs, >+ JSFUN_FAST_NATIVE | JSFUN_TRACEABLE, >+ NULL, >+ NULL); >+ if (fun) { >+ rt->builtinFunctions[index] = funobj = FUN_OBJECT(fun); >+ STOBJ_CLEAR_PROTO(funobj); >+ STOBJ_CLEAR_PARENT(funobj); >+ } >+ } >+ JS_UNLOCK_GC(rt); I mentioned this on IRC, may have missed a reply saying why not: if rt->buildinFunctions[i] never goes null after becoming non-null, avoid taking the GC lock until you have loaded null once. You'll have to retest in case you lost a race, but then the fast path is lock free. /be

That interdiff is -b ...the actual v7 patch shows correct indentation.

Comment on attachment 358937 [details] [diff] [review] v7 Sorry, don't mean to drip these comments out like water torture, but this needs to avoid self-locking on the GC lock: >+JSObject * >+js_GetBuiltinFunction(JSContext *cx, uintN index) >+{ >+ JSRuntime *rt = cx->runtime; >+ JSObject *funobj = rt->builtinFunctions[index]; >+ >+ if (!funobj) { >+ JS_LOCK_GC(rt); >+ funobj = rt->builtinFunctions[index]; /* Retest now that the lock is held. */ >+ if (!funobj) { >+ /* Use NULL parent and atom. Builtin functions never escape to scripts. */ >+ JSFunction *fun = js_NewFunction(cx, >+ NULL, >+ (JSNative) builtinFunctionInfo[index].tn, >+ builtinFunctionInfo[index].nargs, >+ JSFUN_FAST_NATIVE | JSFUN_TRACEABLE, >+ NULL, >+ NULL); >+ if (fun) { >+ rt->builtinFunctions[index] = funobj = FUN_OBJECT(fun); >+ STOBJ_CLEAR_PROTO(funobj); >+ STOBJ_CLEAR_PARENT(funobj); >+ } >+ } >+ JS_UNLOCK_GC(rt); >+ } >+ return funobj; Juggle things so on a null load, you make a new function, and only store in rt->builtinFunctions inside the lock and within a null retest. Race loser makes a bit of garbage, no big deal. /be

Good point.

Comment on attachment 358942 [details] [diff] [review] v8 >+js_GetBuiltinFunction(JSContext *cx, uintN index) >+{ >+ JSRuntime *rt = cx->runtime; >+ JSObject *funobj = rt->builtinFunctions[index]; >+ >+ if (!funobj) { >+ /* Use NULL parent and atom. Builtin functions never escape to scripts. */ >+ JSFunction *fun = js_NewFunction(cx, >+ NULL, >+ (JSNative) builtinFunctionInfo[index].tn, >+ builtinFunctionInfo[index].nargs, >+ JSFUN_FAST_NATIVE | JSFUN_TRACEABLE, >+ NULL, >+ NULL); >+ if (fun) { >+ funobj = FUN_OBJECT(fun); >+ STOBJ_CLEAR_PROTO(funobj); >+ STOBJ_CLEAR_PARENT(funobj); >+ >+ JS_LOCK_GC(rt); >+ if (!rt->builtinFunctions[index]) /* retest now that the lock is held */ >+ rt->builtinFunctions[index] = funobj; >+ JS_UNLOCK_GC(rt); >+ } >+ } >+ return funobj; >+} Although the builtin should never escape, it seems better to do the usual trick here of returning the one true funobj, and letting the dup created by the losing racer become garbage right away: >+ JS_LOCK_GC(rt); >+ if (!rt->builtinFunctions[index]) /* retest now that the lock is held */ >+ rt->builtinFunctions[index] = funobj; else funobj = rt->builtinFunctions[index]; >+ JS_UNLOCK_GC(rt); Could use another JSObject * temporary, but no need with any compiler worthy of the name. /be

Caused failures across all platforms. Backed out. http://hg.mozilla.org/tracemonkey/rev/947262de4794

Dumb bug in my changes to imacro_asm.js.in. This passed our tests, which means we have no trace-tests or regrtests that check the values produced by a for-each loop nested in a (traceable) outer loop. The patch adds a couple trace-tests.

http://hg.mozilla.org/tracemonkey/rev/ece63b96379b caused js1_5/Regress/regress-451322.js to change from a time out (> 480 seconds) to Assertion failed: "Unknown branch type in nPatchBranch": 0 (../nanojit/Nativei386.cpp:442)

Should this get re-opened? Dunno if we want to land it on 191 with a known regression :(

I think Gal's opinion is that this patch was not the cause of the "Unknown branch type" assertion, but rather was just responsible for making the existing problem more visible.

This should go into 1.9.1 with the patch for bug 475916. /be

http://hg.mozilla.org/tracemonkey/rev/9d9412b90552 fixes the rare OOM condition this patch tickled. The issue is completely unrelated. It just changed the alignment of the moon and the stars under memory pressure.

(In reply to comment #45) > http://hg.mozilla.org/releases/mozilla-1.9.1/rev/00d1e8523f73 er, wrong rev. this hasn't landed yet

js1_8_1/trace/trace-test.js v 1.9.1, 1.9.2