Last Comment Bug 362909 - pushobj is not GC-atomic when debugger is installed.
: pushobj is not GC-atomic when debugger is installed.
Status: RESOLVED FIXED
[sg:moderate]
: fixed1.8.0.10, fixed1.8.1.2
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: mozilla1.9alpha1
Assigned To: Igor Bukanov
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-12-05 20:30 PST by Igor Bukanov
Modified: 2007-02-23 21:52 PST (History)
2 users (show)
bob: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch to record bytecode successor counts (4.56 KB, patch)
2006-12-11 19:18 PST, Brendan Eich [:brendan]
no flags Details | Diff | Splinter Review
bytecode graph for perfect.js (147.00 KB, image/png)
2006-12-11 19:19 PST, Brendan Eich [:brendan]
no flags Details
filter edges < 1% of grand total bytecode count (4.58 KB, patch)
2006-12-11 19:30 PST, Brendan Eich [:brendan]
no flags Details | Diff | Splinter Review
filtered gmail load and first message view bytecode graph (86.38 KB, image/png)
2006-12-11 19:30 PST, Brendan Eich [:brendan]
no flags Details
Minimal fix v1 (1.62 KB, patch)
2006-12-15 09:58 PST, Igor Bukanov
brendan: review+
dveditz: approval1.8.1.2+
Details | Diff | Splinter Review
1.8.0 version of Minimal fix v1 (887 bytes, patch)
2006-12-18 15:58 PST, Igor Bukanov
brendan: review+
jaymoz: approval1.8.0.10+
Details | Diff | Splinter Review

Description Igor Bukanov 2006-12-05 20:30:29 PST
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.
Comment 1 Brian Crowder 2006-12-08 23:34:07 PST
Should registers always be considered as GC roots?
Comment 2 Brendan Eich [:brendan] 2006-12-09 10:47:56 PST
(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
Comment 3 Igor Bukanov 2006-12-09 11:09:17 PST
(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.
Comment 4 Brendan Eich [:brendan] 2006-12-09 22:18:03 PST
(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
Comment 5 Igor Bukanov 2006-12-10 07:30:36 PST
(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.
Comment 6 Brendan Eich [:brendan] 2006-12-11 19:18:05 PST
Created attachment 248339 [details] [diff] [review]
patch to record bytecode successor counts

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

/be
Comment 7 Brendan Eich [:brendan] 2006-12-11 19:19:01 PST
Created attachment 248340 [details]
bytecode graph for perfect.js

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
Comment 8 Brendan Eich [:brendan] 2006-12-11 19:30:02 PST
Created attachment 248343 [details] [diff] [review]
filter edges < 1% of grand total bytecode count
Comment 9 Brendan Eich [:brendan] 2006-12-11 19:30:40 PST
Created attachment 248344 [details]
filtered gmail load and first message view bytecode graph
Comment 10 Brendan Eich [:brendan] 2006-12-11 19:37:38 PST
* 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
Comment 11 Brendan Eich [:brendan] 2006-12-11 20:45:14 PST
Tracked by cover bug 363530, which blocks bytecode-optimation meta bug 363529.

/be
Comment 12 Igor Bukanov 2006-12-15 09:58:20 PST
Created attachment 248752 [details] [diff] [review]
Minimal fix v1

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 13 Brendan Eich [:brendan] 2006-12-15 11:19:58 PST
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
Comment 14 Igor Bukanov 2006-12-15 11:27:21 PST
(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?
Comment 15 Brendan Eich [:brendan] 2006-12-15 12:23:17 PST
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
Comment 16 Igor Bukanov 2006-12-15 15:58:16 PST
(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

?
 
Comment 17 Igor Bukanov 2006-12-15 16:00:40 PST
Ignore the prev comment. I forgot about the suggestion to rename [call] to [set_call]. Then

PROPCALL
NAMECALL
ELEMCALL

are nice and simple.
Comment 18 Brendan Eich [:brendan] 2006-12-15 16:12:11 PST
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
Comment 19 Igor Bukanov 2006-12-15 16:54:55 PST
(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.

?

Comment 20 Brendan Eich [:brendan] 2006-12-15 18:00:14 PST
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
Comment 21 Igor Bukanov 2006-12-16 18:00:13 PST
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 22 Daniel Veditz [:dveditz] 2006-12-18 15:17:08 PST
Comment on attachment 248752 [details] [diff] [review]
Minimal fix v1

approved for 1.8/1.8.0 branches, a=dveditz for drivers
Comment 23 Igor Bukanov 2006-12-18 15:49:09 PST
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
 
Comment 24 Igor Bukanov 2006-12-18 15:58:54 PST
Created attachment 249054 [details] [diff] [review]
1.8.0 version of Minimal fix v1 

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 25 Brendan Eich [:brendan] 2006-12-18 16:27:00 PST
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 26 Igor Bukanov 2006-12-19 15:26:20 PST
Comment on attachment 248752 [details] [diff] [review]
Minimal fix v1

For 1.8.0 see the next attachment.
Comment 27 Jay Patel [:jay] 2006-12-20 15:21:44 PST
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.
Comment 28 Igor Bukanov 2006-12-20 17:29:41 PST
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

Note You need to log in before you can comment on or make changes to this bug.