Dictionary property access should be fast
authorggaren@apple.com <ggaren@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 1 Jun 2016 19:32:34 +0000 (19:32 +0000)
committerggaren@apple.com <ggaren@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 1 Jun 2016 19:32:34 +0000 (19:32 +0000)
https://bugs.webkit.org/show_bug.cgi?id=158250

Reviewed by Keith Miller.

We have some remnant code that unnecessarily takes a slow path for
dictionaries. This caused the Dromaeo regression in r201436. Let's fix
that.

* jit/Repatch.cpp:
(JSC::tryCacheGetByID): Attempt to flatten a dictionary if necessary, but
not too much. This is our idiom in other places.

(JSC::tryCachePutByID): See tryCacheGetByID.

* llint/LLIntSlowPaths.cpp:
(JSC::LLInt::setupGetByIdPrototypeCache): See tryCacheGetByID.

* runtime/JSObject.cpp:
(JSC::JSObject::fillGetterPropertySlot):
* runtime/JSObject.h:
(JSC::JSObject::fillCustomGetterPropertySlot): The rules for caching a
getter are the same as the rules for caching anything else: We're
allowed to cache even in dictionaries, as long as they're cacheable
dictionaries. Any transition that would change to/from getter/setter
or change other attributes requires a structure transition.

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/jit/Repatch.cpp
Source/JavaScriptCore/runtime/JSObject.cpp
Source/JavaScriptCore/runtime/JSObject.h

index f37674b..c75878d 100644 (file)
@@ -1,3 +1,32 @@
+2016-05-31  Geoffrey Garen  <ggaren@apple.com>
+
+        Dictionary property access should be fast
+        https://bugs.webkit.org/show_bug.cgi?id=158250
+
+        Reviewed by Keith Miller.
+
+        We have some remnant code that unnecessarily takes a slow path for
+        dictionaries. This caused the Dromaeo regression in r201436. Let's fix
+        that.
+
+        * jit/Repatch.cpp:
+        (JSC::tryCacheGetByID): Attempt to flatten a dictionary if necessary, but
+        not too much. This is our idiom in other places.
+
+        (JSC::tryCachePutByID): See tryCacheGetByID.
+
+        * llint/LLIntSlowPaths.cpp:
+        (JSC::LLInt::setupGetByIdPrototypeCache): See tryCacheGetByID.
+
+        * runtime/JSObject.cpp:
+        (JSC::JSObject::fillGetterPropertySlot):
+        * runtime/JSObject.h:
+        (JSC::JSObject::fillCustomGetterPropertySlot): The rules for caching a
+        getter are the same as the rules for caching anything else: We're
+        allowed to cache even in dictionaries, as long as they're cacheable
+        dictionaries. Any transition that would change to/from getter/setter
+        or change other attributes requires a structure transition.
+
 2016-05-31  Yusuke Suzuki  <utatane.tea@gmail.com>
 
         [JSC] Drop "replace" from JSC_COMMON_PRIVATE_IDENTIFIERS_EACH_WELL_KNOWN_SYMBOL_NOT_IMPLEMENTED_YET
index 1a12c50..8298453 100644 (file)
@@ -297,8 +297,14 @@ static InlineCacheAction tryCacheGetByID(ExecState* exec, JSValue baseValue, con
         PropertyOffset offset = slot.isUnset() ? invalidOffset : slot.cachedOffset();
 
         if (slot.isUnset() || slot.slotBase() != baseValue) {
-            if (structure->typeInfo().prohibitsPropertyCaching() || structure->isDictionary())
+            if (structure->typeInfo().prohibitsPropertyCaching())
                 return GiveUpOnCache;
+
+            if (structure->isDictionary()) {
+                if (structure->hasBeenFlattenedBefore())
+                    return GiveUpOnCache;
+                structure->flattenDictionaryStructure(vm, jsCast<JSObject*>(baseCell));
+            }
             
             if (slot.isUnset() && structure->typeInfo().getOwnPropertySlotIsImpureForPropertyAbsence())
                 return GiveUpOnCache;
@@ -445,9 +451,15 @@ static InlineCacheAction tryCachePutByID(ExecState* exec, JSValue baseValue, Str
         } else {
             ASSERT(slot.type() == PutPropertySlot::NewProperty);
 
-            if (!structure->isObject() || structure->isDictionary())
+            if (!structure->isObject())
                 return GiveUpOnCache;
 
+            if (structure->isDictionary()) {
+                if (structure->hasBeenFlattenedBefore())
+                    return GiveUpOnCache;
+                structure->flattenDictionaryStructure(vm, jsCast<JSObject*>(baseValue));
+            }
+
             PropertyOffset offset;
             Structure* newStructure =
                 Structure::addPropertyTransitionToExistingStructureConcurrently(
index 2c1a5bd..e9cc676 100644 (file)
@@ -2001,10 +2001,13 @@ bool JSObject::removeDirect(VM& vm, PropertyName propertyName)
 
 NEVER_INLINE void JSObject::fillGetterPropertySlot(PropertySlot& slot, JSValue getterSetter, unsigned attributes, PropertyOffset offset)
 {
-    if (structure()->isDictionary()) {
+    if (structure()->isUncacheableDictionary()) {
         slot.setGetterSlot(this, attributes, jsCast<GetterSetter*>(getterSetter));
         return;
     }
+
+    // This access is cacheable because Structure requires an attributeChangedTransition
+    // if this property stops being an accessor.
     slot.setCacheableGetterSlot(this, attributes, jsCast<GetterSetter*>(getterSetter), offset);
 }
 
index 6a3931e..c4d329c 100644 (file)
@@ -1225,10 +1225,13 @@ ALWAYS_INLINE bool JSObject::getOwnNonIndexPropertySlot(VM& vm, Structure& struc
 
 ALWAYS_INLINE void JSObject::fillCustomGetterPropertySlot(PropertySlot& slot, JSValue customGetterSetter, unsigned attributes, Structure& structure)
 {
-    if (structure.isDictionary()) {
+    if (structure.isUncacheableDictionary()) {
         slot.setCustom(this, attributes, jsCast<CustomGetterSetter*>(customGetterSetter)->getter());
         return;
     }
+
+    // This access is cacheable because Structure requires an attributeChangedTransition
+    // if this property stops being an accessor.
     slot.setCacheableCustom(this, attributes, jsCast<CustomGetterSetter*>(customGetterSetter)->getter());
 }