413045 - JS_Get(Prototype|Private|Parent|Class) API performance improvements.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Johnny Stenback (:jst)]

Attachment: Make stuff faster.

There's a significant difference in the performance characteristics of JS_GetPrivate() vs. JS_GetClass() (~5x) even though both of them basically do the same thing, access a slot in obj->fslots. This comes down to JS_GetClass() using GC_AWARE_GET_CLASS, which is defined to be STOBJ_GET_CLASS, which does a direct access to obj->fslots[JSSLOT_CLASS], whereas JS_GetPrivate() uses GC_AWARE_GET_SLOT() which does a CX_THREAD_IS_RUNNING_GC() check before accessing the property.

See the attached patch for a change that fixes that, and a couple of other random similar things.

Comment 1

[Brendan Eich [:brendan]]

Comment on attachment 297900 [details] [diff] [review]
Make stuff faster.

If you want to do this (I'm in favor) please go all the way and remove GC_AWARE_GET_* (also for bonus points fix the JSCLASS_SLOT typo in a jsobj.h comment -- should be JSSLOT_CLASS of course).

If you wanna minimize change, keep GC_AWARE_GET_* macros but make them call STOBJ_GET_*.

/be

Comment 2

[Brendan Eich [:brendan]]

I am happy with either minimal or maximal change -- if minimal, I'll take a followup bug.

/be

Comment 3

[Johnny Stenback (:jst)]

Attachment: No more GC_AWARE_* macros.

Brendan, this gets rid of all the GC_AWARE_* macros and fixes the typo. There's a bunch of places in the JS engine that still uses JS_GetPrivate(), but I don't want to change them all to just use STOBJ_GET_PRIVATE as there's a JSVAL_IS_INT() check in JS_GetPrivate() that's needed to maintain the current functionality, and I don't know if we want to make the macro do that (or if we even can in a threadsave way). Let me know what you think... Oh, and do we need to worry about the fact that this change lets obj->fslots mutate on a locked object on a context that doesn't hold the lock?

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 298536 [details] [diff] [review]
No more GC_AWARE_* macros.

>diff --git a/js/src/jsapi.c b/js/src/jsapi.c
>index 2dd9bff..2d075cd 100644
>--- a/js/src/jsapi.c
>+++ b/js/src/jsapi.c
>@@ -2767,13 +2767,13 @@ bad:
> JS_PUBLIC_API(JSClass *)
> JS_GetClass(JSContext *cx, JSObject *obj)
> {
>-    return GC_AWARE_GET_CLASS(cx, obj);
>+    return STOBJ_GET_CLASS(obj);

Sorry, should have noted this: OBJ_GET_CLASS(cx,obj) is STOBJ_GET_CLASS(obj), same for other fixed slots. We don't want the callers to use STOBJ_ if they are not using a known single-threaded object -- that violates the slender abstraction (which we might fatten up over time, who knows?). So I would use OBJ_GET_* here and elsewhere. I'll make detailed comments to be sure; also asking Igor for second review (no sr? for js/src).

>@@ -2809,7 +2809,7 @@ JS_GetPrivate(JSContext *cx, JSObject *obj)
>     jsval v;
> 
>     JS_ASSERT(OBJ_GET_CLASS(cx, obj)->flags & JSCLASS_HAS_PRIVATE);
>-    v = GC_AWARE_GET_SLOT(cx, obj, JSSLOT_PRIVATE);
>+    v = obj->fslots[JSSLOT_PRIVATE];
>     if (!JSVAL_IS_INT(v))
>         return NULL;
>     return JSVAL_TO_PRIVATE(v);

Note this obj->fslots[JSSLOT_PRIVATE] usage for next comment.

>@@ -2819,7 +2819,7 @@ JS_PUBLIC_API(JSBool)
> JS_SetPrivate(JSContext *cx, JSObject *obj, void *data)
> {
>     JS_ASSERT(OBJ_GET_CLASS(cx, obj)->flags & JSCLASS_HAS_PRIVATE);
>-    OBJ_SET_SLOT(cx, obj, JSSLOT_PRIVATE, PRIVATE_TO_JSVAL(data));
>+    STOBJ_SET_SLOT(obj, JSSLOT_PRIVATE, PRIVATE_TO_JSVAL(data));

For symmetry how about obj->fslots[JSSLOT_PRIVATE] = PRIVAT_TO_JSVAL(data) here.

>@@ -2838,7 +2838,7 @@ JS_GetPrototype(JSContext *cx, JSObject *obj)
>@@ -2850,7 +2850,7 @@ JS_SetPrototype(JSContext *cx, JSObject *obj, JSObject *proto)
>@@ -2859,7 +2859,7 @@ JS_GetParent(JSContext *cx, JSObject *obj)
>@@ -2871,7 +2871,7 @@ JS_SetParent(JSContext *cx, JSObject *obj, JSObject *parent)

These should be OBJ_{GET,SET}_{PROTO,PARENT} per first comment in this review.

>@@ -3780,7 +3780,7 @@ JS_ClearScope(JSContext *cx, JSObject *obj)
>         obj->map->ops->clear(cx, obj);
> 
>     /* Clear cached class objects on the global object. */
>-    if (JS_GET_CLASS(cx, obj)->flags & JSCLASS_IS_GLOBAL) {
>+    if (STOBJ_GET_CLASS(obj)->flags & JSCLASS_IS_GLOBAL) {

OBJ_GET_CLASS(cx, obj).

>@@ -3868,7 +3868,7 @@ prop_iter_finalize(JSContext *cx, JSObject *obj)
>     jsint i;
>     JSIdArray *ida;
> 
>-    v = GC_AWARE_GET_SLOT(cx, obj, JSSLOT_ITER_INDEX);
>+    v = OBJ_GET_SLOT(cx, obj, JSSLOT_ITER_INDEX);

Assertion via #if ensures that JSSLOT_ITER_INDEX is in obj->fslots[] so here, we want obj->fslots[...] I claim.

When the abstraction is not buying us anything, and we have static checks putting indexes in bounds, don't abstract ;-).

>@@ -3890,7 +3890,7 @@ prop_iter_trace(JSTracer *trc, JSObject *obj)
>     JSIdArray *ida;
>     jsid id;
> 
>-    v = GC_AWARE_GET_SLOT(trc->context, obj, JSSLOT_PRIVATE);
>+    v = obj->fslots[JSSLOT_PRIVATE];

Righteous.

>     JS_ASSERT(!JSVAL_IS_VOID(v));
> 
>     i = JSVAL_TO_INT(OBJ_GET_SLOT(trc->context, obj, JSSLOT_ITER_INDEX));

obj->fslots[JSSLOT_ITER_INDEX].

>+++ b/js/src/jsinterp.c
>@@ -406,8 +406,8 @@ js_GetScopeChain(JSContext *cx, JSStackFrame *fp)
>      * if this frame is a call frame.
>      */
>     if (fp->fun && !fp->callobj) {
>-        JS_ASSERT(OBJ_GET_CLASS(cx, fp->scopeChain) != &js_BlockClass ||
>-                  JS_GetPrivate(cx, fp->scopeChain) != fp);
>+        JS_ASSERT(STOBJ_GET_CLASS(fp->scopeChain) != &js_BlockClass ||
>+                  STOBJ_GET_PRIVATE(fp->scopeChain) != fp);
>         if (!js_GetCallObject(cx, fp, fp->scopeChain))
>             return NULL;
>     }

These should be OBJ_GET_CLASS (as before) and OBJ_GET_PRIVATE.

>@@ -479,7 +479,7 @@ PutBlockObjects(JSContext *cx, JSStackFrame *fp)
>     ok = JS_TRUE;
>     for (obj = fp->scopeChain; obj; obj = OBJ_GET_PARENT(cx, obj)) {
>         if (OBJ_GET_CLASS(cx, obj) == &js_BlockClass) {
>-            if (JS_GetPrivate(cx, obj) != fp)
>+            if (STOBJ_GET_PRIVATE(obj) != fp)
>                 break;
>             ok &= js_PutBlockObject(cx, obj);
>         }

Ditto.

>@@ -1004,7 +1004,7 @@ have_fun:
>                         native,
>                         JSVAL_IS_OBJECT(vp[1])
>                         ? ((OBJ_GET_CLASS(cx, frame.thisp) == &js_FunctionClass)
>-                           ? JS_GetFunctionName(JS_GetPrivate(cx, frame.thisp))
>+                           ? JS_GetFunctionName(STOBJ_GET_PRIVATE(frame.thisp))
>                            : OBJ_GET_CLASS(cx, frame.thisp)->name)
>                         : JSVAL_IS_BOOLEAN(vp[1])

Ditto, sigh -- could go with obj->fslots all over, you say? Maybe not just yet.

>@@ -2183,7 +2183,7 @@ interrupt:
>@@ -5909,7 +5909,7 @@ out:

Ditto.

>+++ b/js/src/jslock.c
>@@ -609,7 +609,7 @@ js_GetSlotThreadSafe(JSContext *cx, JSObject *obj, uint32 slot)
>     JS_ASSERT(slot < obj->map->freeslot);
> 
>     /*
>-     * Avoid locking if called from the GC (see GC_AWARE_GET_SLOT in jsobj.h).
>+     * Avoid locking if called from the GC.
>      * Also avoid locking an object owning a sealed scope.  If neither of those
>      * special cases applies, try to claim scope's flyweight lock from whatever
>      * context may have had it in an earlier request.

Reformat with vim Q3j when at "Avoid ...".

>@@ -702,7 +702,7 @@ js_SetSlotThreadSafe(JSContext *cx, JSObject *obj, uint32 slot, jsval v)
>     JS_ASSERT(slot < obj->map->freeslot);
> 
>     /*
>-     * Avoid locking if called from the GC (see GC_AWARE_GET_SLOT in jsobj.h).
>+     * Avoid locking if called from the GC.
>      * Also avoid locking an object owning a sealed scope.  If neither of those
>      * special cases applies, try to claim scope's flyweight lock from whatever
>      * context may have had it in an earlier request.

Ditto, sorry for nits.

>+++ b/js/src/jsobj.c
>@@ -2874,7 +2874,7 @@ js_FinalizeObject(JSContext *cx, JSObject *obj)
>     }
> 
>     /* Finalize obj first, in case it needs map and slots. */
>-    GC_AWARE_GET_CLASS(cx, obj)->finalize(cx, obj);
>+    STOBJ_GET_CLASS(obj)->finalize(cx, obj);

This is a good use of STOBJ_.

/be

Comment 5

[Johnny Stenback (:jst)]

Attachment: Updated fix.

This should address all of brendan's comments.

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 298592 [details] [diff] [review]
Updated fix.

Great, thanks. Igor, if you are ok with this please stamp -- sorry if it seems trivial, I wanted to get a second opinion and you did the STOBJ_* work.

/be

Comment 7

[Igor Bukanov]

Comment on attachment 298592 [details] [diff] [review]
Updated fix.

>diff --git a/js/src/jsapi.c b/js/src/jsapi.c
>index 2dd9bff..423b1e7 100644
>--- a/js/src/jsapi.c
>+++ b/js/src/jsapi.c
>@@ -2767,7 +2767,7 @@ bad:
> JS_PUBLIC_API(JSClass *)
> JS_GetClass(JSContext *cx, JSObject *obj)
> {
>-    return GC_AWARE_GET_CLASS(cx, obj);
>+    return OBJ_GET_CLASS(cx, obj);

I would prefer to drop the unused cx argument from OBJ_GET_CLASS. This would be on the same scale as other changes to use explicit obj->fslots[]. It would also emphasis that macro the never fails and does not require any synchronization. 

The same is applicable to OBJ_(GET|SET)_(PROTO|PARENT). In general it is the access to JSObject.dslots that must be carefully protected, not the permanent JSObject.fslots. But this can be done in another bug.

Comment 8

[Johnny Stenback (:jst)]

Fix checked in.

Bug 413730 filed on removing the cx argument from the relevant OBJ_GET_... macros.