539829 - Encapsulate JSScopeProperty::flags

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

See bug 533876 comment 5.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: v1

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: v2

Oops - missed a spot in v1.

Looking at the interdiff you can see that I'm doing something a little awkward. This is because I'm not entirely clear on whether it's OK to be calling these methods during GC. If I felt confident about that, I would just use getFlags() there instead.

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 424350 [details] [diff] [review]
v2

>diff --git a/js/src/jsscope.cpp b/js/src/jsscope.cpp

> static JSDHashNumber
> js_HashScopeProperty(JSDHashTable *table, const void *key)
> {
>     const JSScopeProperty *sprop = (const JSScopeProperty *)key;
>-    JSDHashNumber hash;
...
>-    hash = JS_ROTATE_LEFT32(hash, 4) ^ sprop->id;
>-    return hash;
>+    return sprop->hash();
> }

Very nice.


>-static JSScopeProperty *
>-GetPropertyTreeChild(JSContext *cx, JSScopeProperty *parent,
>-                     const JSScopeProperty &child)
>+JSScopeProperty *
>+js_GetPropertyTreeChild(JSContext *cx, JSScopeProperty *parent,
>+                        const JSScopeProperty &child)

This method feels like it should be a member function, maybe in JSScopeProperty, at some point -- but not necessarily now.


>         /*
>          * Pass null to make a stub getter, but pass along sprop->setter to
>          * preserve watchpoints. Clear SPROP_IS_METHOD from flags as we are
>          * despecializing from a method memoized in the property tree to a
>          * plain old function-valued property.
>          */
>         sprop = putProperty(cx, sprop->id, NULL, sprop->setter, sprop->slot,
>-                            sprop->attrs, sprop->flags & ~SPROP_IS_METHOD,
>+                            sprop->attrs,
>+                            sprop->flags & ~(JSScopeProperty::IN_DICTIONARY |
>+                                             JSScopeProperty::METHOD),
>                             sprop->shortid);

I don't like this "random" clearing of IN_DICTIONARY here.  The comment doesn't explain why it's necessary, and it's not obviously motivated by the surrounding code.  Is there some better abstraction we can use to do this?  Maybe a copyFlags() method that removes the two bits or something?  At the very least, if a way to perform such abstraction isn't immediately obvious, the comment above should explain why IN_DICTIONARY is being removed.

Another possibility: consolidate the public-facing flag bits into a bitfield and move the private ones to the end, and hide them.  Then you wouldn't have to worry about extra bits, because the process of assignment of potential flags to the bitfield would lop them off (or if you selected bit-by-bit you'd lose them equally easily).


>-static void
>-DumpSubtree(JSContext *cx, JSScopeProperty *sprop, int level, FILE *fp)
>+void
>+JSScopeProperty::dump(JSContext *cx, FILE *fp)
> {

Start this off with a !null(id) assertion.


>+    if (attrs) {
>+        int first = 1;
>+        fputs("(", fp);
>+#define DUMP_ATTR(name) if (attrs & JSPROP_##name) fputs(" " #name + first, fp), first = 0
>+        DUMP_ATTR(ENUMERATE);
>+        DUMP_ATTR(READONLY);
>+        DUMP_ATTR(PERMANENT);
>+        DUMP_ATTR(GETTER);
>+        DUMP_ATTR(SETTER);
>+        DUMP_ATTR(SHARED);
>+#undef  DUMP_ATTR
>+        fputs(") ", fp);
>+    }

I have a feeling we're going to get tired of seeing attributes (and flags further down) SHOUTED TO HIGH HEAVEN like this.  Maybe add a second argument that's the first argument lowercased so that it's not so loud.


>@@ -2007,29 +1990,29 @@ js_SweepScopeProperties(JSContext *cx)
>             /*
>              * If the mark bit is set, sprop is alive, so clear the mark bit
>              * and continue the while loop.
>              *
>              * Regenerate sprop->shape if it hasn't already been refreshed
>              * during the mark phase, when live scopes' lastProp members are
>              * followed to update both scope->shape and lastProp->shape.
>              */
>-            if (sprop->flags & SPROP_MARK) {
>-                sprop->flags &= ~SPROP_MARK;
>+            if (sprop->flags & JSScopeProperty::MARK) {
>+                sprop->flags &= ~JSScopeProperty::MARK;

Either a marked() or (...maybe) alive() class inline seems appropriate for this action.


>                 if (rt->gcRegenShapes) {
>-                    if (sprop->flags & SPROP_FLAG_SHAPE_REGEN)
>-                        sprop->flags &= ~SPROP_FLAG_SHAPE_REGEN;
>+                    if (sprop->flags & JSScopeProperty::SHAPE_REGEN)
>+                        sprop->flags &= ~JSScopeProperty::SHAPE_REGEN;

hasRegenFlag()


>diff --git a/js/src/jsscope.h b/js/src/jsscope.h

>+private:
>+    /* Implementation-private bits stored in sprop->flags. */
>+    enum {
>+        MARK =          0x01,
>+        SHAPE_REGEN =   0x08,
>+        IN_DICTIONARY = 0x20
>+    };

I'd like to see some brief remarks next to these, strawmen ideas below (is third correct?  not fully certain dictionaries aren't shared, would seem folly if they were):

  /* Mark bit for tracking liveness. */
  /*
   * Bit used to track when shapes must be regenerated; meaning
   * is inverted after each GC sweep.
   */
  /* Property stored in per-object dictionary, not shared property tree. */


>+    bool inDictionary() const { return (flags & IN_DICTIONARY) != 0; }

You don't need the != 0, likewise for the other accessors.  I'm agnostic to whether that should or should not be canonical style, myself; do as you see fit.


> /*
>- * If SPROP_HAS_SHORTID is set in sprop->flags, we use sprop->shortid rather
>+ * If SHORTID is set in sprop->flags, we use sprop->shortid rather
>  * than id when calling sprop's getter or setter.
>  */
> #define SPROP_USERID(sprop)                                                   \
>-    (((sprop)->flags & SPROP_HAS_SHORTID) ? INT_TO_JSVAL((sprop)->shortid)    \
>-                                          : ID_TO_VALUE((sprop)->id))
>+    ((sprop)->hasShortID() ? INT_TO_JSVAL((sprop)->shortid)                   \
>+                           : ID_TO_VALUE((sprop)->id))

Should we have JSScopeProperty::userID?  Followup if you think so.


>diff --git a/js/src/jsscopeinlines.h b/js/src/jsscopeinlines.h

>         if (sprop) {
>-            if (!(sprop->flags & SPROP_FLAG_SHAPE_REGEN)) {
>+            if (!(sprop->flags & JSScopeProperty::SHAPE_REGEN)) {

!sprop->hasRegenFlag()

Please check whether there are any other places which could use this change while you're at it.


>+inline JSDHashNumber
>+JSScopeProperty::hash() const
>+{
>+    JSDHashNumber hash = 0;
>+
>+    /* Accumulate from least to most random so the low bits are most random. */
>+    JS_ASSERT_IF(isMethod(), !setter || setter == js_watch_set);
>+    if (getter)
>+        hash = JS_ROTATE_LEFT32(hash, 4) ^ jsword(getter);
>+    if (setter)
>+        hash = JS_ROTATE_LEFT32(hash, 4) ^ jsword(setter);
>+    hash = JS_ROTATE_LEFT32(hash, 4) ^ (flags & PUBLIC_FLAGS);
>+    hash = JS_ROTATE_LEFT32(hash, 4) ^ attrs;
>+    hash = JS_ROTATE_LEFT32(hash, 4) ^ shortid;
>+    hash = JS_ROTATE_LEFT32(hash, 4) ^ slot;
>+    hash = JS_ROTATE_LEFT32(hash, 4) ^ id;
>+    return hash;
>+}

Why are you casting to jsword here rather than jsuword, as was previously done, and as matches the signedness of JSDHashNumber?

Tangentially, does truncation to 32 bits adequately hash pointers on 64-bit OSes?  Followup bug if you agree this deserves more thought.


>+inline bool
>+JSScopeProperty::matches(const JSScopeProperty *p) const
>+{
>+    return (id == p->id &&
>+            matchesParamsAfterId(p->getter, p->setter, p->slot, p->attrs, p->flags, p->shortid));
>+}

You've removed the !null-id assertions that check that |this| and |p| aren't GC'd; please readd them.

Less substantively, |pcregrep '.*return.*==.*' *.cpp| in js/src suggests it's more common style not to over-parenthesize here.


>+inline bool
>+JSScopeProperty::matchesParamsAfterId(JSPropertyOp agetter, JSPropertyOp asetter, uint32 aslot,
>+                                      uintN aattrs, uintN aflags, intN ashortid) const
>+{
>+    return (getter == agetter &&
>+            setter == asetter &&
>+            slot == aslot &&
>+            attrs == aattrs &&
>+            ((flags ^ aflags) & PUBLIC_FLAGS) == 0 &&
>+            shortid == ashortid);
>+}

Since this method isn't called only through matches(), please add a !null(this->id) assertion here as well.


All great except for the bit about "randomly" removing IN_DICTIONARY -- once that's resolved the rest is fine with the requested changes.

Comment 4

[Jason Orendorff [:jorendorff]]

Thanks for the review. I've taken all the suggestions except as explained below. New patch after lunch.

(In reply to comment #3)
> >-static JSScopeProperty *
> >-GetPropertyTreeChild(JSContext *cx, JSScopeProperty *parent,
> >-                     const JSScopeProperty &child)
> >+JSScopeProperty *
> >+js_GetPropertyTreeChild(JSContext *cx, JSScopeProperty *parent,
> >+                        const JSScopeProperty &child)
> 
> This method feels like it should be a member function, maybe in
> JSScopeProperty, at some point -- but not necessarily now.

Right. I think there should be a JSPropertyTree class with this stuff, also a friend of JSScopeProperty, and at that point JSScopeProperty::kids can be made private too.

(Maybe JSScopeProperty should have two subclasses: JSTreeScopeProperty with `kids` and `shape` and JSPropertyTree as a friend, and JSDictScopeProperty with childp. A bit much perhaps.)

> >+    bool inDictionary() const { return (flags & IN_DICTIONARY) != 0; }
> 
> You don't need the != 0, likewise for the other accessors.  I'm agnostic to
> whether that should or should not be canonical style, myself; do as you see
> fit.

I was very slightly worried that MSVC might spit out a stupid warning if I omitted the !=. I'm not sure exactly what situations cause the stupid warning. If you have MSVC handy, I'd appreciate it if you'd check, and then I'll get rid of the superfluous `!= 0`.

> Should we have JSScopeProperty::userID?  Followup if you think so.

I think so. I'll do it in bug 539170.

> >+        hash = JS_ROTATE_LEFT32(hash, 4) ^ jsword(getter);
>[...]
> Why are you casting to jsword here rather than jsuword, as was previously done,
> and as matches the signedness of JSDHashNumber?

The original casts to jsword too. I just copied those lines over from js_HashScopeProperty. But you're right, jsuword makes more sense. Changed.

> Tangentially, does truncation to 32 bits adequately hash pointers on 64-bit
> OSes?  Followup bug if you agree this deserves more thought.

I'd be surprised if it made any difference. Feel free to file a follow-up anyway, of course.

> >+    return (id == p->id &&
> >+            matchesParamsAfterId(p->getter, p->setter, p->slot, p->attrs, p->flags, p->shortid));
>[...]
> Less substantively, |pcregrep '.*return.*==.*' *.cpp| in js/src suggests it's
> more common style not to over-parenthesize here.

Oh - the over-parenthesization was against the line break, not the low-precedence operator. It was an indentation thing. Now I have jimb's mozilla-c-style installed, so it won't happen again.

Comment 5

[Jason Orendorff [:jorendorff]]

Attachment: v3

Removed the "random" bitmask in favor of using getFlags(). This is the right approach because JSScope::methodScopeChange is "outside" the part of JSScope that needs to know about private flags of JSScopeProperty. Really only a few GC-related and property-tree-related functions need friend status--maybe this can be a short-lived friendship; I don't know yet.

(In comment 2 I mentioned I didn't know if this could be called during GC. I checked and it clearly can't.)

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 425064 [details] [diff] [review]
v3

I looked at the jsword bit multiple times and misread it as previously being jsuword, not sure how that happened...


>diff --git a/js/src/jsdbgapi.cpp b/js/src/jsdbgapi.cpp

>-            userid = (sprop->flags & SPROP_HAS_SHORTID)
>-                     ? INT_TO_JSVAL(sprop->shortid)
>-                     : propid;
>+            userid = sprop->hasShortID() ? INT_TO_JSVAL(sprop->shortid) : propid;

I think this is equivalent to |SPROP_USERID(sprop)| which should probably be used to keep delving into the userid abstraction to a minimum of locations.


>diff --git a/js/src/jsscope.h b/js/src/jsscope.h

>+    bool marked() const { return (flags & MARK) != 0; }
>+    void setMark() { flags |= MARK; }
>+    void clearMark() { flags &= ~MARK; }

It seems to me the right verb is simply "mark" here, rather than "setMark".  It's mark-and-sweep, after all, not set-mark-and-sweep.  "clearMark" is all cool, although you might try "unmark" on for size.


>+    bool hasRegenFlag() const { return (flags & SHAPE_REGEN) != 0; }
>+    void setRegenFlag() { flags |= SHAPE_REGEN; }
>+    void clearRegenFlag() { flags &= ~SHAPE_REGEN; }

Purty.  I kind of hesitate to suggest setters like this, but it probably is better in terms of allowing the underlying bit-storage to be changed with greater ease.

Comment 7

[Jason Orendorff [:jorendorff]]

Sorry, this landed weeks ago.

http://hg.mozilla.org/tracemonkey/rev/5ac654258c2c

Comment 8

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/5ac654258c2c