Currently SpiderMonkey implements function call operator using a helper pushobj bytecode that pushes the obj register to the stack. The register is populated during the previous JSOP_GETPRROP, JS_NAME etc. operation. The problem is that the register does not belong to a root set so with the debugger installed a call like "1()" can be a GC hazard. This will happen when the debugger is called between "name" and "pushobj" bytecode and when the debugger calls JS_GC or JS_MaybeGC() at that point.

Should registers always be considered as GC roots?

(In reply to comment #1) > Should registers always be considered as GC roots? No, that's conservative GC (indeed we can't know whether obj is in a register or homed to a stack word, so conservative scanning of thread stacks would be needed, not just of thread register sets). The fix is to compress bytecode so that JSOP_GETMETHOD or newer forms always push two jsvals: callee, thisp. /be

(In reply to comment #2) > The fix is to compress bytecode so that JSOP_GETMETHOD or newer forms always > push two jsvals: callee, thisp. Or alternatively to avoid proliferation of bytecodes [call] itself can be turned into a prefix opcode that sets a flag. Then [name] etc. would need to check for it and jump to to a common call label when they are done. This would removes the need for [pushobj]. Yet another alternative is to have [call_name], [call_local], [call_prop] etc. (which is similar to the current [foreach_name])] that calculates this and function and again jump to the common call code.

(In reply to comment #3) > Yet another alternative is to have [call_name], [call_local], [call_prop] etc. > (which is similar to the current [foreach_name])] that calculates this and > function and again jump to the common call code. Have to take care not to violate guaranteed order of evaluation, which is left to right, callee before arguments. /be

(In reply to comment #4) > Have to take care not to violate guaranteed order of evaluation, which is left > to right, callee before arguments. Right, [call_prop] does not work as arguments can mutate the value of the property. Then a propere fix is either indeed to add [name_and_this], [var_and_this] etc. that push to the stack both this and function (which is how Rhino implements it), or turn [pushobj] into a prefix bytecode and make all [name], [var] check/reset a global flag set by [pushobj]. On the other hand a quick and dirty fix is not to call the debugger for pushobj. This is good for branches.

This produces a dot file, usable by graphviz, dotty, etc. Sample output for perfect.js next. /be

Lots of room for optimization here. A browser graph for startup and load of my gmail folder is coming next (takes a while to render). /be

* Assignment (set*) bytecodes should pop, which means assignment expressions whose results are used (e.g. for other assignments, or in actual arguments) must dup their RHS before assigning and popping. This is bug 312354 in disguise. * Lots of unqualified names being called: getmethod and name => pushobj. Covered by this bug. * lt, etc. => ifeq, a bug to be filed shortly. * this => getprop, TBF. /be

Tracked by cover bug 363530, which blocks bytecode-optimation meta bug 363529. /be

The patch makes sure that interruptHandler is never called for pushobj. In the threaded case for this the patch records L_JSOP_PUSHOBJ in interruptJumpTable so the handler never gets the control for the opcode. In the switch case the code checks directly for pushobj when calling interruptHandler. This is a minimal fix to make easy branch patching.

Comment on attachment 248752 [details] [diff] [review] Minimal fix v1 Thanks, good for the branches. I'm planning to hack on bytecode soon, did you want to hack the long-term patch for this bug? /be

(In reply to comment #13) > (From update of attachment 248752 [details] [diff] [review] [edit]) > Thanks, good for the branches. > > I'm planning to hack on bytecode soon, did you want to hack the long-term patch > for this bug? Yep, I can try to introduce the extra bytecodes like [name_and_this], [prop_and_this] etc. But the names are what I put 3 years ago to Rhino and, I guess, are too long for bytecode nicks. Any better idea?

We could build on JSOP_GETMETHOD. NAME : GETNAMEDMETHOD GETPROP : GETMETHOD GETELEM : GETINDEXEDMETHOD CALL : SETCALL I think this works, but it's not parallel, and the MM in GETELEMMETHOD is ugly. How about renaming GETMETHOD? NAME : NAMECALL GETPROP : PROPCALL GETELEM : ELEMCALL Confusion: these are not call ops, but "eval and push [callee, this]" ops. But it's not really that confusing, IMHO. If you want, you could add "EE" at the end for "CALLEE". /be

(In reply to comment #15) > We could build on JSOP_GETMETHOD. Currently JSOP_GETMETHOD is used to implement function::x not followed by the method call: function f(foo) { return foo.function::bar; } dis(f); 00000: getarg 0 00003: getmethod "bar" 00006: return 00007: stop With the new semantics [propcall] would push both function and this object so function:: would have to be implemented via [propcall]/[pop] pair. Then "call" in [propcall] is confusing. So indeed [propcallee] would be better. But what about then using "this" as a suffix, like in: PROPTHIS NAMETHIS ELEMTHIS Or perhaps "obj": PROPOBJ NAMEOBJ ELEMOBJ ?

Ignore the prev comment. I forgot about the suggestion to rename [call] to [set_call]. Then PROPCALL NAMECALL ELEMCALL are nice and simple.

Sorry, I didn't mean to propose renaming JSOP_CALL to JSOP_SETCALL -- the latter already exists for o.item(i) = j (MS JScript compatibility, native o.item only). NAMETHIS PROPTHIS ELEMTHIS are better, if you ask me -- why did you prefer CALL in the name? /be

(In reply to comment #18) > > NAMETHIS PROPTHIS ELEMTHIS are better, if you ask me -- why did you prefer CALL > in the name? Because it reminds about a call context. NAMETHIS also reminds about it, but not that strongly. So as the last attempt and following FORNAME, FORPROP etc. tradition, what about: FCNAME FCPROP FCELEM FCLOCAL FCARG FCVAR where FC stands for Function call Context and means to push both function and this. ?

FC -- too concise, crypic without more precedent; context not enough to decrypt. I'd go back to CALL{NAME,PROP,ELEM} if you want CALL more than THIS. /be

I committed the patch from comment 12 to the trunk: Checking in jsinterp.c; /cvsroot/mozilla/js/src/jsinterp.c,v <-- jsinterp.c new revision: 3.309; previous revision: 3.308 done

Comment on attachment 248752 [details] [diff] [review] Minimal fix v1 approved for 1.8/1.8.0 branches, a=dveditz for drivers

I committed the patch from comment 12 to MOZILLA_1_8_BRANCH: Checking in jsinterp.c; /cvsroot/mozilla/js/src/jsinterp.c,v <-- jsinterp.c new revision: 3.181.2.77; previous revision: 3.181.2.76 done

1.8.0 branch does not use indirect threading. That required not-so-trivial efforts to backport the patch there. hence I ask for another approval/review.

Comment on attachment 249054 [details] [diff] [review] 1.8.0 version of Minimal fix v1 Sure, only hurts perf when interruptHandler is set. /be

Comment on attachment 248752 [details] [diff] [review] Minimal fix v1 For 1.8.0 see the next attachment.

Comment on attachment 249054 [details] [diff] [review] 1.8.0 version of Minimal fix v1 Approved for the 1.8.0 branch, a=jay for drivers.

I committed the patch from comment 24 to MOZILLA_1_8_0_BRANCH: Checking in jsinterp.c; /cvsroot/mozilla/js/src/jsinterp.c,v <-- jsinterp.c new revision: 3.181.2.17.2.24; previous revision: 3.181.2.17.2.23 done