Reviewed by Darin and Geoff.
authormjs <mjs@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 27 Dec 2005 09:24:14 +0000 (09:24 +0000)
committermjs <mjs@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 27 Dec 2005 09:24:14 +0000 (09:24 +0000)
Changes by me and Anders.

- mostly fixed REGRESSION: 5-10% performance regression on JS iBench from getter/setter change
http://bugzilla.opendarwin.org/show_bug.cgi?id=6083

- also fixed some warnings reported by -Winline

        * JavaScriptCorePrefix.h: Move new and delete definitions higher so there
aren't conflicts with use in standard C++ headers
        * kjs/object.cpp:
        (KJS::throwSetterError): Moved this piece of put into a seprate function
to avoid the PIC branch.
        (KJS::JSObject::put): Use hasGetterSetterProperties to avoid expensive stuff
when not needed. Also use GetterSetter properties attribute.
        (KJS::JSObject::deleteProperty): Recompute whether any properties are getter/setter
properties any more, if this one was one.
        (KJS::JSObject::defineGetter): Let the PropertyMap know that it has getter/setter
properties now (and use the new attribute).
        (KJS::JSObject::defineSetter): Ditto.
        (KJS::JSObject::fillGetterPropertySlot): Out-of-line helper for getOwnPropertySlot,
to avoid global variable access in the hot code path.
        * kjs/object.h:
        (KJS::): Added GetterSetter attribute.
        (KJS::JSCell::isObject): Moved lower to be after inline methods it uses.
        (KJS::JSValue::isObject): ditto
        (KJS::JSObject::getOwnPropertySlot): try to avoid impact of getters and setters
as much as possible in the case where they are not being used
        * kjs/property_map.cpp:
        (KJS::PropertyMap::containsGettersOrSetters): New method to help with this
        * kjs/property_map.h:
        (KJS::PropertyMap::hasGetterSetterProperties): Ditto
        (KJS::PropertyMap::setHasGetterSetterProperties): Ditto
        (KJS::PropertyMap::PropertyMap): Added a crazy hack to store the
global "has getter/setter properties" flag in the property map
single entry, to avoid making objects any bigger.
        * kjs/value.h: Moved some things to object.h to make -Winline happier

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@11773 268f45cc-cd09-0410-ab3c-d52691b4dbfc

JavaScriptCore/ChangeLog
JavaScriptCore/JavaScriptCorePrefix.h
JavaScriptCore/kjs/object.cpp
JavaScriptCore/kjs/object.h
JavaScriptCore/kjs/property_map.cpp
JavaScriptCore/kjs/property_map.h
JavaScriptCore/kjs/value.h

index e1e7f86..8241247 100644 (file)
@@ -1,3 +1,44 @@
+2005-12-26  Maciej Stachowiak  <mjs@apple.com>
+
+        Reviewed by Darin and Geoff.
+
+       Changes by me and Anders.
+
+       - mostly fixed REGRESSION: 5-10% performance regression on JS iBench from getter/setter change
+       http://bugzilla.opendarwin.org/show_bug.cgi?id=6083
+
+       - also fixed some warnings reported by -Winline
+       
+        * JavaScriptCorePrefix.h: Move new and delete definitions higher so there
+       aren't conflicts with use in standard C++ headers
+        * kjs/object.cpp:
+        (KJS::throwSetterError): Moved this piece of put into a seprate function
+       to avoid the PIC branch.
+        (KJS::JSObject::put): Use hasGetterSetterProperties to avoid expensive stuff
+       when not needed. Also use GetterSetter properties attribute.
+        (KJS::JSObject::deleteProperty): Recompute whether any properties are getter/setter
+       properties any more, if this one was one.
+        (KJS::JSObject::defineGetter): Let the PropertyMap know that it has getter/setter
+       properties now (and use the new attribute).
+        (KJS::JSObject::defineSetter): Ditto.
+        (KJS::JSObject::fillGetterPropertySlot): Out-of-line helper for getOwnPropertySlot,
+       to avoid global variable access in the hot code path.
+        * kjs/object.h:
+        (KJS::): Added GetterSetter attribute.
+        (KJS::JSCell::isObject): Moved lower to be after inline methods it uses.
+        (KJS::JSValue::isObject): ditto
+        (KJS::JSObject::getOwnPropertySlot): try to avoid impact of getters and setters
+       as much as possible in the case where they are not being used
+        * kjs/property_map.cpp:
+        (KJS::PropertyMap::containsGettersOrSetters): New method to help with this
+        * kjs/property_map.h:
+        (KJS::PropertyMap::hasGetterSetterProperties): Ditto
+        (KJS::PropertyMap::setHasGetterSetterProperties): Ditto
+        (KJS::PropertyMap::PropertyMap): Added a crazy hack to store the
+       global "has getter/setter properties" flag in the property map
+       single entry, to avoid making objects any bigger.
+        * kjs/value.h: Moved some things to object.h to make -Winline happier
+
 2005-12-24  Maciej Stachowiak  <mjs@apple.com>
 
         Reviewed by Eric and Dave Hyatt.
index 8aa9016..9c2abef 100644 (file)
@@ -1,9 +1,16 @@
 #ifdef __cplusplus
+#define new ("if you use new/delete make sure to include config.h at the top of the file"()) 
+#define delete ("if you use new/delete make sure to include config.h at the top of the file"()) 
+#endif
+
+#ifdef __cplusplus
 #define NULL __null
 #else
 #define NULL ((void *)0)
 #endif
 
+#include "config.h"
+
 #include <assert.h>
 #include <ctype.h>
 #include <float.h>
@@ -40,8 +47,4 @@
 #include <list>
 #include <typeinfo>
 
-#ifdef __cplusplus
-#define new ("if you use new/delete make sure to include config.h at the top of the file"()) 
-#define delete ("if you use new/delete make sure to include config.h at the top of the file"()) 
-#endif
 #endif
index 8a7c715..ed6938e 100644 (file)
@@ -190,6 +190,11 @@ bool JSObject::getOwnPropertySlot(ExecState *exec, unsigned propertyName, Proper
   return getOwnPropertySlot(exec, Identifier::from(propertyName), slot);
 }
 
+static void throwSetterError(ExecState *exec)
+{
+  throwError(exec, TypeError, "setting a property that has only a getter");
+}
+
 // ECMA 8.6.2.2
 void JSObject::put(ExecState *exec, const Identifier &propertyName, JSValue *value, int attr)
 {
@@ -215,12 +220,14 @@ void JSObject::put(ExecState *exec, const Identifier &propertyName, JSValue *val
 
   JSObject *obj = this;
   while (true) {
-    if (JSValue *gs = obj->_prop.get(propertyName)) {
-      if (gs->type() == GetterSetterType) {
+    JSValue *gs;
+    int attributes;
+    if (obj->_prop.hasGetterSetterProperties() && (gs = obj->_prop.get(propertyName, attributes))) {
+      if (attributes & GetterSetter) {
         JSObject *setterFunc = static_cast<GetterSetterImp *>(gs)->getSetter();
             
         if (!setterFunc) {
-          throwError(exec, TypeError, "setting a property that has only a getter");
+          throwSetterError(exec);
           return;
         }
             
@@ -236,7 +243,7 @@ void JSObject::put(ExecState *exec, const Identifier &propertyName, JSValue *val
       }
     }
      
-    if (!obj->_proto || !obj->_proto->isObject())
+    if (!obj->_proto->isObject())
       break;
         
     obj = static_cast<JSObject *>(obj->_proto);
@@ -287,6 +294,8 @@ bool JSObject::deleteProperty(ExecState */*exec*/, const Identifier &propertyNam
     if ((attributes & DontDelete))
       return false;
     _prop.remove(propertyName);
+    if (attributes & GetterSetter) 
+        _prop.setHasGetterSetterProperties(_prop.containsGettersOrSetters());
     return true;
   }
 
@@ -370,9 +379,10 @@ void JSObject::defineGetter(ExecState *exec, const Identifier& propertyName, JSO
         gs = static_cast<GetterSetterImp *>(o);
     } else {
         gs = new GetterSetterImp;
-        putDirect(propertyName, gs);
+        putDirect(propertyName, gs, GetterSetter);
     }
     
+    _prop.setHasGetterSetterProperties(true);
     gs->setGetter(getterFunc);
 }
 
@@ -386,8 +396,10 @@ void JSObject::defineSetter(ExecState *exec, const Identifier& propertyName, JSO
     } else {
         gs = new GetterSetterImp;
         putDirect(propertyName, gs);
+        putDirect(propertyName, gs, GetterSetter);
     }
     
+    _prop.setHasGetterSetterProperties(true);
     gs->setSetter(setterFunc);
 }
 
@@ -520,6 +532,16 @@ void JSObject::putDirect(const Identifier &propertyName, int value, int attr)
     _prop.put(propertyName, jsNumber(value), attr);
 }
 
+void JSObject::fillGetterPropertySlot(PropertySlot& slot, JSValue **location)
+{
+    GetterSetterImp *gs = static_cast<GetterSetterImp *>(*location);
+    JSObject *getterFunc = gs->getGetter();
+    if (getterFunc)
+        slot.setGetterSlot(this, getterFunc);
+    else
+        slot.setUndefined(this);
+}
+
 // ------------------------------ Error ----------------------------------------
 
 const char * const errorNamesArr[] = {
index bc768a0..7a1c46b 100644 (file)
@@ -50,12 +50,13 @@ namespace KJS {
 
   // ECMA 262-3 8.6.1
   // Property attributes
-  enum Attribute { None       = 0,
-                   ReadOnly   = 1 << 1, // property can be only read, not written
-                   DontEnum   = 1 << 2, // property doesn't appear in (for .. in ..)
-                   DontDelete = 1 << 3, // property can't be deleted
-                   Internal   = 1 << 4, // an internal property, set to bypass checks
-                   Function   = 1 << 5 }; // property is a function - only used by static hashtables
+  enum Attribute { None         = 0,
+                   ReadOnly     = 1 << 1, // property can be only read, not written
+                   DontEnum     = 1 << 2, // property doesn't appear in (for .. in ..)
+                   DontDelete   = 1 << 3, // property can't be deleted
+                   Internal     = 1 << 4, // an internal property, set to bypass checks
+                   Function     = 1 << 5, // property is a function - only used by static hashtables
+                   GetterSetter = 1 << 6 }; // property is a getter or setter
 
   /**
    * Class Information
@@ -502,7 +503,9 @@ namespace KJS {
         { return _prop.getLocation(propertyName); }
     void putDirect(const Identifier &propertyName, JSValue *value, int attr = 0);
     void putDirect(const Identifier &propertyName, int value, int attr = 0);
-    
+
+    void fillGetterPropertySlot(PropertySlot& slot, JSValue **location);
+
     void defineGetter(ExecState *exec, const Identifier& propertyName, JSObject *getterFunc);
     void defineSetter(ExecState *exec, const Identifier& propertyName, JSObject *setterFunc);
 
@@ -566,11 +569,6 @@ JSObject *throwError(ExecState *, ErrorType, const UString &message, int lineNum
 JSObject *throwError(ExecState *, ErrorType, const UString &message);
 JSObject *throwError(ExecState *, ErrorType, const char *message);
 JSObject *throwError(ExecState *, ErrorType);
-  
-inline bool JSCell::isObject(const ClassInfo *info) const
-{
-    return isObject() && static_cast<const JSObject *>(this)->inherits(info);
-}
 
 inline JSObject::JSObject(JSObject *proto)
     : _proto(proto), _internalValue(0)
@@ -612,6 +610,18 @@ inline bool JSObject::inherits(const ClassInfo *info) const
     return false;
 }
 
+// this method is here to be after the inline declaration of JSObject::inherits
+inline bool JSCell::isObject(const ClassInfo *info) const
+{
+    return isObject() && static_cast<const JSObject *>(this)->inherits(info);
+}
+
+// this method is here to be after the inline declaration of JSCell::isObject
+inline bool JSValue::isObject(const ClassInfo *c) const
+{
+    return !SimpleNumber::is(this) && downcast()->isObject(c);
+}
+
 // It may seem crazy to inline a function this large but it makes a big difference
 // since this is function very hot in variable lookup
 inline bool JSObject::getPropertySlot(ExecState *exec, const Identifier& propertyName, PropertySlot& slot)
@@ -632,19 +642,13 @@ inline bool JSObject::getPropertySlot(ExecState *exec, const Identifier& propert
 // It may seem crazy to inline a function this large, especially a virtual function,
 // but it makes a big difference to property lookup that derived classes can inline their
 // base class call to this.
-inline bool JSObject::getOwnPropertySlot(ExecState *exec, const Identifier& propertyName, PropertySlot& slot)
+ALWAYS_INLINE bool JSObject::getOwnPropertySlot(ExecState *exec, const Identifier& propertyName, PropertySlot& slot)
 {
     if (JSValue **location = getDirectLocation(propertyName)) {
-        if ((*location)->type() == GetterSetterType) {
-            GetterSetterImp *gs = static_cast<GetterSetterImp *>(*location);
-            JSObject *getterFunc = gs->getGetter();
-            if (getterFunc)
-                slot.setGetterSlot(this, getterFunc);
-            else
-                slot.setUndefined(this);
-        } else {
+        if (_prop.hasGetterSetterProperties() && location[0]->type() == GetterSetterType)
+            fillGetterPropertySlot(slot, location);
+        else
             slot.setValueSlot(this, location);
-        }
         return true;
     }
 
index 8370fcc..5d0d2e9 100644 (file)
@@ -566,6 +566,23 @@ static int comparePropertyMapEntryIndices(const void *a, const void *b)
     return 0;
 }
 
+bool PropertyMap::containsGettersOrSetters() const
+{
+    if (!_table) {
+#if USE_SINGLE_ENTRY
+        return _singleEntry.attributes & GetterSetter;
+#endif
+        return false;
+    }
+
+    for (int i = 0; i != _table->size; ++i) {
+        if (_table->entries[i].attributes & GetterSetter)
+            return true;
+    }
+    
+    return false;
+}
+
 void PropertyMap::addEnumerablesToReferenceList(ReferenceList &list, JSObject *base) const
 {
     if (!_table) {
index 5d6fba4..1ac4e9c 100644 (file)
@@ -60,7 +60,8 @@ namespace KJS {
         PropertyMapHashTableEntry() : key(0) { }
         UString::Rep *key;
         JSValue *value;
-        int attributes;
+        short attributes;
+        short globalGetterSetterFlag;
         int index;
     };
 /**
@@ -87,6 +88,10 @@ namespace KJS {
         void save(SavedProperties &) const;
         void restore(const SavedProperties &p);
 
+        bool hasGetterSetterProperties() const { return _singleEntry.globalGetterSetterFlag; }
+        void setHasGetterSetterProperties(bool f) { _singleEntry.globalGetterSetterFlag = f; }
+
+        bool containsGettersOrSetters() const;
     private:
         static bool keysMatch(const UString::Rep *, const UString::Rep *);
         void expand();
@@ -105,8 +110,9 @@ namespace KJS {
         Entry _singleEntry;
     };
 
-inline PropertyMap::PropertyMap() : _table(NULL)
+inline PropertyMap::PropertyMap() : _table(0)
 {
+    _singleEntry.globalGetterSetterFlag = 0;
 }
 
 } // namespace
index 912a23e..f638b52 100644 (file)
@@ -313,11 +313,6 @@ inline bool JSValue::isObject() const
     return !SimpleNumber::is(this) && downcast()->isObject();
 }
 
-inline bool JSValue::isObject(const ClassInfo *c) const
-{
-    return !SimpleNumber::is(this) && downcast()->isObject(c);
-}
-
 inline bool JSValue::getBoolean(bool& v) const
 {
     return !SimpleNumber::is(this) && downcast()->getBoolean(v);