566836 - Eliminate JSObjectOps::dropProperty

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Igor Bukanov]

In most places where the code uses JSObjectOps::lokupProperty the code also does the isNative() check. Thus the idea is to eliminate dropProperty calls as it has non-empty implementation only for native objects.

Comment 1

[Igor Bukanov]

Attachment: v1

The patch replaces JSObjectOps::dropProperty with a check for native objects and JS_UNLOCK_OBJ. When the code already does the native checks after lookupProperty calls the patch replaces the drop calls with explicit JS_UNLOCK_OBJ. In code without the native checks the patch keeps JSObject::dropObject now implemented as an inline helper. This minimizes the number of lines to change.

The patch also eliminates JSProperty argument from JSObjectOps::(get|set)Property. For non-native objects it is never necessary but for native one it is faster to just read the attributes from JSScopeProperty especially since in most of the cases the code already contains isNative checks. 

Both these changes makes code easy to verify in case of possible class mutations between lookup and (get|set)Attributes, dropProperty with non-native classes becoming native ones. Now this safe since there is no dependancy on JSProperty.

Comment 2

[Igor Bukanov]

Attachment: diff -b of v1

Comment 3

[Igor Bukanov]

Attachment: v2

This new version fixes redundant getAttribute call for proxies in GetPropertyAttributesById.

Comment 4

[Igor Bukanov]

Attachment: diff -b of v2

Comment 5

[Igor Bukanov]

Attachment: v3

the new version fixes the return of uninitialized ok

Comment 6

[Igor Bukanov]

Attachment: v4

some extra cleanups

Comment 7

[Igor Bukanov]

Attachment: diff -b version of v4

Comment 8

[Brendan Eich [:brendan]]

Can we change lookupProperty to hasProperty and get rid of JSProperty from the JSObjectOps API altogether? Otherwise there's still a native-only feature, which if it's used by non-natives would beg the "who drops, and how?" questions.

/be

Comment 9

[Igor Bukanov]

(In reply to comment #8)
> Can we change lookupProperty to hasProperty and get rid of JSProperty from the
> JSObjectOps API altogether?

I do not want to do it in this bug since I am not sure what would be the right approach here. In many cases the callers of lookupProperty really want sprop for native objects together with the corresponding obj2. So one cannot just replace the method with hasProperty.

Comment 10

[Igor Bukanov]

(In reply to comment #9)
> I do not want to do it in this bug since I am not sure what would be the right
> approach here.

I suspect something like getOwnPropertyDescriptor would do the job, but that for the bug 566143.

Comment 11

[Brendan Eich [:brendan]]

(In reply to comment #9)
> (In reply to comment #8)
> > Can we change lookupProperty to hasProperty and get rid of JSProperty from the
> > JSObjectOps API altogether?
> 
> I do not want to do it in this bug since I am not sure what would be the right
> approach here. In many cases the callers of lookupProperty really want sprop
> for native objects together with the corresponding obj2. So one cannot just
> replace the method with hasProperty.

Yet you said the calls check isNative, so they therefore could use any of the underlying js_LookupProperty* functions, specifically the fundamental function js_LookupPropertyWithFlags.

Ok to split this work across bugs, but let's get it all done. Having lookup but not drop is incoherent.

/be

Comment 12

[Igor Bukanov]

(In reply to comment #11)
> Yet you said the calls check isNative, so they therefore could use any of the
> underlying js_LookupProperty* functions, specifically the fundamental function
> js_LookupPropertyWithFlags.

The lookup on a native object can return a non-native prototype. So a code cannot simply call isNative initially and hope that the result of js_LookupPropertyWithFlags stays native.

Comment 13

[Jason Orendorff [:jorendorff]]

Reviewing this today.

Comment 14

[Jason Orendorff [:jorendorff]]

Comment on attachment 447998 [details] [diff] [review]
v4

In jsapi.cpp, in SetPropertyAttributes:
>-    if (!prop || obj != obj2) {
>-        *foundp = JS_FALSE;
>-        if (prop)
>-            obj2->dropProperty(cx, prop);
>-        return JS_TRUE;
>+    if (!prop) {
>+        *foundp = false;
>+        return true;
>     }
> 
>-    *foundp = JS_TRUE;
>-    ok = obj->setAttributes(cx, ATOM_TO_JSID(atom), prop, &attrs);
>-    obj->dropProperty(cx, prop);
>-    return ok;
>+    bool found = (obj == obj2);
>+    if (obj2->isNative()) {
>+        JSScopeProperty *sprop = (JSScopeProperty *) prop;
>+        if (found) {
>+            sprop = js_ChangeNativePropertyAttrs(cx, obj, sprop, attrs, 0,
>+                                                 sprop->getter(),
>+                                                 sprop->setter());
>+        }
>+        JS_UNLOCK_OBJ(cx, obj2);
>+        if (!sprop)
>+            return false;
>+    } else {
>+        if (found && !obj->setAttributes(cx, ATOM_TO_JSID(atom), &attrs))
>+            return false;
>+    }
>+    *foundp = found;
>+    return true;
> }

Here I found the original code easier to follow, it's shorter, and I
think it would work without changes. Can we leave it?

In jsobj.cpp, js_FindClassObject:
>     if (prop)  {
>         if (pobj->isNative()) {
>             sprop = (JSScopeProperty *) prop;
>             if (SPROP_HAS_VALID_SLOT(sprop, pobj->scope())) {
>                 v = pobj->lockedGetSlot(sprop->slot);
>                 if (JSVAL_IS_PRIMITIVE(v))
>                     v = JSVAL_VOID;
>             }
>+            JS_UNLOCK_OBJ(cx, pobj);
>         }
>-        pobj->dropProperty(cx, prop);
>     }

After this change, the blocks can be combined:

   if (prop && pobj->isNative()) ...

In jsarray.cpp, array_getProperty:
>         if (js_LookupPropertyWithFlags(cx, proto, id, cx->resolveFlags,
>                                        &obj2, &prop) < 0)
>             return JS_FALSE;
> 
>         if (prop) {
>             if (obj2->isNative()) {
>                 sprop = (JSScopeProperty *) prop;
>                 if (!js_NativeGet(cx, obj, obj2, sprop, JSGET_METHOD_BARRIER, vp))
>                     return JS_FALSE;
>+                JS_UNLOCK_OBJ(cx, obj2);
>             }
>-            obj2->dropProperty(cx, prop);
>         }

Same here.

In jsops.cpp, CASE(JSOP_CALLNAME):
>     /* Take the slow path if prop was not found in a native object. */
>     if (!obj->isNative() || !obj2->isNative()) {
>-        obj2->dropProperty(cx, prop);
>         if (!obj->getProperty(cx, id, &rval))
>             goto error;

It's a rare corner case, but it is possible via the API to get a
non-native obj and a native obj2 here, right? Let's leave the
dropProperty in or now. Eventually we do want to unsupport this.

Comment 15

[Igor Bukanov]

(In reply to comment #14)
> Here I found the original code easier to follow, it's shorter, and I
> think it would work without changes.

It is shorter without the patch because SetPropertyAttributes uses js_SetAttributes taking JSProperty * parameter. 

> Can we leave it?

Almost - I will add SetNativeAttributes that preserves the current implementation of js_SetAttributes and use the new function here.


> In jsops.cpp, CASE(JSOP_CALLNAME):
> >     /* Take the slow path if prop was not found in a native object. */
> >     if (!obj->isNative() || !obj2->isNative()) {
> >-        obj2->dropProperty(cx, prop);

..
> It's a rare corner case, but it is possible via the API to get a
> non-native obj and a native obj2 here, right?

This is very good catch, thanks.

Comment 16

[Igor Bukanov]

Attachment: v5

The new version fixes the nits and adds js_SetNativeAttributes to SetPropertyAttributesById from jsapi.cpp would stay simple. Here the relevant changes from the patch:

 static JSBool
 SetPropertyAttributesById(JSContext *cx, JSObject *obj, jsid id, uintN attrs, JSBool *foundp)
 {
     JSObject *obj2;
     JSProperty *prop;
-    JSBool ok;
 
     if (!LookupPropertyById(cx, obj, id, JSRESOLVE_QUALIFIED, &obj2, &prop))
-        return JS_FALSE;
+        return false;
     if (!prop || obj != obj2) {
-        *foundp = JS_FALSE;
+        *foundp = false;
         if (prop)
             obj2->dropProperty(cx, prop);
-        return JS_TRUE;
+        return true;
     }
-
-    *foundp = JS_TRUE;
-    ok = obj->setAttributes(cx, id, prop, &attrs);
-    obj->dropProperty(cx, prop);
+    JSBool ok = obj->isNative()
+                ? js_SetNativeAttributes(cx, obj, (JSScopeProperty *) prop, attrs)
+                : obj->setAttributes(cx, id, &attrs);
+    if (ok)
+        *foundp = true;
     return ok;
 }

...
+/*
+ * Change attributes for the given native property. The caller must ensure
+ * that obj is locked and this function always unlocks obj on return.
+ */
+js_SetNativeAttributes(JSContext *cx, JSObject *obj, JSScopeProperty *sprop,
+                       uintN attrs);


 JSBool
-js_SetAttributes(JSContext *cx, JSObject *obj, jsid id, JSProperty *prop,
-                 uintN *attrsp)
+js_SetNativeAttributes(JSContext *cx, JSObject *obj, JSScopeProperty *sprop,
+                       uintN attrs)
 {
-    JSBool noprop, ok;
-    JSScopeProperty *sprop;
-
-    noprop = !prop;
-    if (noprop) {
-        if (!js_LookupProperty(cx, obj, id, &obj, &prop))
-            return JS_FALSE;
-        if (!prop)
-            return JS_TRUE;
-        if (!obj->isNative()) {
-            ok = obj->setAttributes(cx, id, prop, attrsp);
-            obj->dropProperty(cx, prop);
-            return ok;
-        }
-    }
-    sprop = (JSScopeProperty *)prop;
-    sprop = js_ChangeNativePropertyAttrs(cx, obj, sprop, *attrsp, 0,
+    JS_ASSERT(obj->isNative());
+    sprop = js_ChangeNativePropertyAttrs(cx, obj, sprop, attrs, 0,
                                          sprop->getter(), sprop->setter());
-    if (noprop)
-        obj->dropProperty(cx, prop);
+    JS_UNLOCK_OBJ(cx, obj);
     return (sprop != NULL);
 }
 
 JSBool
+js_SetAttributes(JSContext *cx, JSObject *obj, jsid id, uintN *attrsp)
+{
+    JSProperty *prop;
+    if (!js_LookupProperty(cx, obj, id, &obj, &prop))
+        return false;
+    if (!prop)
+        return true;
+    return obj->isNative()
+           ? js_SetNativeAttributes(cx, obj, (JSScopeProperty *) prop, *attrsp)
+           : obj->setAttributes(cx, id, attrsp);
+}
+

Comment 17

[Andreas Gal :gal]

Stealing with jorendorff's consent. Once we remove locking we should collapse this back into one code bath and remove the js_*Native* bypass, especially where performance doesn't matter (attributes almost never change). It doesn't really hurt right now either. r=me

Comment 18

[Nicholas Nethercote [inactive]]

(In reply to comment #17)
> Once we remove locking we should collapse this back into one code bath

LOL, is that like a bloodbath?

Comment 19

[Igor Bukanov]

http://hg.mozilla.org/tracemonkey/rev/ec31975e7669

Comment 20

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/ec31975e7669