Closed
Bug 805052
Opened 11 years ago
Closed 9 years ago
Remove object parents
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla39
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: n.nethercote, Assigned: bzbarsky)
References
Details
(Whiteboard: [js:t])
Attachments
(4 files)
148.06 KB,
patch
|
luke
:
feedback+
|
Details | Diff | Splinter Review |
9.64 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
1.28 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
62.76 KB,
patch
|
Waldo
:
review+
terrence
:
review+
|
Details | Diff | Splinter Review |
Luke claimed yesterday that, if you ignore JS_GetParent() and JS_SetParent(), we don't need to track object parents.
![]() |
Reporter | |
Comment 1•11 years ago
|
||
This patch tests that claim. I just started by removing BaseShape::parent and going from there. There are still quite a few places where we use the parent in a non-trivial way. Search for "njn" in the patch to see all such places -- in all cases I commented out the offending code and inserted a MOZ_ASSERT(0). Some of these can undoubtedly be changed easily; the question is whether they all can. Notable cases: - We abuse the parent field for recording the target of bound functions. A slot could probably be used instead. - The parent is used in multiple places when deciding whether to use the NewObjectCache. - The parent is used to determine if a function is "internal". Perhaps a flag could be used instead? - Is ExpressionDecompiler::findLetVar() going to be removed when the decompiler is removed?
Attachment #674689 -
Flags: feedback?(luke)
Comment 2•11 years ago
|
||
What do you mean by "if you ignore JS_GetParent()" etc.? You are looking at what would it take to remove parents from the JS engine itself, and then separately it could be investigated what it would take to wean API users off of parents? The browser uses parents for a bunch of stuff I don't really understand. See for instance XPCWrappedNative::ReparentWrapperIfFound.
![]() |
||
Comment 3•11 years ago
|
||
Comment on attachment 674689 [details] [diff] [review] patch Looks generally right.
Attachment #674689 -
Flags: feedback?(luke) → feedback+
![]() |
Reporter | |
Comment 4•11 years ago
|
||
> You are looking at > what would it take to remove parents from the JS engine itself, and then > separately it could be investigated what it would take to wean API users off > of parents? Yes. > Looks generally right. Good! But I was hoping for suggestions on getting rid of some of those "njn"-marked cases...
![]() |
Assignee | |
Comment 5•11 years ago
|
||
Isn't the parent chain also used for the scope chain for functions so far? Not just the parent of the function, but the entire parent chain....
![]() |
||
Comment 6•11 years ago
|
||
It used to, but now JSFunctions uses JSFunction::u.i.env_ and the parent is the global object. During ObjShrink, when parent was moved into the BaseShape, the goal was to have as little variance in parent as possible to avoid unnecessary shape duplication.
![]() |
Assignee | |
Comment 7•11 years ago
|
||
I'm not sure I follow. Given this: <body> <div onclick="alert(x); alert(y); alert(z)">Click me<div> <script> var x = 1; document.y = 2; document.querySelector("div").z = 3; </script> </body> Clicking should alert 1, 2, 3 in that order. That works, right now, because the parent of the <div> is the document and the parent of the document is the global, plus whatever makes the function start the lookup at the <div> to start with. So we'd need to replace that with a different setup, right? (Which is certainly desirable, btw; don't get me wrong.)
![]() |
||
Comment 8•11 years ago
|
||
When you call a function, the scope chain is JSFunction::environment(), which can be anything, so the above isn't a problem (assuming that onclick is a function; if it's an evaluated script, we'll just use whatever scope is passed to the JSAPI).
![]() |
Assignee | |
Comment 9•11 years ago
|
||
onclick is a function. But the point is that the <div> needs to be parented to the document, no?
![]() |
Reporter | |
Comment 10•11 years ago
|
||
I looked more closely at this case: inline JSObject * JSObject::enclosingScope() { return isScope() ? &asScope().enclosingScope() : isDebugScope() ? &asDebugScope().enclosingScope() : getParent(); } We hit the getParent() case frequently in tests. In the test I looked at it was due to this code in FunctionBox::FunctionBox: JSObject *scope = outerpc->sc->asGlobal()->scopeChain(); while (scope) { if (scope->isWith()) inWith = true; scope = scope->enclosingScope(); }
![]() |
||
Comment 11•11 years ago
|
||
Ah, yes. Now I remember this problem; I keep realizing and forgetting it; sorry :) So the last time I thought about it, I thought my resolution was: add a JSCLASS flag that says "I have an enclosing scope". This can be used to generalize the tri-case in JSObject::enclosingScope: if you have an enclosing slot, return it, otherwise, return the global". Now, either we'll have to fix a slot that was good enough for everyone or else do like we do with JSCLASS_RESERVED_SLOTS and encode the slot in the JSClass (there probably aren't enough bits in flags, but we could add a new field).
![]() |
Assignee | |
Comment 12•11 years ago
|
||
The other option, if the scope chain is not supposed to mutable, is to just encode it in the function object directly (e.g. have it hold a vector of scopes).
![]() |
Reporter | |
Updated•11 years ago
|
Assignee: n.nethercote → general
![]() |
Reporter | |
Comment 13•11 years ago
|
||
Bug 717637 appears to be relevant -- I think bholley is getting rid of uses of the parent in the DOM.
Updated•10 years ago
|
Assignee: general → nobody
![]() |
Assignee | |
Comment 14•9 years ago
|
||
So I just looked at where we use JS_GetParent and js::GetObjectParent. Remaining JS_GetParent uses outside the JS engine: 1) nsDOMWindowUtils::GetParent -- used in tests only, I assume. 2) ReparentWrapper. This can probably just use the global, since we only use globals for DOM object parents now.... unless we can have XPConnect things going through here? 3) BuildScopeChainForObject in the subscript loader. This would automatically go away if things only had globals as parents. We can try to nix it anyway. 4) JSObject2JSObjectMap::Reparent. 5) SandboxCallableProxyHandler::call -- it uses a custom parent for a proxy to store another object, and 6) Parent() in XPCshell: this is supposed to return the parent. Remaining js::GetObjectParent uses outside the JS engine: 7) An assertion in XPCOMObjectToJsval that only things with JSCLASS_IS_GLOBAL don't have parents. 8) DOMProxyHandler::EnsureExpandoObject which parents the expando object to the proxy itself. I don't think it should. 9) XPConnect's FixUpThisIfBroken. We can probably replace this by functions with extra reservd slots which store the relevant object in those slots. 10) XPCCallContext ctor gets the wrapper as the parent of the tearoff in the tearoff case. We should be able to use a reserved slot there, I'd think. 11) An assert in nsXPConnect::InitClassesWithNewWrappedGlobal that the global we end up creating has no parent. 12) RescueOrphans in XPCWrappedNative, which has a cross-compartment wrapper as parent?? 13) WrapperFactory::PrepareForWrapping in a null-check meant to detect globals. 14) XrayAwareCalleeGlobal uses the parent of the function to get to the Xray the function came from. Maybe we can use a reserved slot here too. 15) CycleCollectedJSRuntime::UsefulToMergeZones asserting the parent is null (so something is a global).
![]() |
Assignee | |
Updated•9 years ago
|
![]() |
Assignee | |
Comment 15•9 years ago
|
||
OK, so now JS_GetParent, JS_SetParent, and js::GetObjectParent are gone. Gecko is no longer using parents for anything. That leaves SpiderMonkey internals. After the patches in bug 1140582, bug 1140573, bug 1140670 we are left with the following JSObject::getParent() consumers: 1) js::SetIntegrityLevel -- just preserving the original parent in the new shape. 2) NewObjectWithTaggedProtoIsCachable -- not cacheable if parent is different. 3) NewObjectWithGroupIsCachable -- not cacheable if parent is different. 4) JSObject::dump() -- just lists it. 5) IsInternalFunctionObject() -- just cares about null or not null. 6) ClonedBlockObject::create -- sets parent to global if it's not global (in which case it asserts that it's null). 7) js::GetObjectEnvironmentObjectForFunction -- returns the getParent() of the function. This is actually used by mozJSComponentLoader::FindTargetObject! 8) JSObject::enclosingScope, which returns getParent() for things that are not ScopeObject or DebugScopeObject. I didn't look into the objectparent bits in shape too much. Most of the above uses are totally happy with parent either always being global or being global-or-null, I think. An exception is enclosingScope(), which requires that we make sure we don't have random non-ScopeObject things on scope chains. Sounds like we may need to fix bug 1097987 first? :(
![]() |
Assignee | |
Comment 16•9 years ago
|
||
Attachment #8577392 -
Flags: review?(jwalden+bmo)
![]() |
Assignee | |
Updated•9 years ago
|
Assignee: nobody → bzbarsky
![]() |
Assignee | |
Comment 17•9 years ago
|
||
Attachment #8577393 -
Flags: review?(jwalden+bmo)
![]() |
Assignee | |
Comment 18•9 years ago
|
||
Terrence, could you look over the tracing/marking bits in here, please?
Attachment #8577396 -
Flags: review?(terrence)
Attachment #8577396 -
Flags: review?(jwalden+bmo)
![]() |
Assignee | |
Comment 19•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=aaa7a9a4e057
Comment 20•9 years ago
|
||
Comment on attachment 8577396 [details] [diff] [review] part 3. Remove parents from SpiderMonkey Review of attachment 8577396 [details] [diff] [review]: ----------------------------------------------------------------- \o/ ::: js/src/gc/Marking.cpp @@ +1120,2 @@ > gcmarker->traverse(global); > } Remove {}'s. ::: js/src/jsarray.cpp @@ +3220,5 @@ > if (!NewObjectMetadata(cx, &metadata)) > return nullptr; > > RootedShape shape(cx, EmptyShape::getInitialShape(cx, &ArrayObject::class_, TaggedProto(proto), > + metadata, gc::FINALIZE_OBJECT0)); This will need s/FINALIZE_OBJECT0/AllocKind::OBJECT0/ when you rebase. ::: js/src/vm/Shape.h @@ +427,5 @@ > + * we have to throw in a bit of padding. > + */ > +#ifndef HAVE_64BIT_BUILD > + uint32_t unusedPadding_; > +#endif I think Brian had some ideas for removing the compartment pointer here, but didn't because it wouldn't save space on 32bit. Probably worth revisiting now. @@ +1241,1 @@ > * the original values. Looks like this needs rewrapped to 80 columns.
Attachment #8577396 -
Flags: review?(terrence) → review+
Updated•9 years ago
|
Attachment #8577392 -
Flags: review?(jwalden+bmo) → review+
Updated•9 years ago
|
Attachment #8577393 -
Flags: review?(jwalden+bmo) → review+
![]() |
||
Comment 21•9 years ago
|
||
(In reply to Terrence Cole [:terrence] from comment #20) > > +#ifndef HAVE_64BIT_BUILD > > + uint32_t unusedPadding_; > > +#endif > > I think Brian had some ideas for removing the compartment pointer here, but > didn't because it wouldn't save space on 32bit. Probably worth revisiting > now. Alternatively, could we remove the metadata pointer and use a WeakMap instead?
![]() |
Assignee | |
Comment 22•9 years ago
|
||
> Remove {}'s. Done. > Looks like this needs rewrapped to 80 columns. Done. I filed bug 1143256 on shrinking down BaseShape more.
Comment 23•9 years ago
|
||
Comment on attachment 8577396 [details] [diff] [review] part 3. Remove parents from SpiderMonkey Review of attachment 8577396 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/gc/Marking.cpp @@ +1068,5 @@ > + gc::MarkBaseShape(trc, &unowned_, "base"); > + > + JSObject* global = compartment()->unsafeUnbarrieredMaybeGlobal(); > + MOZ_ASSERT(global); > + MarkObjectUnbarriered(trc, &global, "global"); So this is consistent and all, I guess, to keep marking the global here. But I sort of wonder. Is it truly necessary to mark it? Is it necessarily BaseShape's duty to keep the global alive? I don't see an intuitive reason why this is the case. Something to ponder, maybe a link we could cut somehow at a future time. ::: js/src/jsobj.cpp @@ +2143,5 @@ > return nullptr; > > RootedPlainObject res(cx, NewObjectWithGroup<PlainObject>(cx, group, kind, > MaybeSingletonObject)); > if (!res) Mind renaming CloneObjectLiteral to CloneArrayOrObjectLiteral? It *does* indeed handle both kinds, as this testcase demonstrates: function func(v) { return v; } var arr = clone(_ => func`contents`)(); assertEq(arr.length, 1); assertEq(arr[0], "contents"); Please add a jit-test somewhere that does this, so that we have *any code at all* exercising the tail of this function. (We don't among jit-tests, jstests, jsapi-tests right now. :-( ) ...or wait. That testcase crashes my shell if I type it in. js> function func(v) { return v; } js> var arr = clone(_ => func`contents`)(); Assertion failure: srcArray->denseElementsAreCopyOnWrite(), at /home/jwalden/moz/slots/js/src/jsobj.cpp:2225 Segmentation fault (core dumped) Stab me with a fork. That's the very first thing in the tail code after converting srcObj to an ArrayObject*. And of course after that point who knows what happens, or how awry things go if these assumptions don't hold. I guess, if this cloning isn't accessible to the web (as long as we don't have a tagged template in self-hosted code, and it appears that the only other path to this cocde with an array is dead because cloneSingletons is only ever set by testing code, see bug 1143268), it's not the end of the world for now. But please file a bug on the above, at least. @@ +2147,5 @@ > if (!res) > return nullptr; > > + // XXXbz Do we still need the reshape here? We got "kind" > + // off the scrObj, no? I don't think we need the reshape, and we can just return |res|. If it passes a try-run, and I see no reason why it shouldn't, do that. Well, *maybe* something about background versus non-background allockinds, but I'm extradordinarily unsure about that, and rather dubious that's the change this code's trying to effect, if indeed it is. Failing that, please spell srcObj right. ;-) @@ +3916,5 @@ > dumpValue(ObjectOrNullValue(proto.toObjectOrNull())); > fputc('\n', stderr); > > + fprintf(stderr, "global "); > + dumpValue(ObjectValue(obj->global())); I mostly doubt this is useful -- I'd remove it from output rather than make it global instead of parent. ::: js/src/vm/Shape.h @@ +309,1 @@ > * be set for the object. Except for the class, this information is mutable and This probably wants 79ch rewrapping now.
Attachment #8577396 -
Flags: review?(jwalden+bmo) → review+
![]() |
Assignee | |
Comment 24•9 years ago
|
||
> Is it necessarily BaseShape's duty to keep the global alive? I think the idea is that an object should keep its global alive. Doing that via the shape is just expedient: object traces shape, shape traces global. > But please file a bug on the above, at least. Bug 1143269. > If it passes a try-run, and I see no reason why it shouldn't, do that. Filed followup bug 1143270 and fixed the spelling. > I'd remove it from output OK, done. > This probably wants 79ch rewrapping now. Done.
Blocks: 1143270
![]() |
Assignee | |
Comment 25•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/94c3742d22df https://hg.mozilla.org/integration/mozilla-inbound/rev/92d88eefea89 https://hg.mozilla.org/integration/mozilla-inbound/rev/c0a7bfc8dfae
Updated•9 years ago
|
Keywords: dev-doc-needed
Comment 26•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/94c3742d22df https://hg.mozilla.org/mozilla-central/rev/92d88eefea89 https://hg.mozilla.org/mozilla-central/rev/c0a7bfc8dfae
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
![]() |
||
Comment 27•9 years ago
|
||
\o/ \o/ \o/
Comment 28•9 years ago
|
||
Awesome - Congratulations on pushing this through Boris!
Comment 29•9 years ago
|
||
bz: I just want to tell you good luck, we're all counting on you.
Updated•3 years ago
|
Keywords: dev-doc-needed
You need to log in
before you can comment on or make changes to this bug.
Description
•