bug 462734 - fixing JSOP_BINDNAME caching issues. r=brendan
authorIgor Bukanov <igor@mir2.org>
Sat, 11 Apr 2009 10:11:06 +0200
changeset 24908 dd259477ab3b9a58c76dee035ab51208cf028e07
parent 24907 4948d39190a927d3a819c9432273c97e7f1c950d
child 24909 ea7424828fb1e3c00512710c6bda836544c2a7e8
push id1267
push userrsayre@mozilla.com
push dateSun, 19 Apr 2009 02:47:24 +0000
reviewersbrendan
bugs462734
milestone1.9.1b4pre
bug 462734 - fixing JSOP_BINDNAME caching issues. r=brendan
js/src/jsinterp.cpp
js/src/jsobj.cpp
js/src/jsobj.h
--- a/js/src/jsinterp.cpp
+++ b/js/src/jsinterp.cpp
@@ -3635,30 +3635,48 @@ js_Interpret(JSContext *cx)
             regs.sp -= 3;
           END_CASE(JSOP_ENUMCONSTELEM)
 #endif
 
           BEGIN_CASE(JSOP_BINDNAME)
             do {
                 JSPropCacheEntry *entry;
 
+                /*
+                 * We can skip the property lookup for the global object. If
+                 * the property does not exist anywhere on the scope chain,
+                 * JSOP_SETNAME adds the property to the global.
+                 *
+                 * As a consequence of this optimization for the global object
+                 * we run its JSRESOLVE_ASSIGNING-tolerant resolve hooks only
+                 * in JSOP_SETNAME, after the interpreter evaluates the right-
+                 * hand-side of the assignment, and not here.
+                 *
+                 * This should be transparent to the hooks because the script,
+                 * instead of name = rhs, could have used global.name = rhs
+                 * given a global object reference, which also calls the hooks
+                 * only after evaluating the rhs. We desire such resolve hook
+                 * equivalence between the two forms.
+                 */
                 obj = fp->scopeChain;
+                if (!OBJ_GET_PARENT(cx, obj))
+                    break;
                 if (JS_LIKELY(OBJ_IS_NATIVE(obj))) {
                     PROPERTY_CACHE_TEST(cx, regs.pc, obj, obj2, entry, atom);
                     if (!atom) {
                         ASSERT_VALID_PROPERTY_CACHE_HIT(0, obj, obj2, entry);
                         JS_UNLOCK_OBJ(cx, obj2);
                         break;
                     }
                 } else {
                     entry = NULL;
                     LOAD_ATOM(0);
                 }
                 id = ATOM_TO_JSID(atom);
-                obj = js_FindIdentifierBase(cx, id, entry);
+                obj = js_FindIdentifierBase(cx, fp->scopeChain, id, entry);
                 if (!obj)
                     goto error;
             } while (0);
             PUSH_OPND(OBJECT_TO_JSVAL(obj));
           END_CASE(JSOP_BINDNAME)
 
           BEGIN_CASE(JSOP_IMACOP)
             JS_ASSERT(JS_UPTRDIFF(fp->imacpc, script->code) < script->length);
@@ -4754,18 +4772,20 @@ js_Interpret(JSContext *cx)
                         }
                     }
                 }
 
                 if (!atom)
                     LOAD_ATOM(0);
                 id = ATOM_TO_JSID(atom);
                 if (entry) {
-                    if (!js_SetPropertyHelper(cx, obj, id, &rval, &entry))
+                    if (!js_SetPropertyHelper(cx, obj, id, op == JSOP_SETNAME,
+                                              &rval, &entry)) {
                         goto error;
+                    }
 #ifdef JS_TRACER
                     if (entry)
                         TRACE_1(SetPropMiss, entry);
 #endif
                 } else {
                     if (!OBJ_SET_PROPERTY(cx, obj, id, &rval))
                         goto error;
                 }
@@ -6409,17 +6429,17 @@ js_Interpret(JSContext *cx)
                 id = ATOM_TO_JSID(atom);
 
                 /* Set the property named by obj[id] to rval. */
                 if (!js_CheckRedeclaration(cx, obj, id, JSPROP_INITIALIZER,
                                            NULL, NULL)) {
                     goto error;
                 }
                 if (JS_UNLIKELY(atom == cx->runtime->atomState.protoAtom)
-                    ? !js_SetPropertyHelper(cx, obj, id, &rval, &entry)
+                    ? !js_SetPropertyHelper(cx, obj, id, false, &rval, &entry)
                     : !js_DefineNativeProperty(cx, obj, id, rval, NULL, NULL,
                                                JSPROP_ENUMERATE, 0, 0, NULL,
                                                &entry)) {
                     goto error;
                 }
 #ifdef JS_TRACER
                 if (entry)
                     TRACE_1(SetPropMiss, entry);
--- a/js/src/jsobj.cpp
+++ b/js/src/jsobj.cpp
@@ -4065,55 +4065,78 @@ js_FindPropertyHelper(JSContext *cx, jsi
 JS_FRIEND_API(JSBool)
 js_FindProperty(JSContext *cx, jsid id, JSObject **objp, JSObject **pobjp,
                 JSProperty **propp)
 {
     return js_FindPropertyHelper(cx, id, objp, pobjp, propp, NULL) >= 0;
 }
 
 JS_REQUIRES_STACK JSObject *
-js_FindIdentifierBase(JSContext *cx, jsid id, JSPropCacheEntry *entry)
+js_FindIdentifierBase(JSContext *cx, JSObject *scopeChain, jsid id,
+                      JSPropCacheEntry *entry)
 {
-    JSObject *obj, *pobj;
-    JSProperty *prop;
-
     /*
-     * Look for id's property along the "with" statement chain and the
-     * statically-linked scope chain.
+     * This function should not be called for a global object or from the
+     * trace and should have a valid cache entry for native scopeChain.
      */
-    if (js_FindPropertyHelper(cx, id, &obj, &pobj, &prop, &entry) < 0)
-        return NULL;
-    if (prop) {
-        OBJ_DROP_PROPERTY(cx, pobj, prop);
-        return obj;
-    }
+    JSObject *parent = OBJ_GET_PARENT(cx, scopeChain);
+    JS_ASSERT(parent);
+    JS_ASSERT(!JS_ON_TRACE(cx));
+    JS_ASSERT_IF(OBJ_IS_NATIVE(scopeChain), entry);
 
     /*
-     * Use the top-level scope from the scope chain, which won't end in the
-     * same scope as cx->globalObject for cross-context function calls.
-     */
-    JS_ASSERT(obj);
-
-    /*
-     * Property not found.  Give a strict warning if binding an undeclared
-     * top-level variable.
+     * Optimize and cache only for classes that do not have resolve hooks and
+     * where the prototype is used only to implement a shared scope, bug 462734
+     * and bug 487039.
      */
-    if (JS_HAS_STRICT_OPTION(cx)) {
-        JSString *str = JSVAL_TO_STRING(ID_TO_VALUE(id));
-        const char *bytes = js_GetStringBytes(cx, str);
-
-        if (!bytes ||
-            !JS_ReportErrorFlagsAndNumber(cx,
-                                          JSREPORT_WARNING | JSREPORT_STRICT,
-                                          js_GetErrorMessage, NULL,
-                                          JSMSG_UNDECLARED_VAR, bytes)) {
+    JSObject *obj = scopeChain;
+    for (int scopeIndex = 0; ; scopeIndex++) {
+        JSClass *clasp = OBJ_GET_CLASS(cx, obj);
+        if (clasp != &js_CallClass && clasp != &js_BlockClass)
+            break;
+
+        JSObject *pobj;
+        JSProperty *prop;
+        int protoIndex = js_LookupPropertyWithFlags(cx, obj, id, 0,
+                                                    &pobj, &prop);
+        if (protoIndex < 0)
             return NULL;
+        if (prop) {
+            JS_ASSERT(OBJ_IS_NATIVE(pobj));
+            JS_ASSERT(OBJ_GET_CLASS(cx, pobj) == clasp);
+            js_FillPropertyCache(cx, scopeChain, OBJ_SHAPE(scopeChain),
+                                 scopeIndex, protoIndex, pobj,
+                                 (JSScopeProperty *) prop, &entry);
+            JS_UNLOCK_OBJ(cx, pobj);
+            return obj;
+        }
+
+        obj = parent;
+        parent = OBJ_GET_PARENT(cx, parent);
+        if (!parent) {
+            /*
+             * Here obj is the global one and we can skip any checks for it,
+             * see comments in the JSOP_BINDNAME case of js_Interpret.
+             */
+            return obj;
         }
     }
 
+    do {
+        JSObject *pobj;
+        JSProperty *prop;
+        if (!OBJ_LOOKUP_PROPERTY(cx, obj, id, &pobj, &prop))
+            return NULL;
+        if (prop) {
+            OBJ_DROP_PROPERTY(cx, pobj, prop);
+            break;
+        }
+        obj = parent;
+        parent = OBJ_GET_PARENT(cx, parent);
+    } while (parent);
     return obj;
 }
 
 JSBool
 js_NativeGet(JSContext *cx, JSObject *obj, JSObject *pobj,
              JSScopeProperty *sprop, jsval *vp)
 {
     js_LeaveTraceIfGlobalObject(cx, pobj);
@@ -4332,18 +4355,18 @@ js_GetMethod(JSContext *cx, JSObject *ob
 #if JS_HAS_XML_SUPPORT
     if (OBJECT_IS_XML(cx, obj))
         return js_GetXMLMethod(cx, obj, id, vp);
 #endif
     return OBJ_GET_PROPERTY(cx, obj, id, vp);
 }
 
 JSBool
-js_SetPropertyHelper(JSContext *cx, JSObject *obj, jsid id, jsval *vp,
-                     JSPropCacheEntry **entryp)
+js_SetPropertyHelper(JSContext *cx, JSObject *obj, jsid id,
+                     JSBool unqualified, jsval *vp, JSPropCacheEntry **entryp)
 {
     uint32 shape;
     int protoIndex;
     JSObject *pobj;
     JSProperty *prop;
     JSScopeProperty *sprop;
     JSScope *scope;
     uintN attrs, flags;
@@ -4363,20 +4386,38 @@ js_SetPropertyHelper(JSContext *cx, JSOb
         goto read_only_error;
     }
 
     shape = OBJ_SHAPE(obj);
     protoIndex = js_LookupPropertyWithFlags(cx, obj, id, cx->resolveFlags,
                                             &pobj, &prop);
     if (protoIndex < 0)
         return JS_FALSE;
-
-    if (prop && !OBJ_IS_NATIVE(pobj)) {
-        OBJ_DROP_PROPERTY(cx, pobj, prop);
-        prop = NULL;
+    if (prop) {
+        if (!OBJ_IS_NATIVE(pobj)) {
+            OBJ_DROP_PROPERTY(cx, pobj, prop);
+            prop = NULL;
+        }
+    } else {
+        /* We should never add properties to lexical blocks.  */
+        JS_ASSERT(OBJ_GET_CLASS(cx, obj) != &js_BlockClass);
+
+        if (unqualified && !OBJ_GET_PARENT(cx, obj) &&
+            JS_HAS_STRICT_OPTION(cx)) {
+            JSString *str = JSVAL_TO_STRING(ID_TO_VALUE(id));
+            const char *bytes = js_GetStringBytes(cx, str);
+            if (!bytes ||
+                !JS_ReportErrorFlagsAndNumber(cx,
+                                              JSREPORT_WARNING |
+                                              JSREPORT_STRICT,
+                                              js_GetErrorMessage, NULL,
+                                              JSMSG_UNDECLARED_VAR, bytes)) {
+                return NULL;
+            }
+        }
     }
     sprop = (JSScopeProperty *) prop;
 
     /*
      * Now either sprop is null, meaning id was not found in obj or one of its
      * prototypes; or sprop is non-null, meaning id was found in pobj's scope.
      * If JS_THREADSAFE and sprop is non-null, then scope is locked, and sprop
      * is held: we must OBJ_DROP_PROPERTY or JS_UNLOCK_SCOPE before we return
@@ -4482,19 +4523,16 @@ js_SetPropertyHelper(JSContext *cx, JSOb
         }
 #ifdef __GNUC__ /* suppress bogus gcc warnings */
     } else {
         scope = NULL;
 #endif
     }
 
     if (!sprop) {
-        /* We should never add properties to lexical blocks.  */
-        JS_ASSERT(OBJ_GET_CLASS(cx, obj) != &js_BlockClass);
-
         /*
          * Purge the property cache of now-shadowed id in obj's scope chain.
          * Do this early, before locking obj to avoid nesting locks.
          */
         js_PurgeScopeChain(cx, obj, id);
 
         /* Find or make a property descriptor with the right heritage. */
         JS_LOCK_OBJ(cx, obj);
@@ -4544,17 +4582,17 @@ js_SetPropertyHelper(JSContext *cx, JSOb
     return js_ReportValueErrorFlags(cx, flags, JSMSG_READ_ONLY,
                                     JSDVG_IGNORE_STACK, ID_TO_VALUE(id), NULL,
                                     NULL, NULL);
 }
 
 JSBool
 js_SetProperty(JSContext *cx, JSObject *obj, jsid id, jsval *vp)
 {
-    return js_SetPropertyHelper(cx, obj, id, vp, NULL);
+    return js_SetPropertyHelper(cx, obj, id, false, vp, NULL);
 }
 
 JSBool
 js_GetAttributes(JSContext *cx, JSObject *obj, jsid id, JSProperty *prop,
                  uintN *attrsp)
 {
     JSBool noprop, ok;
     JSScopeProperty *sprop;
--- a/js/src/jsobj.h
+++ b/js/src/jsobj.h
@@ -676,17 +676,18 @@ js_FindPropertyHelper(JSContext *cx, jsi
  * Return the index along the scope chain in which id was found, or the last
  * index if not found, or -1 on error.
  */
 extern JS_FRIEND_API(JSBool)
 js_FindProperty(JSContext *cx, jsid id, JSObject **objp, JSObject **pobjp,
                 JSProperty **propp);
 
 extern JS_REQUIRES_STACK JSObject *
-js_FindIdentifierBase(JSContext *cx, jsid id, JSPropCacheEntry *entry);
+js_FindIdentifierBase(JSContext *cx, JSObject *scopeChain, jsid id,
+                      JSPropCacheEntry *entry);
 
 extern JSObject *
 js_FindVariableScope(JSContext *cx, JSFunction **funp);
 
 /*
  * NB: js_NativeGet and js_NativeSet are called with the scope containing sprop
  * (pobj's scope for Get, obj's for Set) locked, and on successful return, that
  * scope is again locked.  But on failure, both functions return false with the
@@ -706,18 +707,18 @@ js_GetPropertyHelper(JSContext *cx, JSOb
 extern JSBool
 js_GetProperty(JSContext *cx, JSObject *obj, jsid id, jsval *vp);
 
 extern JSBool
 js_GetMethod(JSContext *cx, JSObject *obj, jsid id, jsval *vp,
              JSPropCacheEntry **entryp);
 
 extern JSBool
-js_SetPropertyHelper(JSContext *cx, JSObject *obj, jsid id, jsval *vp,
-                     JSPropCacheEntry **entryp);
+js_SetPropertyHelper(JSContext *cx, JSObject *obj, jsid id,
+                     JSBool unqualified, jsval *vp, JSPropCacheEntry **entryp);
 
 extern JSBool
 js_SetProperty(JSContext *cx, JSObject *obj, jsid id, jsval *vp);
 
 extern JSBool
 js_GetAttributes(JSContext *cx, JSObject *obj, jsid id, JSProperty *prop,
                  uintN *attrsp);