565845 - finish encapsulating JSObject::fslots

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

Attachment: patch

This patch finishes makes 'fslots' a private member of JSObject.

- It encapsulates fslot accesses of "property iterators".
  isPropertyIterator() is not an inline function, unlike most other isXYZ()
  functions, because doing so would require putting it in jsapi.h which
  didn't seem like a good idea.

- It encapsulates fslot accesses of Block and With objects, and separates these
  more than they previously were, for stronger assertions, and because Block
  objects use more slots.

  slotNumOfBlockFirstLocal() and installBlockLocals() are both ugly, but I
  couldn't see better ways to do them.

  The comments describing the Block and With slots are garbled, because the
  single comment that they are derived from was garbled.  Suggestions for
  improvements are welcome.

- It encapsulates fslot accesses of NoSuchMethod objects.  isNoSuchMethod()
  is not an inline function, unlike most other isXYZ() functions, because it
  should only occur on error cases where speed doesn't seem important.

- It adds some isIterator() tests to the Iterator getters/setters.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: patch, v2

I rebased the patch and added more getters/setters for all the new JSSLOT_*
values that Gal added for proxies.

One thing I'm not sure about -- we already had a function isCallable(), so I
created isCallableObject().  Maybe these could be named better.

Comment 2

[Brendan Eich [:brendan]]

I will review tomorrow, after my keynote and some coffee (probably in the other order; then more coffee). Sorry for the delay.

/be

Comment 3

[Nicholas Nethercote [inactive]]

(In reply to comment #2)
> I will review tomorrow, after my keynote and some coffee (probably in the other
> order; then more coffee). Sorry for the delay.

ping

Comment 4

[Nicholas Nethercote [inactive]]

Comment on attachment 450280 [details] [diff] [review]
patch, v2

Re-requesting review from Gal on this one.  (I'm actually not fussed who reviews it, but it would be nice if someone did.)

Comment 5

[Andreas Gal :gal]

Comment on attachment 450280 [details] [diff] [review]
patch, v2

>-        JS_ASSERT(JSVAL_IS_VOID(blockObj->fslots[JSSLOT_BLOCK_DEPTH]));
>-
>-        OBJ_SET_BLOCK_DEPTH(cx, blockObj, cg->stackDepth);
>+        JS_ASSERT(blockObj->isBlockDepthVoid());
>+
>+        blockObj->setBlockDepth(cg->stackDepth);

This should be ->blockDepthIsNotSet() or something like that.

>     clasp = obj->getClass();
>-    if ((clasp == &js_WithClass || clasp == &js_BlockClass) &&
>+    if (clasp == &js_BlockClass &&
>         obj->getPrivate() == js_FloatingFrameIfGenerator(cx, cx->fp) &&
>-        OBJ_BLOCK_DEPTH(cx, obj) >= stackDepth) {
>+        obj->getBlockDepth() >= stackDepth) {
>+        return clasp;
>+    }
>+    if (clasp == &js_WithClass &&
>+        obj->getPrivate() == js_FloatingFrameIfGenerator(cx, cx->fp) &&
>+        obj->getWithDepth() >= stackDepth) {
>         return clasp;
>     }

Is this duplication necessary? (seems like it, just trying to get you to think about it.

> JS_REQUIRES_STACK JSBool
> js_PutBlockObject(JSContext *cx, JSBool normalUnwind)
> {
>-    /* Blocks have one fixed slot available for the first local.*/
>-    JS_STATIC_ASSERT(JS_INITIAL_NSLOTS == JSSLOT_BLOCK_DEPTH + 2);
>-

Did we lose this assert?

>+    normalUnwind = obj->installBlockLocals(cx, normalUnwind, fp->slots() + depth, count);

copyBlockLocals?
>-        depth = (uint16)OBJ_BLOCK_DEPTH(cx, obj);
>+        depth = (uint16)obj->getBlockDepth();
>         count = (uint16)OBJ_BLOCK_COUNT(cx, obj);
>         tmp = (uint32)(depth << 16) | count;

Since you are in town, uint16() is easier to read.

>+  public:
>     jsval       *dslots;                    /* dynamically allocated slots */

This one next would be nice.

Nice path. Nice cleanup. Love it. Now that having said, is there any chance I can talk you into not landing this for a little while? Maybe 2 weeks? We have several major patch queues in flight (fat values/luke, jaegermonkey/david^2, gc compartments/jorendorf+blake+me, scope removal/brendan). This patch touches a lot of code all over the place and is going to make a lot of lives miserable in the merge department. Up to you though. r=me either way

Comment 6

[Nicholas Nethercote [inactive]]

(In reply to comment #5)
> >+        JS_ASSERT(blockObj->isBlockDepthVoid());

> This should be ->blockDepthIsNotSet() or something like that.

I used "Void" deliberately because sometimes we use NULL, sometimes VOID, and language like "NotSet" doesn't make this clear.


> > JS_REQUIRES_STACK JSBool
> > js_PutBlockObject(JSContext *cx, JSBool normalUnwind)
> > {
> >-    /* Blocks have one fixed slot available for the first local.*/
> >-    JS_STATIC_ASSERT(JS_INITIAL_NSLOTS == JSSLOT_BLOCK_DEPTH + 2);
> >-
> 
> Did we lose this assert?

Yes, but with the slots stuff all together in one place it's not nearly as necessary.


> >     jsval       *dslots;                    /* dynamically allocated slots */
> 
> This one next would be nice.
> 
> Nice path. Nice cleanup. Love it. Now that having said, is there any chance I
> can talk you into not landing this for a little while?

I'm planning on getting to dslots, but it's already stalled on brendan's request.  I guess I can stall this one too.  Hopefully the rate of churn will slow some time this year...

Comment 7

[Brendan Eich [:brendan]]

I suck, sorry for the tardy review. I've been head down on a patch that will conflict, but that's no excuse. Reviewing.

/be

Comment 8

[Brendan Eich [:brendan]]

Blake, is the JS_NewPropertyIterator API still needed? I'd like to get rid of it (separate bug of course, just saw it getting love here that it doesn't deserve if it is a goner).

/be

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 450280 [details] [diff] [review]
patch, v2

> JS_REQUIRES_STACK JSClass *
> js_IsActiveWithOrBlock(JSContext *cx, JSObject *obj, int stackDepth)
> {
>     JSClass *clasp;
> 
>     clasp = obj->getClass();
>-    if ((clasp == &js_WithClass || clasp == &js_BlockClass) &&
>+    if (clasp == &js_BlockClass &&
>         obj->getPrivate() == js_FloatingFrameIfGenerator(cx, cx->fp) &&
>-        OBJ_BLOCK_DEPTH(cx, obj) >= stackDepth) {
>+        obj->getBlockDepth() >= stackDepth) {
>+        return clasp;
>+    }
>+    if (clasp == &js_WithClass &&
>+        obj->getPrivate() == js_FloatingFrameIfGenerator(cx, cx->fp) &&
>+        obj->getWithDepth() >= stackDepth) {
>         return clasp;
>     }
>     return NULL;
> }

This code duplication is in the noise, and possibly a super-optimizing compiler could re-unify common code, but I wanted to point out that With and Block objects having a slot in common, the "stack depth" (let's call it, instead of block depth), is not unsound or even ugly (with the right name ;-). Imagine a common subtype of object that is the supertype of with and block sub(-sub)types.

This patch is good for cleanliness. It's gonna conflict with my patch queue for bug 558451 in a big way. I can cope, but that bug aims for a big perf win, not just cleaner code. I'm lagging on it in part due to all the rebasing.

Nick: can this patch can wait a bit longer? Then you'll have to take the rebasing pain. It's a direct trade-off, but the scope elimination win is the bigger prize. Don't let me twist your arm here, though -- it's "just" merge conflict hand-resolution.

/be

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #8)
> Blake, is the JS_NewPropertyIterator API still needed? I'd like to get rid of
> it (separate bug of course, just saw it getting love here that it doesn't
> deserve if it is a goner).

Yeah, it is. We still use it to enumerate the inner window when enumerating over the outer window.

Comment 11

[Nicholas Nethercote [inactive]]

(In reply to comment #9)
> 
> This code duplication is in the noise, and possibly a super-optimizing compiler
> could re-unify common code, but I wanted to point out that With and Block
> objects having a slot in common, the "stack depth" (let's call it, instead of
> block depth), is not unsound or even ugly (with the right name ;-). Imagine a
> common subtype of object that is the supertype of with and block
> sub(-sub)types.

Sure.  My judgment was that it was better to separate them nonetheless.  I could be convinced otherwise (as I was for the XML classes) but it was a decision made with some thought.


> Nick: can this patch can wait a bit longer?

Yes, see comment 6.

Comment 12

[Nicholas Nethercote [inactive]]

Bug 673451 made JSObject::slots private, so there's nothing left to do here.