Abstract Call objects into vm/CallObject.h

RESOLVED FIXED in mozilla9

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Paul Biggar, Assigned: Paul Biggar)

Tracking

unspecified
mozilla9
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

78.53 KB, patch
Paul Biggar
: review+
Details | Diff | Splinter Review
(Assignee)

Description

6 years ago
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.
Attachment #557931 - Flags: review?(luke)

Comment 1

6 years ago
This is great!  I'm sorry I didn't get to this as fast as you got to mine; I'll finish on Monday.
(Assignee)

Comment 2

6 years ago
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?

/be
(Assignee)

Comment 4

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

Comment 5

6 years ago
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

6 years ago
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.
Attachment #557931 - Flags: review?(luke) → review+
(Assignee)

Comment 7

6 years ago
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.
Assignee: general → pbiggar
Attachment #557931 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #559001 - Flags: review+
http://hg.mozilla.org/mozilla-central/rev/9e238421b8f6
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
You need to log in before you can comment on or make changes to this bug.