694247 - Move scope chains of scope objects to reserved slots

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

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.

Comment 1

[Brian Hackett [Laid off!]]

Attachment: patch

https://hg.mozilla.org/projects/jaegermonkey/rev/7b634ad714fd

Comment 2

[Luke Wagner [:luke]]

Comment on attachment 566738 [details] [diff] [review]
patch

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?

Comment 3

[Andrew McCreight [:mccr8]]

I think this landed as part of ObjShrink, quite some time ago.