Closed Bug 694247 Opened 9 years ago Closed 7 years ago

Move scope chains of scope objects to reserved slots


(Core :: JavaScript Engine, defect)

Not set





(Reporter: bhackett1024, Assigned: bhackett1024)




(1 file)

Currently the parent of scope objects (call, block, declenv, with) stores the scope chain links going up to the global.  If parent is moved from JSObject to BaseShape, this will lead to huge fragmentation of Shapes and BaseShapes --- different shape hierarchies would be needed for scope objects entrained on different scope chain (rather than different shape hierarchies for each global, if parent points to the global).  This can be fixed by storing the scope chain of a scope object in a consistent fixed slot (the first slot), and storing the global in the parent itself.
Attached patch patchSplinter Review
Assignee: general → bhackett1024
Attachment #566738 - Flags: review?(luke)
Depends on: 700792
Comment on attachment 566738 [details] [diff] [review]

Review of attachment 566738 [details] [diff] [review]:

From IRC: let's remove what is currently JSObject::scopeChain and rename JSObject::getParentOrScopeChain to JSObject::scopeChain and write a big beefy comment explaining the non-lexical-global-scope-object situation.

::: js/src/jsfriendapi.h
@@ +248,5 @@
> +JS_FRIEND_API(JSObject *)
> +GetObjectParentMaybeScope(const JSObject *obj);
> +
> +JS_FRIEND_API(JSObject *)
> +GetGlobalForObject(JSObject *obj);

There is already JS_GetGlobalForObject, so don't need to add this.

::: js/src/jsobj.h
@@ +879,5 @@
> +     * Information for non-global scope chain objects (call/with/etc.). All
> +     * objects on a scope chain are either isScope() or isGlobal(). isScope()
> +     * objects do not escape to script and only appear on scope chains.
> +     */
> +    inline bool isScope() const;

If isScope() is still useful, can it be renamed isNativeScope() ?

::: js/src/jsobjinlines.h
@@ +370,5 @@
> +JSObject::setScopeChain(JSObject *obj)
> +{
> +    JS_ASSERT(isScope());
> +    setFixedSlot(0, JS::ObjectValue(*obj));
> +}

With scopeChain() repurposed, perhaps call this setEnclosingScope?

::: js/src/vm/CallObject.h
@@ +48,5 @@
>      /*
>       * Reserved slot structure for Call objects:
>       *
> +     * SCOPE_CHAIN_SLOT - The enclosing scope. This must come first, for
> +     *                    JSObject::scopeParent.

Instead of a comment, could you put a set of static asserts (one for each internal scope type) somewhere?
Attachment #566738 - Flags: review?(luke) → review+
I think this landed as part of ObjShrink, quite some time ago.
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.