Move scope chains of scope objects to reserved slots

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: bhackett, Assigned: bhackett)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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.
Posted patch patchSplinter Review
https://hg.mozilla.org/projects/jaegermonkey/rev/7b634ad714fd
Assignee: general → bhackett1024
Attachment #566738 - Flags: review?(luke)
Depends on: 700792
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?
Attachment #566738 - Flags: review?(luke) → review+
I think this landed as part of ObjShrink, quite some time ago.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.