Comment 1 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 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 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?

/be

Comment 4 Paul Biggar 2011-09-02 19:11:22 PDT

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

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

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 2011-09-07 16:49:04 PDT

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) 2011-09-08 16:17:37 PDT

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