Closed Bug 684336 Opened 8 years ago Closed 8 years ago

Abstract Call objects into vm/CallObject.h


(Core :: JavaScript Engine, defect)

Not set





(Reporter: paul.biggar, Assigned: paul.biggar)




(1 file, 1 obsolete file)

Attached patch Abstract CallObject.h (obsolete) — Splinter Review
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.
Attachment #557931 - Flags: review?(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.
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?
(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?

Suppose it is. Have we given up on rooting manually then?
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 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.
Attachment #557931 - Flags: review?(luke) → review+
Attached patch RebasedSplinter 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.
Assignee: general → pbiggar
Attachment #557931 - Attachment is obsolete: true
Attachment #559001 - Flags: review+
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
You need to log in before you can comment on or make changes to this bug.