REGRESSION(r192914): 10% regression on Sunspider's date-format-tofte
authorutatane.tea@gmail.com <utatane.tea@gmail.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 29 Mar 2016 10:25:44 +0000 (10:25 +0000)
committerutatane.tea@gmail.com <utatane.tea@gmail.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 29 Mar 2016 10:25:44 +0000 (10:25 +0000)
https://bugs.webkit.org/show_bug.cgi?id=155559

Reviewed by Saam Barati.

Source/JavaScriptCore:

The fast path of the eval function is the super hot path in date-format-tofte.
Any performance regression is not allowed here.
Before this patch, we allocated SourceCode in the fast path.
This allocation incurs 10% performance regression.

This patch removes this allocation in the fast path.
And change the key of the EvalCodeCache to EvalCodeCache::CacheKey.
It combines RefPtr<StringImpl> and isArrowFunctionContext.
Since EvalCodeCache does not cache any eval code evaluated under the strict mode,
it is unnecessary to include several options (ThisTDZMode, and DerivedContextType) in the cache map's key.
But isArrowFunctionContext is necessary since the sloppy mode arrow function exists.

To validate this change, we add a new test that evaluates the same code
under the non-arrow function context and the arrow function context.

After introducing CacheKey, we observed 1% regression compared to the RefPtr<StringImpl> keyed case.
This is because HashMap<RefPtr<T>, ...>::get(T*) is specially optimized; this path is inlined while the normal ::get() is not inlined.
To avoid this performance regression, we introduce HashMap::fastGet, that aggressively encourages inlining.
The relationship between fastGet() and get() is similar to fastAdd() and add().
After applying this change, the evaluation shows no performance regression in comparison with the RefPtr<StringImpl> keyed case.

* bytecode/EvalCodeCache.h:
(JSC::EvalCodeCache::CacheKey::CacheKey):
(JSC::EvalCodeCache::CacheKey::hash):
(JSC::EvalCodeCache::CacheKey::isEmptyValue):
(JSC::EvalCodeCache::CacheKey::operator==):
(JSC::EvalCodeCache::CacheKey::isHashTableDeletedValue):
(JSC::EvalCodeCache::CacheKey::Hash::hash):
(JSC::EvalCodeCache::CacheKey::Hash::equal):
(JSC::EvalCodeCache::tryGet):
(JSC::EvalCodeCache::getSlow):
(JSC::EvalCodeCache::isCacheable):
* interpreter/Interpreter.cpp:
(JSC::eval):
* tests/stress/eval-in-arrow-function.js: Added.
(shouldBe):
(i):

Source/WTF:

Add HashTable::inlineLookup and HashMap::fastGet.

* wtf/HashMap.h:
* wtf/HashTable.h:

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bytecode/EvalCodeCache.h
Source/JavaScriptCore/interpreter/Interpreter.cpp
Source/JavaScriptCore/tests/stress/eval-in-arrow-function.js [new file with mode: 0644]
Source/WTF/ChangeLog
Source/WTF/wtf/HashMap.h
Source/WTF/wtf/HashTable.h

index a6988de..c832498 100644 (file)
@@ -1,3 +1,48 @@
+2016-03-29  Yusuke Suzuki  <utatane.tea@gmail.com>
+
+        REGRESSION(r192914): 10% regression on Sunspider's date-format-tofte
+        https://bugs.webkit.org/show_bug.cgi?id=155559
+
+        Reviewed by Saam Barati.
+
+        The fast path of the eval function is the super hot path in date-format-tofte.
+        Any performance regression is not allowed here.
+        Before this patch, we allocated SourceCode in the fast path.
+        This allocation incurs 10% performance regression.
+
+        This patch removes this allocation in the fast path.
+        And change the key of the EvalCodeCache to EvalCodeCache::CacheKey.
+        It combines RefPtr<StringImpl> and isArrowFunctionContext.
+        Since EvalCodeCache does not cache any eval code evaluated under the strict mode,
+        it is unnecessary to include several options (ThisTDZMode, and DerivedContextType) in the cache map's key.
+        But isArrowFunctionContext is necessary since the sloppy mode arrow function exists.
+
+        To validate this change, we add a new test that evaluates the same code
+        under the non-arrow function context and the arrow function context.
+
+        After introducing CacheKey, we observed 1% regression compared to the RefPtr<StringImpl> keyed case.
+        This is because HashMap<RefPtr<T>, ...>::get(T*) is specially optimized; this path is inlined while the normal ::get() is not inlined.
+        To avoid this performance regression, we introduce HashMap::fastGet, that aggressively encourages inlining.
+        The relationship between fastGet() and get() is similar to fastAdd() and add().
+        After applying this change, the evaluation shows no performance regression in comparison with the RefPtr<StringImpl> keyed case.
+
+        * bytecode/EvalCodeCache.h:
+        (JSC::EvalCodeCache::CacheKey::CacheKey):
+        (JSC::EvalCodeCache::CacheKey::hash):
+        (JSC::EvalCodeCache::CacheKey::isEmptyValue):
+        (JSC::EvalCodeCache::CacheKey::operator==):
+        (JSC::EvalCodeCache::CacheKey::isHashTableDeletedValue):
+        (JSC::EvalCodeCache::CacheKey::Hash::hash):
+        (JSC::EvalCodeCache::CacheKey::Hash::equal):
+        (JSC::EvalCodeCache::tryGet):
+        (JSC::EvalCodeCache::getSlow):
+        (JSC::EvalCodeCache::isCacheable):
+        * interpreter/Interpreter.cpp:
+        (JSC::eval):
+        * tests/stress/eval-in-arrow-function.js: Added.
+        (shouldBe):
+        (i):
+
 2016-03-29  Joseph Pecoraro  <pecoraro@apple.com>
 
         Audit WebCore builtins for user overridable code
index 663490a..e1c7b2b 100644 (file)
@@ -34,7 +34,6 @@
 #include "JSScope.h"
 #include "Options.h"
 #include "SourceCode.h"
-#include "SourceCodeKey.h"
 #include <wtf/HashMap.h>
 #include <wtf/RefPtr.h>
 #include <wtf/text/StringHash.h>
@@ -45,29 +44,73 @@ namespace JSC {
 
     class EvalCodeCache {
     public:
-        EvalExecutable* tryGet(bool inStrictContext, const SourceCode& evalSource, ThisTDZMode thisTDZMode, JSScope* scope)
+        class CacheKey {
+        public:
+            CacheKey(const String& source, bool isArrowFunctionContext)
+                : m_source(source.impl())
+                , m_isArrowFunctionContext(isArrowFunctionContext)
+            {
+            }
+
+            CacheKey(WTF::HashTableDeletedValueType)
+                : m_source(WTF::HashTableDeletedValue)
+            {
+            }
+
+            CacheKey() = default;
+
+            unsigned hash() const { return m_source->hash(); }
+
+            bool isEmptyValue() const { return !m_source; }
+
+            bool operator==(const CacheKey& other) const
+            {
+                return m_source == other.m_source && m_isArrowFunctionContext == other.m_isArrowFunctionContext;
+            }
+
+            bool isHashTableDeletedValue() const { return m_source.isHashTableDeletedValue(); }
+
+            struct Hash {
+                static unsigned hash(const CacheKey& key)
+                {
+                    return key.hash();
+                }
+                static bool equal(const CacheKey& lhs, const CacheKey& rhs)
+                {
+                    return StringHash::equal(lhs.m_source, rhs.m_source) && lhs.m_isArrowFunctionContext == rhs.m_isArrowFunctionContext;
+                }
+                static const bool safeToCompareToEmptyOrDeleted = false;
+            };
+
+            typedef SimpleClassHashTraits<CacheKey> HashTraits;
+
+        private:
+            RefPtr<StringImpl> m_source;
+            bool m_isArrowFunctionContext { false };
+        };
+
+        EvalExecutable* tryGet(bool inStrictContext, const String& evalSource, bool isArrowFunctionContext, JSScope* scope)
         {
             if (isCacheable(inStrictContext, evalSource, scope)) {
                 ASSERT(!inStrictContext);
-                SourceCodeKey sourceCodeKey(evalSource, String(), SourceCodeKey::EvalType, JSParserBuiltinMode::NotBuiltin, JSParserStrictMode::NotStrict, thisTDZMode);
-                return m_cacheMap.get(sourceCodeKey).get();
+                return m_cacheMap.fastGet(CacheKey(evalSource, isArrowFunctionContext)).get();
             }
             return nullptr;
         }
         
-        EvalExecutable* getSlow(ExecState* exec, JSCell* owner, bool inStrictContext, ThisTDZMode thisTDZMode, DerivedContextType derivedContextType, bool isArrowFunctionContext, const SourceCode& evalSource, JSScope* scope)
+        EvalExecutable* getSlow(ExecState* exec, JSCell* owner, bool inStrictContext, ThisTDZMode thisTDZMode, DerivedContextType derivedContextType, bool isArrowFunctionContext, const String& evalSource, JSScope* scope)
         {
             VariableEnvironment variablesUnderTDZ;
             JSScope::collectVariablesUnderTDZ(scope, variablesUnderTDZ);
-            EvalExecutable* evalExecutable = EvalExecutable::create(exec, evalSource, inStrictContext, thisTDZMode, derivedContextType, isArrowFunctionContext, &variablesUnderTDZ);
-
+            EvalExecutable* evalExecutable = EvalExecutable::create(exec, makeSource(evalSource), inStrictContext, thisTDZMode, derivedContextType, isArrowFunctionContext, &variablesUnderTDZ);
             if (!evalExecutable)
                 return nullptr;
 
             if (isCacheable(inStrictContext, evalSource, scope) && m_cacheMap.size() < maxCacheEntries) {
                 ASSERT(!inStrictContext);
-                SourceCodeKey sourceCodeKey(evalSource, String(), SourceCodeKey::EvalType, JSParserBuiltinMode::NotBuiltin, JSParserStrictMode::NotStrict, thisTDZMode);
-                m_cacheMap.set(sourceCodeKey, WriteBarrier<EvalExecutable>(exec->vm(), owner, evalExecutable));
+                ASSERT_WITH_MESSAGE(thisTDZMode == ThisTDZMode::CheckIfNeeded, "Always CheckIfNeeded because the caching is enabled only in the sloppy mode.");
+                ASSERT_WITH_MESSAGE(derivedContextType == DerivedContextType::None, "derivedContextType is always None because class methods and class constructors are always evaluated as the strict code.");
+                m_cacheMap.set(CacheKey(evalSource, isArrowFunctionContext), WriteBarrier<EvalExecutable>(exec->vm(), owner, evalExecutable));
             }
             
             return evalExecutable;
@@ -88,7 +131,7 @@ namespace JSC {
             return scope->isGlobalLexicalEnvironment() || scope->isFunctionNameScopeObject() || scope->isVarScope();
         }
 
-        ALWAYS_INLINE bool isCacheable(bool inStrictContext, const SourceCode& evalSource, JSScope* scope)
+        ALWAYS_INLINE bool isCacheable(bool inStrictContext, const String& evalSource, JSScope* scope)
         {
             // If eval() is called and it has access to a lexical scope, we can't soundly cache it.
             // If the eval() only has access to the "var" scope, then we can cache it.
@@ -98,7 +141,7 @@ namespace JSC {
         }
         static const int maxCacheEntries = 64;
 
-        typedef HashMap<SourceCodeKey, WriteBarrier<EvalExecutable>, SourceCodeKeyHash, SourceCodeKeyHashTraits> EvalCacheMap;
+        typedef HashMap<CacheKey, WriteBarrier<EvalExecutable>, CacheKey::Hash, CacheKey::HashTraits> EvalCacheMap;
         EvalCacheMap m_cacheMap;
     };
 
index 1a49142..bbe77ef 100644 (file)
@@ -157,12 +157,8 @@ JSValue eval(CallFrame* callFrame)
     JSScope* callerScopeChain = callerFrame->uncheckedR(callerCodeBlock->scopeRegister().offset()).Register::scope();
     UnlinkedCodeBlock* callerUnlinkedCodeBlock = callerCodeBlock->unlinkedCodeBlock();
 
-    ThisTDZMode thisTDZMode = ThisTDZMode::CheckIfNeeded;
-    if (callerUnlinkedCodeBlock->constructorKind() == ConstructorKind::Derived)
-        thisTDZMode = ThisTDZMode::AlwaysCheck;
-
-    SourceCode sourceCode(makeSource(programSource));
-    EvalExecutable* eval = callerCodeBlock->evalCodeCache().tryGet(callerCodeBlock->isStrictMode(), sourceCode, thisTDZMode, callerScopeChain);
+    bool isArrowFunctionContext = callerUnlinkedCodeBlock->isArrowFunction() || callerUnlinkedCodeBlock->isArrowFunctionContext();
+    EvalExecutable* eval = callerCodeBlock->evalCodeCache().tryGet(callerCodeBlock->isStrictMode(), programSource, isArrowFunctionContext, callerScopeChain);
 
     if (!eval) {
         if (!callerCodeBlock->isStrictMode()) {
@@ -179,18 +175,20 @@ JSValue eval(CallFrame* callFrame)
         
         // If the literal parser bailed, it should not have thrown exceptions.
         ASSERT(!callFrame->vm().exception());
-        bool isInArrowFunctionContext = callerCodeBlock->unlinkedCodeBlock()->isArrowFunction() || callerCodeBlock->unlinkedCodeBlock()->isArrowFunctionContext();
 
-        DerivedContextType derivedContextType = callerCodeBlock->unlinkedCodeBlock()->derivedContextType();
+        ThisTDZMode thisTDZMode = ThisTDZMode::CheckIfNeeded;
+        if (callerUnlinkedCodeBlock->constructorKind() == ConstructorKind::Derived)
+            thisTDZMode = ThisTDZMode::AlwaysCheck;
+
+        DerivedContextType derivedContextType = callerUnlinkedCodeBlock->derivedContextType();
         
-        if (!isInArrowFunctionContext && callerCodeBlock->unlinkedCodeBlock()->isClassContext()) {
-            derivedContextType = callerCodeBlock->unlinkedCodeBlock()->isConstructor()
+        if (!isArrowFunctionContext && callerUnlinkedCodeBlock->isClassContext()) {
+            derivedContextType = callerUnlinkedCodeBlock->isConstructor()
                 ? DerivedContextType::DerivedConstructorContext
                 : DerivedContextType::DerivedMethodContext;
         }
 
-        eval = callerCodeBlock->evalCodeCache().getSlow(callFrame, callerCodeBlock, callerCodeBlock->isStrictMode(), thisTDZMode, derivedContextType, isInArrowFunctionContext, sourceCode, callerScopeChain);
-
+        eval = callerCodeBlock->evalCodeCache().getSlow(callFrame, callerCodeBlock, callerCodeBlock->isStrictMode(), thisTDZMode, derivedContextType, isArrowFunctionContext, programSource, callerScopeChain);
         if (!eval)
             return jsUndefined();
     }
diff --git a/Source/JavaScriptCore/tests/stress/eval-in-arrow-function.js b/Source/JavaScriptCore/tests/stress/eval-in-arrow-function.js
new file mode 100644 (file)
index 0000000..0959115
--- /dev/null
@@ -0,0 +1,35 @@
+function shouldBe(actual, expected) {
+    if (actual !== expected)
+        throw new Error(`bad value: ${String(actual)}`);
+}
+
+function shouldThrow(func, errorMessage) {
+    var errorThrown = false;
+    var error = null;
+    try {
+        func();
+    } catch (e) {
+        errorThrown = true;
+        error = e;
+    }
+    if (!errorThrown)
+        throw new Error('not thrown');
+    if (String(error) !== errorMessage)
+        throw new Error(`bad error: ${String(error)}`);
+}
+
+var global = this;
+for (var i = 0; i < 100; ++i) {
+    (() => {
+        // |this| should reference to the global one.
+        shouldBe(eval("this"), global);
+    })();
+}
+
+for (var i = 0; i < 100; ++i) {
+    var THIS = {};
+    (function test() {
+        // |this| should reference to the function's one.
+        shouldBe(eval("this"), THIS);
+    }).call(THIS);
+}
index 067e728..29405f0 100644 (file)
@@ -1,3 +1,15 @@
+2016-03-29  Yusuke Suzuki  <utatane.tea@gmail.com>
+
+        REGRESSION(r192914): 10% regression on Sunspider's date-format-tofte
+        https://bugs.webkit.org/show_bug.cgi?id=155559
+
+        Reviewed by Saam Barati.
+
+        Add HashTable::inlineLookup and HashMap::fastGet.
+
+        * wtf/HashMap.h:
+        * wtf/HashTable.h:
+
 2016-03-25  Alex Christensen  <achristensen@webkit.org>
 
         Add a compile time flag for using QTKit
index 070fdeb..2c734f3 100644 (file)
@@ -102,6 +102,9 @@ public:
     bool contains(const KeyType&) const;
     MappedPeekType get(const KeyType&) const;
 
+    // Same as get(), but aggressively inlined.
+    MappedPeekType fastGet(const KeyType&) const;
+
     // Replaces the value but not the key if the key is already present.
     // Return value includes both an iterator to the key location,
     // and an isNewEntry boolean that's true if a new entry was added.
@@ -392,6 +395,15 @@ auto HashMap<T, U, V, W, MappedTraits>::get(const KeyType& key) const -> MappedP
     return MappedTraits::peek(entry->value);
 }
 
+template<typename T, typename U, typename V, typename W, typename MappedTraits>
+ALWAYS_INLINE auto HashMap<T, U, V, W, MappedTraits>::fastGet(const KeyType& key) const -> MappedPeekType
+{
+    KeyValuePairType* entry = const_cast<HashTableType&>(m_impl).template inlineLookup<typename HashTableType::IdentityTranslatorType>(key);
+    if (!entry)
+        return MappedTraits::peek(MappedTraits::emptyValue());
+    return MappedTraits::peek(entry->value);
+}
+
 template<typename T, typename U, typename V, typename W, typename X>
 inline bool HashMap<T, U, V, W, X>::remove(iterator it)
 {
@@ -457,7 +469,7 @@ template<typename T, typename U, typename V, typename W, typename X>
 template<typename K>
 inline auto HashMap<T, U, V, W, X>::inlineGet(typename GetPtrHelper<K>::PtrType key) const -> typename std::enable_if<IsSmartPtr<K>::value, MappedPeekType>::type
 {
-    KeyValuePairType* entry = const_cast<HashTableType&>(m_impl).template lookup<HashMapTranslator<KeyValuePairTraits, HashFunctions>>(key);
+    KeyValuePairType* entry = const_cast<HashTableType&>(m_impl).template inlineLookup<HashMapTranslator<KeyValuePairTraits, HashFunctions>>(key);
     if (!entry)
         return MappedTraits::peek(MappedTraits::emptyValue());
     return MappedTraits::peek(entry->value);
index 7169b27..cf75472 100644 (file)
@@ -412,6 +412,7 @@ namespace WTF {
 
         ValueType* lookup(const Key& key) { return lookup<IdentityTranslatorType>(key); }
         template<typename HashTranslator, typename T> ValueType* lookup(const T&);
+        template<typename HashTranslator, typename T> ValueType* inlineLookup(const T&);
 
 #if !ASSERT_DISABLED
         void checkTableConsistency() const;
@@ -595,6 +596,13 @@ namespace WTF {
     template<typename HashTranslator, typename T>
     inline auto HashTable<Key, Value, Extractor, HashFunctions, Traits, KeyTraits>::lookup(const T& key) -> ValueType*
     {
+        return inlineLookup<HashTranslator>(key);
+    }
+
+    template<typename Key, typename Value, typename Extractor, typename HashFunctions, typename Traits, typename KeyTraits>
+    template<typename HashTranslator, typename T>
+    ALWAYS_INLINE auto HashTable<Key, Value, Extractor, HashFunctions, Traits, KeyTraits>::inlineLookup(const T& key) -> ValueType*
+    {
         checkKey<HashTranslator>(key);
 
         unsigned k = 0;