Last Comment Bug 713311 - give the remaining scope objects typed interfaces
: give the remaining scope objects typed interfaces
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla12
Assigned To: Luke Wagner [:luke]
:
Mentors:
: 714563 (view as bug list)
Depends on:
Blocks: 690135 742788
  Show dependency treegraph
 
Reported: 2011-12-23 17:10 PST by Luke Wagner [:luke]
Modified: 2012-04-05 10:42 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
do it (305.06 KB, patch)
2011-12-23 17:10 PST, Luke Wagner [:luke]
jwalden+bmo: review+
Details | Diff | Splinter Review
do it (v.2) (308.63 KB, patch)
2012-01-02 12:17 PST, Luke Wagner [:luke]
jwalden+bmo: review+
Details | Diff | Splinter Review

Description Luke Wagner [:luke] 2011-12-23 17:10:42 PST
Created attachment 584145 [details] [diff] [review]
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] 2012-01-02 12:17:38 PST
Created attachment 585321 [details] [diff] [review]
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] (remove +bmo to email) 2012-01-02 12:38:54 PST
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 (⌚ UTC+1/+2) 2012-01-02 12:41:46 PST
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] (remove +bmo to email) 2012-01-02 14:27:07 PST
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] 2012-01-02 14:56:24 PST
(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] 2012-01-02 15:01:07 PST
(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 8 Marco Bonardo [::mak] 2012-01-03 03:46:13 PST
https://hg.mozilla.org/mozilla-central/rev/f0d76403ae9c
Comment 9 Bill McCloskey (:billm) 2012-01-03 13:44:44 PST
*** Bug 714563 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.