Changeset 201562 in webkit

Timestamp:

Jun 1, 2016, 12:32:34 PM (10 years ago)

Author:

ggaren@apple.com

Message:

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.

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• jit/Repatch.cpp (modified) (2 diffs)
• runtime/JSObject.cpp (modified) (1 diff)
• runtime/JSObject.h (modified) (1 diff)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/JavaScriptCore/jit/Repatch.cpp

@@ -298 +298 @@
 
         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())
@@ -446 +452 @@
             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;

trunk/Source/JavaScriptCore/runtime/JSObject.cpp

@@ -2002 +2002 @@
 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);
 }

trunk/Source/JavaScriptCore/runtime/JSObject.h

@@ -1226 +1226 @@
 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());
 }