Closed
Bug 684336
Opened 8 years ago
Closed 8 years ago
Abstract Call objects into vm/CallObject.h
Categories
(Core :: JavaScript Engine, defect)
Not set
Tracking
()
RESOLVED
FIXED
mozilla9
People
(Reporter: paul.biggar, Assigned: paul.biggar)
References
Details
Attachments
(1 file, 1 obsolete file)
78.53 KB,
patch
|
paul.biggar
:
review+
|
Details | Diff | 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)
![]() |
||
Comment 1•8 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•8 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?
Comment 3•8 years ago
|
||
(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•8 years ago
|
||
Suppose it is. Have we given up on rooting manually then?
Comment 5•8 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•8 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•8 years ago
|
||
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•8 years ago
|
Attachment #559001 -
Flags: review+
Comment 8•8 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/9e238421b8f6
Status: NEW → RESOLVED
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.
Description
•