Closed Bug 805052 Opened 12 years ago Closed 10 years ago

Remove object parents

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla39
Tracking Flags:
Tracking Status
firefox39 --- fixed

People

(Reporter: n.nethercote, Assigned: bzbarsky)

References

Details

(Whiteboard: [js:t])

Attachments

(4 files)

patch
(deleted), patch
luke
: feedback+
Details | Diff | Splinter Review
part 1. Remove assertParentIs
(deleted), patch
Waldo
: review+
Details | Diff | Splinter Review
part 2. Stop using the shape's parent to get the object's global
(deleted), patch
Waldo
: review+
Details | Diff | Splinter Review
part 3. Remove parents from SpiderMonkey
(deleted), 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.
Attached patch patch (deleted) — — Splinter Review
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)
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 on attachment 674689 [details] [diff] [review] patch Looks generally right.
Attachment #674689 - Flags: feedback?(luke) → feedback+
> 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...
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....
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.
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.)
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).
onclick is a function. But the point is that the <div> needs to be parented to the document, no?
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(); }
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).
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).
Assignee: n.nethercote → general
Bug 717637 appears to be relevant -- I think bholley is getting rid of uses of the parent in the DOM.
Assignee: general → nobody
Depends on: 1127475
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).
Depends on: 1131791
Depends on: 1131796
Depends on: 1131797
Depends on: 1131799
Depends on: 1131801
Depends on: 1131802
Depends on: 1131803
Depends on: 1131805
Depends on: 1136345
Depends on: 1136516
Depends on: 1136520
Depends on: 599438, 611190
Depends on: 1136523
Depends on: 1136906
Depends on: 1136925
Depends on: 1136980
Depends on: 1137325
Depends on: 1137334
Depends on: 1137578
Depends on: 1140573
Depends on: 1140582
Depends on: 1140586
Depends on: 1140670
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? :(
Depends on: 1141905
Depends on: 1142241
Depends on: 1142266
Depends on: 1142282
Depends on: 1142296
Depends on: 1142304
Depends on: 1142309
Depends on: 1142310
Depends on: 1142311
Depends on: 1142832
Depends on: 1142859
Depends on: 1142864
Depends on: 1142865
No longer depends on: 1142832
Attached patch part 1. Remove assertParentIs (deleted) — — Splinter Review
Attachment #8577392 - Flags: review?(jwalden+bmo)
Assignee: nobody → bzbarsky
Terrence, could you look over the tracing/marking bits in here, please?
Attachment #8577396 - Flags: review?(terrence)
Attachment #8577396 - Flags: review?(jwalden+bmo)
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+
Attachment #8577392 - Flags: review?(jwalden+bmo) → review+
Attachment #8577393 - Flags: review?(jwalden+bmo) → review+
(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?
> Remove {}'s. Done. > Looks like this needs rewrapped to 80 columns. Done. I filed bug 1143256 on shrinking down BaseShape more.
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+
> 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
\o/ \o/ \o/
Awesome - Congratulations on pushing this through Boris!
bz: I just want to tell you good luck, we're all counting on you.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: