Structure::get should instantiate DeferGC only when materializing property map

https://bugs.webkit.org/show_bug.cgi?id=133727

Reviewed by Geoffrey Garen.

DeferGC instances in Structure::get was added in http://trac.webkit.org/r157539 in order to avoid
collecting the property table newly created by materializePropertyMapIfNecessary since GC can happen
when GCSafeConcurrentJITLocker goes out of scope.

However, always instantiating DeferGC inside Structure::get introduced a new performance bottleneck
in JSObject::getPropertySlot because frequently incrementing and decrementing a counter in vm.m_heap
and running a release assertion inside Heap::incrementDeferralDepth() is expensive.

Work around this by instantiating DeferGC only when we're actually calling materializePropertyMap,
and immediately storing a pointer to the newly created property table in the stack before DeferGC
goes out of scope so that the property table will be marked.

This shows 13-16% improvement on the microbenchmark attached in the bug.

* runtime/JSCJSValue.cpp:
* runtime/JSObject.h:
(JSC::JSObject::fastGetOwnPropertySlot):
* runtime/Structure.h:
(JSC::Structure::materializePropertyMapIfNecessary):
* runtime/StructureInlines.h:
(JSC::Structure::get):

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

diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
index ac457fa4652d1f5f5f5baae920a25cae5d9da3e0..4cb96709ad6eaca6327ffeb355e9cc754d453acd 100644 (file)
--- a/Source/JavaScriptCore/ChangeLog
+++ b/Source/JavaScriptCore/ChangeLog
@@ -1,3 +1,32 @@
+2014-06-11  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Structure::get should instantiate DeferGC only when materializing property map
+        https://bugs.webkit.org/show_bug.cgi?id=133727
+
+        Reviewed by Geoffrey Garen.
+
+        DeferGC instances in Structure::get was added in http://trac.webkit.org/r157539 in order to avoid
+        collecting the property table newly created by materializePropertyMapIfNecessary since GC can happen
+        when GCSafeConcurrentJITLocker goes out of scope.
+
+        However, always instantiating DeferGC inside Structure::get introduced a new performance bottleneck
+        in JSObject::getPropertySlot because frequently incrementing and decrementing a counter in vm.m_heap
+        and running a release assertion inside Heap::incrementDeferralDepth() is expensive.
+
+        Work around this by instantiating DeferGC only when we're actually calling materializePropertyMap,
+        and immediately storing a pointer to the newly created property table in the stack before DeferGC
+        goes out of scope so that the property table will be marked.
+
+        This shows 13-16% improvement on the microbenchmark attached in the bug.
+
+        * runtime/JSCJSValue.cpp:
+        * runtime/JSObject.h:
+        (JSC::JSObject::fastGetOwnPropertySlot):
+        * runtime/Structure.h:
+        (JSC::Structure::materializePropertyMapIfNecessary):
+        * runtime/StructureInlines.h:
+        (JSC::Structure::get):
+
 2014-06-11  Andreas Kling  <akling@apple.com>
 
         Some JSValue::get() micro-optimzations.

diff --git a/Source/JavaScriptCore/runtime/JSCJSValue.cpp b/Source/JavaScriptCore/runtime/JSCJSValue.cpp
index 7d7b7bd5ff75a0ca9e08aa48f4db0c34fbf29f11..1965fe1ba57193051535d8128f9b6a04a56c20d0 100644 (file)
--- a/Source/JavaScriptCore/runtime/JSCJSValue.cpp
+++ b/Source/JavaScriptCore/runtime/JSCJSValue.cpp
@@ -34,6 +34,7 @@
 #include "JSGlobalObject.h"
 #include "JSNotAnObject.h"
 #include "NumberObject.h"
+#include "StructureInlines.h"
 #include <wtf/MathExtras.h>
 #include <wtf/StringExtras.h>
 

diff --git a/Source/JavaScriptCore/runtime/JSObject.h b/Source/JavaScriptCore/runtime/JSObject.h
index 4854a0606e17744e0b29a3a5c13e41985f26b06a..d2497585c0b5564b0e7aa3dffa2ade691f762d0d 100644 (file)
--- a/Source/JavaScriptCore/runtime/JSObject.h
+++ b/Source/JavaScriptCore/runtime/JSObject.h
@@ -1256,7 +1256,7 @@ ALWAYS_INLINE bool JSObject::getOwnPropertySlot(JSObject* object, ExecState* exe
 
 ALWAYS_INLINE bool JSObject::fastGetOwnPropertySlot(ExecState* exec, VM& vm, Structure& structure, PropertyName propertyName, PropertySlot& slot)
 {
-    if (!TypeInfo::overridesGetOwnPropertySlot(inlineTypeFlags()))
+    if (LIKELY(!TypeInfo::overridesGetOwnPropertySlot(inlineTypeFlags())))
         return asObject(this)->inlineGetOwnPropertySlot(vm, structure, propertyName, slot);
     return structure.classInfo()->methodTable.getOwnPropertySlot(this, exec, propertyName, slot);
 }

diff --git a/Source/JavaScriptCore/runtime/Structure.h b/Source/JavaScriptCore/runtime/Structure.h
index 023d25a8853c6cbc4d53e348b7852add92ba1a0e..f1a2c2e4379a8d68d94a615cd5f29966bb417097 100644 (file)
--- a/Source/JavaScriptCore/runtime/Structure.h
+++ b/Source/JavaScriptCore/runtime/Structure.h
@@ -434,6 +434,18 @@ private:
         if (!propertyTable() && previousID())
             materializePropertyMap(vm);
     }
+    void materializePropertyMapIfNecessary(VM& vm, PropertyTable*& table)
+    {
+        ASSERT(!isCompilationThread());
+        ASSERT(structure()->classInfo() == info());
+        ASSERT(checkOffsetConsistency());
+        table = propertyTable().get();
+        if (!table && previousID()) {
+            DeferGC deferGC(vm.heap);
+            materializePropertyMap(vm);
+            table = propertyTable().get();
+        }
+    }
     void materializePropertyMapIfNecessaryForPinning(VM& vm, DeferGC&)
     {
         ASSERT(structure()->classInfo() == info());

diff --git a/Source/JavaScriptCore/runtime/StructureInlines.h b/Source/JavaScriptCore/runtime/StructureInlines.h
index 4098456106e29ef49a736852b3b9641553dd36eb..c868231abb4197243eb6635bffb533beb98bfdb7 100644 (file)
--- a/Source/JavaScriptCore/runtime/StructureInlines.h
+++ b/Source/JavaScriptCore/runtime/StructureInlines.h
@@ -77,12 +77,12 @@ inline PropertyOffset Structure::get(VM& vm, PropertyName propertyName)
 {
     ASSERT(!isCompilationThread());
     ASSERT(structure()->classInfo() == info());
-    DeferGC deferGC(vm.heap);
-    materializePropertyMapIfNecessary(vm, deferGC);
-    if (!propertyTable())
+    PropertyTable* propertyTable;
+    materializePropertyMapIfNecessary(vm, propertyTable);
+    if (!propertyTable)
         return invalidOffset;
 
-    PropertyMapEntry* entry = propertyTable()->get(propertyName.uid());
+    PropertyMapEntry* entry = propertyTable->get(propertyName.uid());
     return entry ? entry->offset : invalidOffset;
 }
 
@@ -90,12 +90,12 @@ inline PropertyOffset Structure::get(VM& vm, const WTF::String& name)
 {
     ASSERT(!isCompilationThread());
     ASSERT(structure()->classInfo() == info());
-    DeferGC deferGC(vm.heap);
-    materializePropertyMapIfNecessary(vm, deferGC);
-    if (!propertyTable())
+    PropertyTable* propertyTable;
+    materializePropertyMapIfNecessary(vm, propertyTable);
+    if (!propertyTable)
         return invalidOffset;
 
-    PropertyMapEntry* entry = propertyTable()->findWithString(name.impl()).first;
+    PropertyMapEntry* entry = propertyTable->findWithString(name.impl()).first;
     return entry ? entry->offset : invalidOffset;
 }
     
@@ -104,12 +104,12 @@ inline PropertyOffset Structure::get(VM& vm, PropertyName propertyName, unsigned
     ASSERT(!isCompilationThread());
     ASSERT(structure()->classInfo() == info());
 
-    DeferGC deferGC(vm.heap);
-    materializePropertyMapIfNecessary(vm, deferGC);
-    if (!propertyTable())
+    PropertyTable* propertyTable;
+    materializePropertyMapIfNecessary(vm, propertyTable);
+    if (!propertyTable)
         return invalidOffset;
 
-    PropertyMapEntry* entry = propertyTable()->get(propertyName.uid());
+    PropertyMapEntry* entry = propertyTable->get(propertyName.uid());
     if (!entry)
         return invalidOffset;