Status

()

enhancement
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: igor, Assigned: igor)

Tracking

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.9.2 +

Firefox Tracking Flags

(status1.9.2 beta1-fixed)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 3 obsolete attachments)

Assignee

Description

10 years ago
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);

    JSVAL_TO_PRIVATE(OBJ_GET_SLOT(cx, obj, JSSLOT_PRIVATE))

    JSVAL_TO_PRIVATE(obj->fslots[JSSLOT_PRIVATE])

Writing:

    STOBJ_SET_SLOT(obj, JSSLOT_PRIVATE, JSVAL_TO_PRIVATE(data)); 

    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?
Assignee

Comment 1

10 years ago
Posted patch v1 (obsolete) — Splinter Review
This is what I am going to submit to the try server.
Assignee: general → igor
Assignee

Comment 2

10 years ago
Posted 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.

/be
Assignee

Comment 4

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

That is a good idea. I will update the patch with it.
Assignee

Updated

10 years ago
Attachment #378038 - Attachment is obsolete: true
Attachment #378038 - Flags: review?(mrbkap)
Assignee

Comment 5

10 years ago
Posted 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.

Updated

10 years ago
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+

Updated

10 years ago
Depends on: 503452

Updated

10 years ago
Attachment #378562 - Flags: superreview?(jwalden+bmo) → superreview+
Comment on attachment 378562 [details] [diff] [review]
v3

>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];
>-    JS_ASSERT(!JSVAL_IS_VOID(v));
>+    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. */
>-    STOBJ_SET_SLOT(iterobj, JSSLOT_PRIVATE, PRIVATE_TO_JSVAL(pdata));
>-    STOBJ_SET_SLOT(iterobj, JSSLOT_ITER_INDEX, INT_TO_JSVAL(index));
>+    iterobj->setPrivate(pdata);
>+    iterobj->fslots[JSSLOT_ITER_INDEX] = INT_TO_JSVAL(index);
>     return iterobj;
>-
>-bad:
>-    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

tpyo



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

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

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().




>-JS_ALWAYS_INLINE JSClass*
>-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.
Assignee

Comment 9

10 years ago
Posted 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+
Assignee

Comment 10

10 years ago
http://hg.mozilla.org/tracemonkey/rev/c532c2a2448d
Whiteboard: fixed-in-tracemonkey

Updated

10 years ago
Depends on: 508545
Assignee

Comment 11

10 years ago
http://hg.mozilla.org/tracemonkey/rev/ffae7f39e607 - followup fix for typo in the landed patch that lead to bug 508545
Shouldn't bug 507573 block this bug?

/be
Assignee

Comment 13

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

No: this blocks bug 495061.
Blocks: 495061

Updated

10 years ago
Depends on: 509143

Updated

10 years ago
Depends on: 510449

Comment 14

10 years ago
http://hg.mozilla.org/mozilla-central/rev/c532c2a2448d
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Assignee

Updated

10 years ago
Blocks: 513190
You need to log in before you can comment on or make changes to this bug.