The default bug view has changed. See this FAQ.

finish encapsulating JSObject::fslots

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

7 years ago
Created attachment 445302 [details] [diff] [review]
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.
Attachment #445302 - Flags: review?(brendan)
(Assignee)

Comment 1

7 years ago
Created attachment 450280 [details] [diff] [review]
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.
Attachment #445302 - Attachment is obsolete: true
Attachment #450280 - Flags: review?(gal)
Attachment #445302 - Flags: review?(brendan)
I will review tomorrow, after my keynote and some coffee (probably in the other order; then more coffee). Sorry for the delay.

/be
(Assignee)

Comment 3

7 years ago
(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
(Assignee)

Updated

7 years ago
Attachment #450280 - Flags: review?(gal)
(Assignee)

Comment 4

7 years ago
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.)
Attachment #450280 - Flags: review?(gal)

Updated

7 years ago
Attachment #450280 - Flags: review?(gal) → review+

Comment 5

7 years ago
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
(Assignee)

Comment 6

7 years ago
(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...
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
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 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
(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.
(Assignee)

Comment 11

7 years ago
(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.
(Assignee)

Comment 12

6 years ago
Bug 673451 made JSObject::slots private, so there's nothing left to do here.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.