Closed Bug 556187 Opened 10 years ago Closed 10 years ago

encapsulate JSSLOT_ARRAY_* within JSObject

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: njn, Assigned: njn)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(2 files, 3 obsolete files)

Attached patch patch (obsolete) — Splinter Review
This patch encapsulates all fslots[JSSLOT_ARRAY_*] accesses.  It:

- Moves JSSLOT_ARRAY_* into JSObject and makes them private.
  
- Makes TraceRecorder a friend of JSObject, because it needs to know
  JSObject's internals in order to generate code that manipulates JSObjects.

- Moves a static assertion that JSSLOT_ARRAY_LENGTH==JSSLOT_PRIVATE into
  JSObject, because JSSLOT_ARRAY_* is now private.

- Adds {get,set}ArrayLength(), {get,set}ArrayCount(), {inc,dec}ArrayCount(),
  clearArrayUnused().  Uses them everywhere that's appropriate.  They are in
  jsobjinlines.h because they use isArray().

Note that several old fslots[JSSLOT_ARRAY_LENGTH] expressions were cast to
an unsigned type;  this is no longer necessary because the return type of
getArrayLength() is uint32, so I removed these casts.  But there some slight
mismatches -- uint32 vs. jsuint -- that I didn't try to address.

Shaver, Gal, feedback about whether this will cause problems with array-ng would be useful.
Attachment #436115 - Flags: review?(brendan)
Attachment #436115 - Flags: feedback?(gal)
Summary: mostly encapsulate JSSLOT_ARRAY_* within JSObject → encapsulate JSSLOT_ARRAY_* within JSObject
Comment on attachment 436115 [details] [diff] [review]
patch

>@@ -492,20 +493,20 @@ SetArrayElement(JSContext *cx, JSObject 
>     if (obj->isDenseArray()) {
>         /* Predicted/prefetched code should favor the remains-dense case. */
>         if (index <= jsuint(-1)) {
>             jsuint idx = jsuint(index);
>             if (!INDEX_TOO_SPARSE(obj, idx)) {
>                 JS_ASSERT(idx + 1 > idx);
>                 if (!EnsureCapacity(cx, obj, idx + 1))
>                     return JS_FALSE;
>-                if (idx >= uint32(obj->fslots[JSSLOT_ARRAY_LENGTH]))
>-                    obj->fslots[JSSLOT_ARRAY_LENGTH] = idx + 1;
>+                if (idx >= uint32(obj->getArrayLength()))

No need for uint32 cast given the return type is uint32.

>+                    obj->setArrayLength(idx + 1);
>                 if (obj->dslots[idx] == JSVAL_HOLE)
>-                    obj->fslots[JSSLOT_ARRAY_COUNT]++;
>+                    obj->incArrayCountBy(1);
>                 obj->dslots[idx] = v;
>                 return JS_TRUE;
>             }
>         }
> 
>         if (!js_MakeArraySlow(cx, obj))
>             return JS_FALSE;
>     }
>@@ -523,17 +524,17 @@ static JSBool
> DeleteArrayElement(JSContext *cx, JSObject *obj, jsdouble index)
> {
>     JS_ASSERT(index >= 0);
>     if (obj->isDenseArray()) {
>         if (index <= jsuint(-1)) {
>             jsuint idx = jsuint(index);
>             if (!INDEX_TOO_SPARSE(obj, idx) && idx < js_DenseArrayCapacity(obj)) {
>                 if (obj->dslots[idx] != JSVAL_HOLE)
>-                    obj->fslots[JSSLOT_ARRAY_COUNT]--;
>+                    obj->decArrayCountBy(1);
>                 obj->dslots[idx] = JSVAL_HOLE;
>                 return JS_TRUE;
>             }
>         }
>         return JS_TRUE;
>     }
> 
>     AutoIdRooter idr(cx);
>@@ -614,17 +615,17 @@ js_IsArrayLike(JSContext *cx, JSObject *
>  * array that caused this getter to be invoked. In the setter case to overcome
>  * the JSPROP_SHARED attribute, we must define a shadowing length property.
>  */
> static JSBool
> array_length_getter(JSContext *cx, JSObject *obj, jsval id, jsval *vp)
> {
>     do {
>         if (obj->isArray())
>-            return IndexToValue(cx, obj->fslots[JSSLOT_ARRAY_LENGTH], vp);
>+            return IndexToValue(cx, obj->getArrayLength(), vp);
>     } while ((obj = obj->getProto()) != NULL);
>     return JS_TRUE;
> }
> 
> static JSBool
> array_length_setter(JSContext *cx, JSObject *obj, jsval id, jsval *vp)
> {
>     jsuint newlen, oldlen, gap, index;
>@@ -634,26 +635,26 @@ array_length_setter(JSContext *cx, JSObj
>         jsid lengthId = ATOM_TO_JSID(cx->runtime->atomState.lengthAtom);
> 
>         return obj->defineProperty(cx, lengthId, *vp, NULL, NULL, JSPROP_ENUMERATE);
>     }
> 
>     newlen = ValueIsLength(cx, vp);
>     if (JSVAL_IS_NULL(*vp))
>         return false;
>-    oldlen = obj->fslots[JSSLOT_ARRAY_LENGTH];
>+    oldlen = obj->getArrayLength();
> 
>     if (oldlen == newlen)
>         return true;
> 
>     if (!IndexToValue(cx, newlen, vp))
>         return false;
> 
>     if (oldlen < newlen) {
>-        obj->fslots[JSSLOT_ARRAY_LENGTH] = newlen;
>+        obj->setArrayLength(newlen);
>         return true;
>     }
> 
>     if (obj->isDenseArray()) {
>         /* Don't reallocate if we're not actually shrinking our slots. */
>         jsuint capacity = js_DenseArrayCapacity(obj);
>         if (capacity > newlen && !ResizeSlots(cx, obj, capacity, newlen))
>             return false;
>@@ -686,33 +687,33 @@ array_length_setter(JSContext *cx, JSObj
>                 break;
>             if (js_IdIsIndex(id, &index) && index - newlen < gap &&
>                 !obj->deleteProperty(cx, id, &junk)) {
>                 return false;
>             }
>         }
>     }
> 
>-    obj->fslots[JSSLOT_ARRAY_LENGTH] = newlen;
>+    obj->setArrayLength(newlen);
>     return true;
> }
> 
> /*
>  * We have only indexed properties up to capacity (excepting holes), plus the
>  * length property. For all else, we delegate to the prototype.
>  */
> static inline bool
> IsDenseArrayId(JSContext *cx, JSObject *obj, jsid id)
> {
>     JS_ASSERT(obj->isDenseArray());
> 
>     uint32 i;
>     return id == ATOM_TO_JSID(cx->runtime->atomState.lengthAtom) ||
>            (js_IdIsIndex(id, &i) &&
>-            obj->fslots[JSSLOT_ARRAY_LENGTH] != 0 &&
>+            obj->getArrayLength() != 0 &&
>             i < js_DenseArrayCapacity(obj) &&

js_DenseArrayCapacity wants a make-over -- separate bug or here if you like.

>@@ -922,19 +923,19 @@ dense_grow(JSContext* cx, JSObject* obj,
>     jsuint capacity = js_DenseArrayCapacity(obj);
>     if ((u >= capacity) && (INDEX_TOO_SPARSE(obj, u) || !EnsureCapacity(cx, obj, u + 1)))
>         return JS_FALSE;
> 
>     if (obj->dslots[u] == JSVAL_HOLE) {
>         if (js_PrototypeHasIndexedProperties(cx, obj))
>             return JS_FALSE;
> 
>-        if (u >= jsuint(obj->fslots[JSSLOT_ARRAY_LENGTH]))
>-            obj->fslots[JSSLOT_ARRAY_LENGTH] = u + 1;
>-        ++obj->fslots[JSSLOT_ARRAY_COUNT];
>+        if (u >= jsuint(obj->getArrayLength()))

jsuint is a uint32 typedef-synonym -- no cast needed? Maybe it's worth keeping to haunt us into unifying on one or the other name.

I feel haunted, at any rate. JS arrays were originally (Netscape 2) not uint32 length constrained. That came during standardization at Microsoft's behest. I agreed for no good reason. Using uint32 does not really enable optimizations in either index bounding or length storage. It's more of a de-optimizer.

>@@ -1333,24 +1334,24 @@ js_MakeArraySlow(JSContext *cx, JSObject
> 
>         sprop = scope->addDataProperty(cx, id, JS_INITIAL_NSLOTS + i,
>                                        JSPROP_ENUMERATE);
>         if (!sprop)
>             goto out_bad;
>     }
> 
>     /*
>-     * Render our formerly-reserved count property GC-safe.
>-     * We do not need to make the length slot GC-safe as this slot is private
>-     * where the implementation can store an arbitrary value.
>+     * Render our formerly-reserved count property GC-safe.  We do not need to
>+     * make the length slot GC-safe because it is the private slot (this is
>+     * statically asserted within JSObject) where the implementation can store
>+     * an arbitrary value.
>      */
>     {
>-        JS_STATIC_ASSERT(JSSLOT_ARRAY_LENGTH == JSSLOT_PRIVATE);
>         JS_ASSERT(js_SlowArrayClass.flags & JSCLASS_HAS_PRIVATE);
>-        obj->fslots[JSSLOT_ARRAY_COUNT] = JSVAL_VOID;
>+        obj->setArrayCount(uint32(JSVAL_VOID));

This is wrong in the abstract, although it happens to work. A jsval should not be chopped blindly to uint32. Hardcoding knowledge that JSVAL_VOID can be safely chopped is undesirable if we ever change to a very wide in set bits sense undefined representation (we used to have such, we changed to a small pseudo-boolean, but changing once suggests we might change again).

At this point obj is all-but-slow and it can't go back to being dense. Since slow arrays do not have a count fslot, this code should just use a generic fslots setter, or direct assignment (gasp), to make the point clear that this is not setting count.

>@@ -2708,20 +2708,20 @@ array_splice(JSContext *cx, uintN argc, 
>             (length == 0 || obj->dslots[length - 1] != JSVAL_HOLE)) {
>             if (!EnsureCapacity(cx, obj, length + delta))
>                 return JS_FALSE;
>             /* (uint) end could be 0, so we can't use a vanilla >= test. */
>             while (last-- > end) {
>                 jsval srcval = obj->dslots[last];
>                 jsval* dest = &obj->dslots[last + delta];
>                 if (*dest == JSVAL_HOLE && srcval != JSVAL_HOLE)
>-                    obj->fslots[JSSLOT_ARRAY_COUNT]++;
>+                    obj->incArrayCountBy(1);
>                 *dest = srcval;
>             }
>-            obj->fslots[JSSLOT_ARRAY_LENGTH] += delta;
>+            obj->setArrayLength(obj->getArrayLength() + delta);

Do gcc and MSVC both optimize the new code as if it were the old?

Too tired to do more than skim the rest, looks ok.

/be
I'll attach a new patch shortly (it's annoying that Bugzilla doesn't easily let you reply to a comment and attach a file at the same time).

> >+                if (idx >= uint32(obj->getArrayLength()))
> 
> No need for uint32 cast given the return type is uint32.

Fixed.


> >             i < js_DenseArrayCapacity(obj) &&
>
> js_DenseArrayCapacity wants a make-over -- separate bug or here if you like.

Separate bug, one of many to come.

 
> >+        if (u >= jsuint(obj->getArrayLength()))
> 
> jsuint is a uint32 typedef-synonym -- no cast needed?

Removed.  I also grepped for similar cases, you found the only two.


> Maybe it's worth keeping
> to haunt us into unifying on one or the other name.

Maybe.  Filing a bug for it wouldn't hurt.


> >     {
> >-        JS_STATIC_ASSERT(JSSLOT_ARRAY_LENGTH == JSSLOT_PRIVATE);
> >         JS_ASSERT(js_SlowArrayClass.flags & JSCLASS_HAS_PRIVATE);
> >-        obj->fslots[JSSLOT_ARRAY_COUNT] = JSVAL_VOID;
> >+        obj->setArrayCount(uint32(JSVAL_VOID));
> 
> This is wrong in the abstract, although it happens to work. A jsval should not
> be chopped blindly to uint32. Hardcoding knowledge that JSVAL_VOID can be
> safely chopped is undesirable if we ever change to a very wide in set bits
> sense undefined representation (we used to have such, we changed to a small
> pseudo-boolean, but changing once suggests we might change again).
> 
> At this point obj is all-but-slow and it can't go back to being dense. Since
> slow arrays do not have a count fslot, this code should just use a generic
> fslots setter, or direct assignment (gasp), to make the point clear that this
> is not setting count.

I added setSlowArrayCount().  Reason being I just went to the effort of making JSSLOTS_ARRAY_* private, I didn't want to undo some or all of the new privacy for a single case, and adding a single extra setter is not a problem.  I also added isSlowArray().


> >             while (last-- > end) {
> >                 jsval srcval = obj->dslots[last];
> >                 jsval* dest = &obj->dslots[last + delta];
> >                 if (*dest == JSVAL_HOLE && srcval != JSVAL_HOLE)
> >-                    obj->fslots[JSSLOT_ARRAY_COUNT]++;
> >+                    obj->incArrayCountBy(1);
> >                 *dest = srcval;
> >             }
> >-            obj->fslots[JSSLOT_ARRAY_LENGTH] += delta;
> >+            obj->setArrayLength(obj->getArrayLength() + delta);
> 
> Do gcc and MSVC both optimize the new code as if it were the old?

GCC does, I haven't looked at MSVC.  And if it didn't I doubt anyone would notice -- that doesn't look like a hot code path.
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #436115 - Attachment is obsolete: true
Attachment #436547 - Flags: review?(brendan)
Attachment #436115 - Flags: review?(brendan)
Attachment #436115 - Flags: feedback?(gal)
Attached file interdiff between v1 and v2 (obsolete) —
Interdiff busted, here's a rough hand-made one using only 'diff' and some editing.
Attachment #436548 - Attachment is patch: true
Attachment #436548 - Attachment is patch: false
Comment on attachment 436547 [details] [diff] [review]
patch v2

>+    /*
>+     * Render our formerly-reserved count property GC-safe.  We do not need to
>+     * make the length slot GC-safe because it is the private slot (this is
>+     * statically asserted within JSObject) where the implementation can store
>+     * an arbitrary value.
>+     */
>+    {
>+        JS_ASSERT(js_SlowArrayClass.flags & JSCLASS_HAS_PRIVATE);
>+        obj->setSlowArrayCount(JSVAL_VOID);
>+    }

Lose the braces, they haven't been necessary for a while.

A slow array does not have a reserved COUNT slot, so the method name is misleading. A method named initSlowArrayState that does this and the classword change seems better. But that suggests making this whole function into a makeArraySlow or becomeSlowArray method would be even better.

>+inline uint32
>+JSObject::getArrayLength() const {
>+    JS_ASSERT(isArray());
>+    return uint32(fslots[JSSLOT_ARRAY_LENGTH]);
>+}
>+
>+inline uint32 
>+JSObject::getArrayCount() const {
>+    JS_ASSERT(isArray());
>+    return uint32(fslots[JSSLOT_ARRAY_COUNT]);
>+}
>+
>+inline void 
>+JSObject::setArrayLength(uint32 length) {
>+    JS_ASSERT(isArray());
>+    fslots[JSSLOT_ARRAY_LENGTH] = length;
>+}
>+
>+inline void 
>+JSObject::setArrayCount(uint32 count) {
>+    JS_ASSERT(isArray());
>+    fslots[JSSLOT_ARRAY_COUNT] = count;
>+}
>+
>+inline void 
>+JSObject::setSlowArrayCount(jsval count) {
>+    JS_ASSERT(isSlowArray());
>+    fslots[JSSLOT_ARRAY_COUNT] = count;
>+}
>+
>+inline void 
>+JSObject::incArrayCountBy(uint32 posDelta) {
>+    JS_ASSERT(isArray());
>+    fslots[JSSLOT_ARRAY_COUNT] += posDelta;
>+}
>+
>+inline void 
>+JSObject::decArrayCountBy(uint32 negDelta) {
>+    JS_ASSERT(isArray());
>+    fslots[JSSLOT_ARRAY_COUNT] -= negDelta;
>+}
>+
>+inline void
>+JSObject::clearArrayUnused() {
>+    JS_ASSERT(isArray());
>+    fslots[JSSLOT_ARRAY_COUNT] = JSVAL_VOID;
>+}

These all should put the body's opening brace on its own line, as other inline methods not defined in class bodies do:

>+
> inline void
> JSObject::initSharingEmptyScope(JSClass *clasp, JSObject *proto, JSObject *parent,
>                                 jsval privateSlotValue)
> {

Any others like this, same nit.

/be
Nick, one more patch and I'll r+ -- still need to catch up on sleep and give the long tail a close read.

/be
Attached patch patch v3Splinter Review
(In reply to comment #5)
> >+    {
> >+        JS_ASSERT(js_SlowArrayClass.flags & JSCLASS_HAS_PRIVATE);
> >+        obj->setSlowArrayCount(JSVAL_VOID);
> >+    }
> 
> A slow array does not have a reserved COUNT slot, so the method name is
> misleading. A method named initSlowArrayState that does this and the classword
> change seems better. But that suggests making this whole function into a
> makeArraySlow or becomeSlowArray method would be even better.

I decided against makeArraySlow() because js_MakeArraySlow() is a big complicated function and JSObject methods currently are mostly small.  I also decided against initSlowArrayState() because (a) classword isn't encapsulated yet (everywhere else manipulates it directly), and (b) it would be unclear which parts of the slow array state would be init'd -- there's little point encapsulating code within a function if you have to look at its guts to understand what it's doing anyway.

So I went with clearDenseArrayCount() instead (and removed the extraneous curly braces).  Hope that's ok.

> >+inline uint32
> >+JSObject::getArrayLength() const {
> >+    JS_ASSERT(isArray());
> >+    return uint32(fslots[JSSLOT_ARRAY_LENGTH]);
> >+}
> 
> These all should put the body's opening brace on its own line, as other inline
> methods not defined in class bodies do:

Done.
Attachment #436547 - Attachment is obsolete: true
Attachment #437181 - Flags: review?(brendan)
Attachment #436547 - Flags: review?(brendan)
Odd that Bugzilla's interdiff failed but my command line version succeeded.
Attachment #436548 - Attachment is obsolete: true
Comment on attachment 437181 [details] [diff] [review]
patch v3

Slight pref for "void" as verb instead of "clear", r=me with that.

/be
Attachment #437181 - Flags: review?(brendan) → review+
(In reply to comment #9)
> (From update of attachment 437181 [details] [diff] [review])
> Slight pref for "void" as verb instead of "clear", r=me with that.

Ok, and I'll change the newly added clearArrayUnused() to voidArrayUnused() to match.  Thanks.
Depends on: 558530
http://hg.mozilla.org/mozilla-central/rev/4d5a9468115e
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.