684336 - Abstract Call objects into vm/CallObject.h

Status: RESOLVED FIXED

Description

[Paul Biggar]

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

[Luke Wagner [:luke]]

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

[Paul Biggar]

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

[Brendan Eich [:brendan]]

(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?

/be

Comment 4

[Paul Biggar]

Suppose it is. Have we given up on rooting manually then?

Comment 5

[Andreas Gal :gal]

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

[Luke Wagner [:luke]]

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);

Nice.

@@ +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

[Paul Biggar]

Created attachment 559001 [details] [diff] [review]
Rebased

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

[Matt Brubeck (:mbrubeck)]

http://hg.mozilla.org/mozilla-central/rev/9e238421b8f6