Last Comment Bug 684336 - Abstract Call objects into vm/CallObject.h
: Abstract Call objects into vm/CallObject.h
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Mac OS X
-- normal (vote)
: mozilla9
Assigned To: Paul Biggar
: Jason Orendorff [:jorendorff]
Depends on:
Blocks: 478174
  Show dependency treegraph
Reported: 2011-09-02 13:41 PDT by Paul Biggar
Modified: 2011-09-08 16:17 PDT (History)
5 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Abstract CallObject.h (63.21 KB, patch)
2011-09-02 13:41 PDT, Paul Biggar
luke: review+
Details | Diff | Splinter Review
Rebased (78.53 KB, patch)
2011-09-07 16:49 PDT, Paul Biggar
paul.biggar: review+
Details | Diff | Splinter Review

Description User image Paul Biggar 2011-09-02 13:41:04 PDT
Created attachment 557931 [details] [diff] [review]
Abstract CallObject.h

A simple extraction, following waldo's example of ArgumentsObject. This simplifies bug 478174.

callobj->getPrivate() is the same as argsobj->getPrivate(), so I copied maybeStackFrame, etc, to ArgumentsObject.
Comment 1 User image Luke Wagner [:luke] 2011-09-02 18:22:11 PDT
This is great!  I'm sorry I didn't get to this as fast as you got to mine; I'll finish on Monday.
Comment 2 User image Paul Biggar 2011-09-02 18:52:43 PDT
It occurs to me that obj isn't rooted in CallObject::create. It wasn't before, and isn't in other ::create functions. Is this OK, if so why?
Comment 3 User image Brendan Eich [:brendan] 2011-09-02 19:01:02 PDT
(In reply to Paul Biggar from comment #2)
> It occurs to me that obj isn't rooted in CallObject::create. It wasn't
> before, and isn't in other ::create functions. Is this OK, if so why?

Conservative stack scanner still around, right?

Comment 4 User image Paul Biggar 2011-09-02 19:11:22 PDT
Suppose it is. Have we given up on rooting manually then?
Comment 5 User image Andreas Gal :gal 2011-09-02 20:49:59 PDT
We have not been relying on explicit rooting for more than a year, and we have removed explicitly rooting from most hot paths inside the engine and also in DOM and XPConnect.

There has been a proposal to add back explicit rooting for generational GC (keeping the stack scanner requires doing mostly-copying GC only). If we do so, we would use static analysis to infer rooting (bhackett did some work on that).
Comment 6 User image Luke Wagner [:luke] 2011-09-06 09:49:07 PDT
Comment on attachment 557931 [details] [diff] [review]
Abstract CallObject.h

Review of attachment 557931 [details] [diff] [review]:

Great patch.  I suppose we can declare "mission accomplished" when we can remove JSObject::getPrivate and only JS_GetPrivate breaks :)

::: js/src/jsfun.cpp
@@ +283,5 @@
>       */
>      if (argsobj->isStrictArguments())
>          fp->forEachCanonicalActualArg(PutArg(argsobj->data()->slots));
>      else
> +        argsobj->setStackFrame(fp);


@@ +768,5 @@
>   * Otherwise it must be a call object for eval of strict mode code, and callee
>   * must be null.
>   */
> +CallObject *
> +CallObject::create(JSContext *cx, JSScript *script, JSObject &scopeChain, JSObject *callee)

Can you move this to vm/CallObject.cpp?  I know Waldo didn't move some of the ArgumentsObject stuff but I think that was a mistake.

::: js/src/jsobj.h
@@ +1025,2 @@
>    public:
> +    inline js::CallObject *asCall();

I know the precedent in JSObject is asX() returning an X*, but do you think, in the style of Value::toObject, StackFrame::scopeChain, etc, we can indicate non-optionality by having asX() return an X&?  (Also "." vs. "->" saves a char at the call-site ;-)  There are still only a few asX's so far, so perhaps it isn't too much work?  If it is, feel free to ignore.

@@ +1564,5 @@
>  inline bool JSObject::isObject() const { return getClass() == &js_ObjectClass; }
>  inline bool JSObject::isWith() const   { return getClass() == &js_WithClass; }
>  inline bool JSObject::isBlock() const  { return getClass() == &js_BlockClass; }
> +inline bool JSObject::isDeclEnv() const  { return getClass() == &js_DeclEnvClass; }
> +inline bool JSObject::isCall() const { return getClass() == &js_CallClass; }

Sorry for the rebase pain.

::: js/src/methodjit/PolyIC.cpp
@@ +52,5 @@
>  #include "jspropertycache.h"
>  #include "jspropertycacheinlines.h"
>  #include "jsinterpinlines.h"
>  #include "jsautooplen.h"
> +#include "vm/CallObject-inl.h"

We usually have a newline between the .h's and the -inl.h's.
Comment 7 User image Paul Biggar 2011-09-07 16:49:04 PDT
Created attachment 559001 [details] [diff] [review]

rebased on top of type inference changes. I'm going to carry luke's r+ forward since the changes are all mechanical, but I'll give it a tryserver and another look to make sure.
Comment 8 User image Matt Brubeck (:mbrubeck) 2011-09-08 16:17:37 PDT

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