713311 - give the remaining scope objects typed interfaces

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

Attachment: do it

We have js::CallObject but all the other scope objects muck directly with JSObject (or use gross-looking macros like OBJ_BLOCK_DEPTH). This patch pulls out a full hierarchy of scope objects:

  JSObject
    ScopeObject
      CallObject
      DeclEnvObject
      NestedScopeObject
        BlockObject
          CloneBlockObject
          StaticBlockObject

Where each level adds at least a field or reserved slot.  The patch renames vm/CallObject.h to vm/ScopeObject.h so that they can all be in the same file and have a nice ASCII-art hierarchy and explanation like the one in vm/String.h.

The patch also regularizes some things:
 - JSObject::asX() now always returns a reference
 - JSObject::getGlobal is rename to JSObject::global (since its infallible) and returns a ref
 - JSObject::scopeChain is renamed to JSObject::enclosingScope (scopeChain has been bugging me)

Comment 1

[Luke Wagner [:luke]]

Attachment: do it (v.2)

The interfaces were not as "tight" as they could have been regarding access to the slot values of nested scope objects.  Also, I removed a few ill-fated changes that I missed in the original patch.

Comment 2

[Jeff Walden [:Waldo]]

Comment on attachment 584145 [details] [diff] [review]
do it

Review of attachment 584145 [details] [diff] [review]:
-----------------------------------------------------------------

I was most of the way through this when I saw the cancel and ? on the newer patch.  Here's what I have for this version.  What's different in the new one that I should skim?

I see some inconsistency in changing locals to reference type from pointer type.  Chromium people do some sort of clang-y rewriting to do widespread changing, sometime a few of us should learn how to do similar things and go to town on some of this.  Maybe I should do that....  But this is fine enough as-is if it'd be a matter of manual changing.

::: js/src/jsfun.cpp
@@ +739,5 @@
>          /* Clear private pointers to fp, which is about to go away. */
>          if (js_IsNamedLambda(fun)) {
> +            JSObject &env = callobj.enclosingScope();
> +            JS_ASSERT(env.isDeclEnv());
> +            JS_ASSERT(env.getPrivate() == fp);

|env.asDeclEnv().maybeStackFrame() == fp|?

::: js/src/jsinterp.cpp
@@ +206,5 @@
>      /*
>       * Special-case cloning the innermost block; this doesn't have enough in
>       * common with subsequent steps to include in the loop.
>       *
>       * js_CloneBlockObject leaves the clone's parent slot uninitialized. We

This comment needs updating for the name change.

@@ +214,3 @@
>      if (!innermostNewChild)
>          return NULL;
>      AutoObjectRooter tvr(cx, innermostNewChild);

You might as well remove this tvr while you're here, right?

@@ +1123,2 @@
>  {
>      JS_ASSERT(stackDepth >= 0);

This will trigger compiler warnings.

@@ +1141,2 @@
>              /* Don't fail until after we've updated all stacks. */
> +            normalUnwind &= scopeChain.asClonedBlock().put(cx, normalUnwind);

Could we rename this "put" operation at some point (not here, of course), maybe something like storeBlockVariablesToStack?  Dunno, but "put" is not a good name.

::: js/src/jsobj.h
@@ +326,5 @@
>  extern Class ErrorClass;
>  extern Class GeneratorClass;
>  extern Class IteratorClass;
>  extern Class JSONClass;
> +extern Class BlockClass;

Erm, why this move?  Alphabetical order seemed cool to me...

@@ +905,2 @@
>  
> +    inline js::GlobalObject &global() const;

I am a fan of this new, shorter name, and of less ->.

@@ +1330,5 @@
> +     *     XObject &x = obj.asX();
> +     *     x.foo();
> +     *   }
> +     *
> +     * Note that these XObject classes form a hierarchy. For example, a cloned

You could/probably should omit the unnecessary "Note that" at the start of this.

@@ +1331,5 @@
> +     *     x.foo();
> +     *   }
> +     *
> +     * Note that these XObject classes form a hierarchy. For example, a cloned
> +     * block object isClonedBlock, isBlock, isNestedScope and isScope. Each of

You accidentally the verb?

@@ +1332,5 @@
> +     *   }
> +     *
> +     * Note that these XObject classes form a hierarchy. For example, a cloned
> +     * block object isClonedBlock, isBlock, isNestedScope and isScope. Each of
> +     * these has a respective class that derives and adds operations.

Purty.  As a followup, could you file a bug to delete all these isX and asX methods from their corresponding classes, the same way JSLinearString::isLinear and JSLinearString::asLinear are deleted?  The error those prevent seems a bit less likely to happen with object subclasses, but -- particularly after this much type-changing -- it seems quite possible.

@@ +1339,5 @@
> +     * triplet (along with any class YObject that derives XObject).
> +     *
> +     * Note that X represents a low-level representation and not, for example,
> +     * query the [[Class]] property of object defined by the spec (for this,
> +     * see js::ObjectClassIs).

This paragraph is garbled.

@@ +1341,5 @@
> +     * Note that X represents a low-level representation and not, for example,
> +     * query the [[Class]] property of object defined by the spec (for this,
> +     * see js::ObjectClassIs).
> +     *
> +     * SpiderMonkey has not been completely transition to the isX/asX/XObject

"transitioned", or better, "switched".  Verbing weirds language, as they say.

::: js/src/jsobjinlines.h
@@ +74,1 @@
>  #include "jsscopeinlines.h"

opqr?

@@ +291,2 @@
>  {
> +    return isScope() ? &asScope().enclosingScope() : getParent();

Purty.

@@ +890,5 @@
>  {
>      return !!getClass()->ext.equality;
>  }
>  
>  inline bool JSObject::isArguments() const { return isNormalArguments() || isStrictArguments(); }

...I'm pretty surprised this doesn't trigger is{Normal,Strict}Arguments-not-defined warnings.  I'd kind of prefer, for sanity, if those were defined before this use of them is defined.  I guess that disrupts having one huge alphabetical list.  Not sure what to do about this.  :-\

::: js/src/jsopcode.cpp
@@ -227,5 @@
>      JS_STATIC_ASSERT(JSOP_ENTERBLOCK_LENGTH == JSOP_ENTERLET1_LENGTH);
>  
>      JSObject *obj = NULL;
>      GET_OBJECT_FROM_BYTECODE(script, pc, 0, obj);
> -    return OBJ_BLOCK_COUNT(NULL, obj);

Guh, that was awful.

::: js/src/vm/CallObject-inl.h
@@ +62,5 @@
> +
> +inline StackFrame *
> +ScopeObject::maybeStackFrame() const
> +{
> +    return reinterpret_cast<StackFrame *>(getPrivate());

Assert that this isn't a StaticBlockObject.

@@ +68,5 @@
> +
> +inline void
> +ScopeObject::setStackFrame(StackFrame *frame)
> +{
> +    return setPrivate(frame);

Assert that this isn't a StaticBlockObject.

@@ +202,5 @@
>  
> +inline uint32_t
> +NestedScopeObject::stackDepth() const
> +{
> +    return uint32_t(getFixedSlot(DEPTH_SLOT).toInt32());

Perhaps this slot should be changed to a PrivateUint32 slot at some point.

@@ +214,5 @@
> +
> +inline JSObject &
> +WithObject::object() const
> +{
> +    return *getProto();

Perhaps we should start deleting the methods that don't make sense from object subclasses.  It's certainly not meaningful to talk about a WithObject's prototype, or a ClonedBlockObject's prototype, or about (in generic, void* terms) the private of a ScopeObject.  I think this would help with understandability, and it would prevent unintentional uses that subvert our careful refinement type hierarchies.

@@ +281,5 @@
> +inline js::NestedScopeObject &
> +JSObject::asNestedScope()
> +{
> +    JS_ASSERT(isWith() || isBlock());
> +    return *static_cast<js::BlockObject *>(this);

This is a little weird, casting to a subclass of NestedScopeObject.

::: js/src/vm/CallObject.cpp
@@ +120,3 @@
>  }
>  
> +Class js::DeclEnvClass = {

I've done DeclEvnObject::jsClass before -- maybe worth doing here, and for the other such cases.

@@ +120,5 @@
>  }
>  
> +Class js::DeclEnvClass = {
> +    js_Object_str,
> +    JSCLASS_HAS_PRIVATE |

Private is apparently slower than reserved slot nowadays.  Followup to switch to a reserved slot perhaps?

@@ +565,5 @@
> +     * Don't convert this object to dictionary mode so that we can clone the
> +     * block's shape later.
> +     */
> +    uint32_t slot = JSSLOT_FREE(&BlockClass) + index;
> +    const Shape *shape = addPropertyInternal(cx, id, let_getProperty, let_setProperty,

You could just return this and cut out a few unnecessary lines.

::: js/src/vm/CallObject.h
@@ +56,5 @@
> + *   JSObject                   Generic object
> + *     \
> + *   ScopeObject                Engine-internal scope
> + *     \   \   \
> + *      \   \  DeclEnvObject    Holds name of recursive named lambda

It's really recursive and/or heavyweight, isn't it?  Change this, please.

@@ +78,5 @@
> + *
> + * *** Static block objects are a special case: these objects are created at
> + * compile time to hold the shape/binding information from which block objects
> + * are cloned at runtime. These objects should never escape into the wild and
> + * support a restricted set of ScopeObject operations.

Delete those operations that aren't supported, please.

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 585321 [details] [diff] [review]
do it (v.2)

>diff --git a/js/src/jsfriendapi.h b/js/src/jsfriendapi.h
>--- a/js/src/jsfriendapi.h
>+++ b/js/src/jsfriendapi.h
>-IsScopeObject(const JSObject *obj);
>+IsScopeObject(JSObject *obj);

Why's that?

>diff --git a/js/src/jsobj.h b/js/src/jsobj.h
>--- a/js/src/jsobj.h
>+++ b/js/src/jsobj.h
>+     * SpiderMonkey has not been completely transition to the isX/asX/XObject

transitioned

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 585321 [details] [diff] [review]
do it (v.2)

Review of attachment 585321 [details] [diff] [review]:
-----------------------------------------------------------------

The modifications you noted on IRC (to EmitEnterBlock, and to ScopeObject.h/inl) look good.  Pack the previous comments into the patch, and r=me.

Comment 5

[Luke Wagner [:luke]]

(In reply to Jeff Walden (remove +bmo to email) from comment #2)

Thanks, lots of good comments.

> I see some inconsistency in changing locals to reference type from pointer
> type.

Yes, I did this in a rather ad hoc manner.  In some cases, the pointer was being reassigned; in others, I would have had to add a bunch of address-of's and I didn't want to make a big patch bigger.

> Could we rename this "put" operation at some point (not here, of course),
> maybe something like storeBlockVariablesToStack?  Dunno, but "put" is not a
> good name.

How about I remove them altogether in bug 659577 ;-) ?

> > +     * Note that these XObject classes form a hierarchy. For example, a cloned
> > +     * block object isClonedBlock, isBlock, isNestedScope and isScope. Each of
> 
> You accidentally the verb?

Actually, I was being cute (the verb being the "is" in "isClonedBlock"), but it doesn't really work.  I'll reword.

> >  inline bool JSObject::isArguments() const { return isNormalArguments() || isStrictArguments(); }
> 
> ...I'm pretty surprised this doesn't trigger
> is{Normal,Strict}Arguments-not-defined warnings.

I think such warnings are only made after the whole translation unit has been parsed since that's all the compiler needs.

> Perhaps this slot should be changed to a PrivateUint32 slot at some point.

Ooh, yeah, I'll do that.

> Perhaps we should start deleting the methods that don't make sense from
> object subclasses.

Good idea; I'll do that (it found two cases).
 
> I've done DeclEvnObject::jsClass before -- maybe worth doing here, and for
> the other such cases.

The problem is that static class members can't be forward declared so it forces some unpleasant inclusion ordering problems.  Perhaps a future thing someone will do all at once b/c, for the moment, all js::Class's tend to be non-static-members.

> Private is apparently slower than reserved slot nowadays.  Followup to
> switch to a reserved slot perhaps?

The shiny future is to not store a StackFrame* at all (bug 659577).

Comment 6

[Luke Wagner [:luke]]

(In reply to Ms2ger from comment #3)
> >-IsScopeObject(const JSObject *obj);
> >+IsScopeObject(JSObject *obj);
> 
> Why's that?

b/c 'const' is annoying and verbose when applied to most JSObject members so we end up injecting random const_casts that make it also useless.  bhackett and I came to the same conclusion about 'const Shape'; hopefully we'll remove that at some point in time.

Comment 7

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f0d76403ae9c

Comment 8

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/f0d76403ae9c