Closed Bug 493457 Opened 12 years ago Closed 12 years ago

uniform access to private slots


(Core :: JavaScript Engine, enhancement)

Not set



Tracking Status
status1.9.2 --- beta1-fixed


(Reporter: igor, Assigned: igor)



(Whiteboard: fixed-in-tracemonkey)


(1 file, 3 obsolete files)

Currently SpiderMonkey sources use the following ways to access the private data pointer stored in the private slot:

Reading a slot that can be not-yet-initialized:

    JS_GetPrivate(cx, obj);

    jsval v = STOBJ_GET_SLOT(obj, JSSLOT_PRIVATE); 
    if (!JSVAL_IS_VOID(v))
        p = JSVAL_TO_PRIVATE(v);

Reading a previously initialized slot:
    OBJ_GET_PRIVATE(cx, obj);

    STOBJ_GET_PRIVATE(cx, obj);





    obj->fslots[JSSLOT_PRIVATE] = JSVAL_TO_PRIVATE(data);

    JS_SetPrivate(cx, obj, data);

This suggests to consolidate all of these cases using few inline functions covering all the intended usage. 

It would not only improve the readability of the code but also reduce useless code bloat resulting from using function calls like JS_(Get|Set)Property for what is effectively a field access. Another source of the bloat is using OBJ_GET_SLOT(cx, obj, JSSLOT_PRIVATE) that adds a locking/unlocking code for accessing this fixed slot.
Flags: wanted1.9.2?
Attached patch v1 (obsolete) — Splinter Review
This is what I am going to submit to the try server.
Assignee: general → igor
Attached patch v2 (obsolete) — Splinter Review
The patch adds 3 inline functions, js_GetPrivateOrNull, js_GetAssignedPrivate and js_SetPrivate that reads and store the private data in JSSLOT_PRIVATE. The difference between js_GetPrivateOrNull and js_GetAssignedPrivate is that the former returns NULL when it see a void value in the private slot (the same semantic as JS_GetPrivate) while the latter does not check for void assuming the private value was already set (the same semantic as the OBJ_GET_PRIVATE or STOBJ_GET_PRIVATE macros).

The patch also uses the explicit obj->fslots[JSSLOT_PRIVATE] when the code access non-private values in the slot like various native wrappers does for the corresponding primitive values.

Most of the changes are pretty much a mechanical renames. The exceptions are the following cases:

1. The property iterator code in jsapi.cpp assumes that it is enough to set the newborn to null to prevent the access to the private-less object from the GC-tracing code. This is wrong as the object can also be placed on the local root stack. In addition, a debug hook called from js_NewObject can run a GC also exposing private-less object to the GC trace hook.

2. The code from jsexn.cpp currently explicitly assigns the private slot to JSVAL_VOID when removing the private data. For consistency with the code that clear JSStackFrame private using js_SetPrivate(obj, NULL) the patch changed that assignment to the void into js_SetPrivate(obj, NULL).

3. The patch changed most of E4X code to call js_GetAssignedPrivate as a replacement for JS_GetPrivate calls. The only places that use js_GetPrivateOrNull are the GC trace and finalize hooks. This is possible as E4X always initializes the private data in the constructors.
Attachment #377934 - Attachment is obsolete: true
Attachment #378038 - Flags: review?(mrbkap)
Why not use inline methods in JSObject? Assuming this is for 1.9.2 and beyond, or if you need to cope with LiveConnect, use #ifdef __cplusplus.

(In reply to comment #3)
> Why not use inline methods in JSObject?

That is a good idea. I will update the patch with it.
Attachment #378038 - Attachment is obsolete: true
Attachment #378038 - Flags: review?(mrbkap)
Attached patch v3 (obsolete) — Splinter Review
The new version of the patch uses inline methods in JSObject to access the private data. Another important change is that I replaced in Boolen, Number and String classes JSCLASS_HAS_PRIVATE with JSCLASS_HAS_RESERVED_SLOTS. This change makes JSCLASS_HAS_PRIVATE to mean that the access to the private slot should be done only using (get|set)Private. It also fixed some debugging code to properly dump those 3 classes.

I have also considered to initialize the private slot for JSCLASS_HAS_PRIVATE classes with JSVAL_ZERO to avoid JSVAL_VOID checks when accessing the slot (it would also eliminate the need to have separated getAssignedSlot). But that would lead to an extra check during object construction. So I defer that for another bug.
Attachment #378562 - Flags: review?(mrbkap)
Various other bits of Mozilla code use the idiom that method names beginning with [Gg]et are fallible and that infallible methods omit it.  I suggest using assignedPrivate() for getAssignedPrivate() and clasp() for getClass() (at least reuses the canonical naming convention for JSClass*, too bad for the keyword conflict).

setPrivate() should have an alignment assertion on the passed-in pointer.

Why revert STOBJ_GET_CLASS to a macro?  You should be able to just replace the body with obj->getClass() or whatever, I'd think, until we can remove STOBJ_GET_CLASS entirely.

JSSLOT_* can be |const uint32_t ...| or somesuch as you did for JSSLOT_ITER_INDEX, I'd think.
Flags: wanted1.9.2? → wanted1.9.2+
For what it's worth the patch generally looks good to me. I'll defer to others who care more about naming to decide what the new functions should be called.

One quibble:

+    /*
+     * Get private value previously assigned with setPrivate.
+     */

Should be:

Get the private value previously assigned with setPrivate.
Attachment #378562 - Flags: superreview?(jwalden+bmo)
Attachment #378562 - Flags: review?(mrbkap)
Attachment #378562 - Flags: review+
Depends on: 503452
Attachment #378562 - Flags: superreview?(jwalden+bmo) → superreview+
Comment on attachment 378562 [details] [diff] [review]

>Index: tm/js/src/jsapi.cpp

> static void
> prop_iter_trace(JSTracer *trc, JSObject *obj)
> {
>-    jsval v;
>-    jsint i, n;
>-    JSScopeProperty *sprop;
>-    JSIdArray *ida;
>-    jsid id;
>-    v = obj->fslots[JSSLOT_PRIVATE];
>+    void *pdata = obj->getPrivate();
>+    if (!pdata)
>+        return;

This removes the !void assert -- why?

>@@ -4110,61 +4075,53 @@ JS_NewPropertyIterator(JSContext *cx, JS
>         return NULL;
>     if (OBJ_IS_NATIVE(obj)) {
>         /* Native case: start with the last property in obj's own scope. */
>         scope = OBJ_SCOPE(obj);
>         pdata = (scope->object == obj) ? scope->lastProp : NULL;
>         index = -1;
>     } else {
>-        JSTempValueRooter tvr;
>         /*
>          * Non-native case: enumerate a JSIdArray and keep it via private.
>          *
>          * Note: we have to make sure that we root obj around the call to
>          * JS_Enumerate to protect against multiple allocations under it.
>          */
>-        JS_PUSH_SINGLE_TEMP_ROOT(cx, OBJECT_TO_JSVAL(iterobj), &tvr);
>+        JSAutoTempValueRooter tvr(cx, iterobj);
>         ida = JS_Enumerate(cx, obj);
>-        JS_POP_TEMP_ROOT(cx, &tvr);
>         if (!ida)
>-            goto bad;
>+            return NULL;
>         pdata = ida;
>         index = ida->length;
>     }
>-    /* iterobj cannot escape to other threads here. */
>+    iterobj->setPrivate(pdata);
>+    iterobj->fslots[JSSLOT_ITER_INDEX] = INT_TO_JSVAL(index);
>     return iterobj;
>-    cx->weakRoots.newborn[GCX_OBJECT] = NULL;
>-    return NULL;
> }

Not nulling out the newborn weak root just means this garbage might live a little longer than needed, right?  Nothing more pernicious than that.  Just checking my understanding...

>Index: tm/js/src/jsfun.cpp

>@@ -1492,17 +1490,17 @@ static void
> DestroyLocalNames(JSContext *cx, JSFunction *fun);
> static void
> fun_trace(JSTracer *trc, JSObject *obj)
> {
>     JSFunction *fun;
>     /* A newborn function object may have a not yet initialized private slot. */
>-    fun = (JSFunction *) JS_GetPrivate(trc->context, obj);
>+    fun = (JSFunction *) obj->getPrivate();
>     if (!fun)
>         return;

This is...unfortunate and fragile.  Is there a reason we couldn't mock up a static JSFunction which traces away to nothing, rather than needing extra care to be taken when tracing functions?  That, or reorder how functions are created (looks harder going by js_NewFunction's signature, but I think not impossible), or fill in with the original JSFunction created when the class is initialized, stashed in the context or similar.  Same thing seems to apply to generators.  Followup to be sure if one of these is possible...

>     /*
>-     * We use JS_GetPrivate and not GET_FUNCTION_PRIVATE because during
>+     * We use getPrivate and not getAssignedPrivateE because during
>      * js_InitFunctionClass invocation the function is called before the


>Index: tm/js/src/jsobj.h

>+#define JSSLOT_PROTO        0
>+#define JSSLOT_PARENT       1
>+#define JSSLOT_PRIVATE      2

These can be const uint32_t.

>@@ -184,21 +190,54 @@ struct JSObjectMap {
>  * of jsvals for reserved and dynamic slots. If dslots is not null, dslots[-1]
>  * records the number of available slots.
>  */
> struct JSObject {
>     JSObjectMap *map;                       /* propery map, see jsscope.h */
>     jsuword     classword;                  /* classword, see above */
>     jsval       fslots[JS_INITIAL_NSLOTS];  /* small number of fixed slots */
>     jsval       *dslots;                    /* dynamically allocated slots */
>+#ifdef __cplusplus /* FIXME: bug 442399 removes this LiveConnect requirement. */

LiveConnect is dead!  This isn't needed any more!

>+    /*
>+     * Get private value previously assigned with setPrivate.
>+     */
>+    void *getAssignedPrivate() const {
>+        JS_ASSERT(getClass()->flags & JSCLASS_HAS_PRIVATE);
>+        jsval v = fslots[JSSLOT_PRIVATE];
>+        JS_ASSERT(JSVAL_IS_INT(v));
>+        return JSVAL_TO_PRIVATE(v);
>+    }

I still think avoiding get (when possible, getClass is an ugly exception) when possible is good, so this should be assignedPrivate().

>-STOBJ_GET_CLASS(const JSObject* obj)
>-    return (JSClass *) (obj->classword & ~JSSLOT_CLASS_MASK_BITS);
>+#define STOBJ_GET_CLASS(obj) ((obj)->getClass())

Why are you reverting this to a macro?  At least make the body be |return obj->getClass()|, ideal would be to kill this macro entirely, but given patch scope it's entirely reasonable (probably best) to leave it to later.
Attached patch v4Splinter Review
patch to land that keeps STOBJ_GET_CLASS as an inline function
Attachment #378562 - Attachment is obsolete: true
Attachment #392566 - Flags: review+
Whiteboard: fixed-in-tracemonkey
Depends on: 508545 - followup fix for typo in the landed patch that lead to bug 508545
Shouldn't bug 507573 block this bug?

(In reply to comment #12)
> Shouldn't bug 507573 block this bug?

No: this blocks bug 495061.
Blocks: 495061
Depends on: 509143
Depends on: 510449
Closed: 12 years ago
Resolution: --- → FIXED
Blocks: 513190
You need to log in before you can comment on or make changes to this bug.