Changeset 41232 in webkit for trunk/JavaScriptCore/runtime/Structure.cpp

Timestamp:

Feb 25, 2009, 3:44:07 PM (16 years ago)

Author:

[email protected]

Message:

JavaScriptCore:

2009-02-25 Geoffrey Garen <​[email protected]>

Reviewed by Maciej Stachowiak.

Fixed <rdar://problem/6611174> REGRESSION (r36701): Unable to select
messages on hotmail (24052)

The bug was that for-in enumeration used a cached prototype chain without
validating that it was up-to-date.

This led me to refactor prototype chain caching so it was easier to work
with and harder to get wrong.

After a bit of inlining, this patch is performance-neutral on SunSpider
and the v8 benchmarks.

• interpreter/Interpreter.cpp: (JSC::Interpreter::tryCachePutByID): (JSC::Interpreter::tryCacheGetByID):
• jit/JITStubs.cpp: (JSC::JITStubs::tryCachePutByID): (JSC::JITStubs::tryCacheGetByID): (JSC::JITStubs::cti_op_get_by_id_proto_list): Use the new refactored goodness. See lines beginning with "-" and smile.

• runtime/JSGlobalObject.h: (JSC::Structure::prototypeForLookup): A shout out to const.

• runtime/JSPropertyNameIterator.h: (JSC::JSPropertyNameIterator::next): We can use a pointer comparison to see if our cached structure chain is equal to the object's structure chain, since in the case of a cache hit, we share references to the same structure chain.

• runtime/Operations.h: (JSC::countPrototypeChainEntriesAndCheckForProxies): Use the new refactored goodness.

• runtime/PropertyNameArray.h: (JSC::PropertyNameArray::PropertyNameArray): (JSC::PropertyNameArray::setShouldCache): (JSC::PropertyNameArray::shouldCache): Renamed "cacheable" to "shouldCache" to communicate that the client is specifying a recommendation, not a capability.

• runtime/Structure.cpp: (JSC::Structure::Structure): No need to initialize a RefPtr. (JSC::Structure::getEnumerablePropertyNames): Moved some code into helper functions.

(JSC::Structure::prototypeChain): New centralized accessor for a prototype
chain. Revalidates on every access, since the objects in the prototype
chain may have mutated.

(JSC::Structure::isValid): Helper function for revalidating a cached
prototype chain.

(JSC::Structure::getEnumerableNamesFromPropertyTable):
(JSC::Structure::getEnumerableNamesFromClassInfoTable): Factored out of
getEnumerablePropertyNames.

• runtime/Structure.h:

• runtime/StructureChain.cpp: (JSC::StructureChain::StructureChain):
• runtime/StructureChain.h: (JSC::StructureChain::create): No need for structureChainsAreEqual, since we use pointer equality now. Refactored StructureChain to make a little more sense and eliminate special cases for null prototypes.

LayoutTests:

2009-02-24 Geoffrey Garen <​[email protected]>

Reviewed by Maciej Stachowiak.

Added a test for <rdar://problem/6611174> REGRESSION (r36701): Unable to
select messages on hotmail (24052)

• fast/js/for-in-cached-expected.txt: Added.
• fast/js/for-in-cached.html: Added.
• fast/js/resources/for-in-cached.js: Added. (forIn):

File:

• trunk/JavaScriptCore/runtime/Structure.cpp (modified) (5 diffs)

trunk/JavaScriptCore/runtime/Structure.cpp

@@ -124 +124 @@
     : m_typeInfo(typeInfo)
     , m_prototype(prototype)
-    , m_cachedPrototypeChain(0)
-    , m_previous(0)
-    , m_nameInPrevious(0)
     , m_propertyTable(0)
     , m_propertyStorageCapacity(JSObject::inlineStorageCapacity)
@@ -290 +287 @@
 void Structure::getEnumerablePropertyNames(ExecState* exec, PropertyNameArray& propertyNames, JSObject* baseObject)
 {
-    bool shouldCache = propertyNames.cacheable() && !(propertyNames.size() || m_isDictionary);
+    bool shouldCache = propertyNames.shouldCache() && !(propertyNames.size() || m_isDictionary);
+
+    if (shouldCache && m_cachedPropertyNameArrayData) {
+        if (m_cachedPropertyNameArrayData->cachedPrototypeChain() == prototypeChain(exec)) {
+            propertyNames.setData(m_cachedPropertyNameArrayData);
+            return;
+        }
+        clearEnumerationCache();
+    }
+
+    getEnumerableNamesFromPropertyTable(propertyNames);
+    getEnumerableNamesFromClassInfoTable(exec, baseObject->classInfo(), propertyNames);
+
+    if (m_prototype.isObject()) {
+        propertyNames.setShouldCache(false); // No need for our prototypes to waste memory on caching, since they're not being enumerated directly.
+        asObject(m_prototype)->getPropertyNames(exec, propertyNames);
+    }
 
     if (shouldCache) {
-        if (m_cachedPropertyNameArrayData) {
-            if (structureChainsAreEqual(m_cachedPropertyNameArrayData->cachedPrototypeChain(), cachedPrototypeChain())) {
-                propertyNames.setData(m_cachedPropertyNameArrayData);
-                return;
-            }
-        }
-        propertyNames.setCacheable(false);
-    }
-
-    getEnumerablePropertyNamesInternal(propertyNames);
-
-    // Add properties from the static hashtables of properties
-    for (const ClassInfo* info = baseObject->classInfo(); info; info = info->parentClass) {
-        const HashTable* table = info->propHashTable(exec);
-        if (!table)
-            continue;
-        table->initializeIfNeeded(exec);
-        ASSERT(table->table);
-#if ENABLE(PERFECT_HASH_SIZE)
-        int hashSizeMask = table->hashSizeMask;
-#else
-        int hashSizeMask = table->compactSize - 1;
-#endif
-        const HashEntry* entry = table->table;
-        for (int i = 0; i <= hashSizeMask; ++i, ++entry) {
-            if (entry->key() && !(entry->attributes() & DontEnum))
-                propertyNames.add(entry->key());
-        }
-    }
-
-    if (m_prototype.isObject())
-        asObject(m_prototype)->getPropertyNames(exec, propertyNames);
-
-    if (shouldCache) {
-        if (m_cachedPropertyNameArrayData)
-            m_cachedPropertyNameArrayData->setCachedStructure(0);
-
         m_cachedPropertyNameArrayData = propertyNames.data();
-
-        StructureChain* chain = cachedPrototypeChain();
-        if (!chain)
-            chain = createCachedPrototypeChain();
-        m_cachedPropertyNameArrayData->setCachedPrototypeChain(chain);
+        m_cachedPropertyNameArrayData->setCachedPrototypeChain(prototypeChain(exec));
         m_cachedPropertyNameArrayData->setCachedStructure(this);
     }
@@ -535 +507 @@
     clearEnumerationCache();
     return offset;
-}
-
-StructureChain* Structure::createCachedPrototypeChain()
-{
-    ASSERT(typeInfo().type() == ObjectType);
-    ASSERT(!m_cachedPrototypeChain);
-
-    JSValuePtr prototype = storedPrototype();
-    if (!prototype.isCell())
-        return 0;
-
-    RefPtr<StructureChain> chain = StructureChain::create(asObject(prototype)->structure());
-    setCachedPrototypeChain(chain.release());
-    return cachedPrototypeChain();
 }
 
@@ -925 +883 @@
 }
 
-void Structure::getEnumerablePropertyNamesInternal(PropertyNameArray& propertyNames)
+void Structure::getEnumerableNamesFromPropertyTable(PropertyNameArray& propertyNames)
 {
     materializePropertyMapIfNecessary();
@@ -979 +937 @@
         for (size_t i = 0; i < sortedEnumerables.size(); ++i)
             propertyNames.add(sortedEnumerables[i]->key);
+    }
+}
+
+void Structure::getEnumerableNamesFromClassInfoTable(ExecState* exec, const ClassInfo* classInfo, PropertyNameArray& propertyNames)
+{
+    // Add properties from the static hashtables of properties
+    for (; classInfo; classInfo = classInfo->parentClass) {
+        const HashTable* table = classInfo->propHashTable(exec);
+        if (!table)
+            continue;
+        table->initializeIfNeeded(exec);
+        ASSERT(table->table);
+#if ENABLE(PERFECT_HASH_SIZE)
+        int hashSizeMask = table->hashSizeMask;
+#else
+        int hashSizeMask = table->compactSize - 1;
+#endif
+        const HashEntry* entry = table->table;
+        for (int i = 0; i <= hashSizeMask; ++i, ++entry) {
+            if (entry->key() && !(entry->attributes() & DontEnum))
+                propertyNames.add(entry->key());
+        }
     }
 }