Changeset 198778 in webkit


Ignore:
Timestamp:
Mar 29, 2016, 3:25:44 AM (9 years ago)
Author:
Yusuke Suzuki
Message:

REGRESSION(r192914): 10% regression on Sunspider's date-format-tofte
​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:
Location:
trunk/Source
Files:
1 added
6 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/JavaScriptCore/ChangeLog

    r198776 r198778  
     12016-03-29  Yusuke Suzuki  <utatane.tea@gmail.com>
     2
     3        REGRESSION(r192914): 10% regression on Sunspider's date-format-tofte
     4        https://bugs.webkit.org/show_bug.cgi?id=155559
     5
     6        Reviewed by Saam Barati.
     7
     8        The fast path of the eval function is the super hot path in date-format-tofte.
     9        Any performance regression is not allowed here.
     10        Before this patch, we allocated SourceCode in the fast path.
     11        This allocation incurs 10% performance regression.
     12
     13        This patch removes this allocation in the fast path.
     14        And change the key of the EvalCodeCache to EvalCodeCache::CacheKey.
     15        It combines RefPtr<StringImpl> and isArrowFunctionContext.
     16        Since EvalCodeCache does not cache any eval code evaluated under the strict mode,
     17        it is unnecessary to include several options (ThisTDZMode, and DerivedContextType) in the cache map's key.
     18        But isArrowFunctionContext is necessary since the sloppy mode arrow function exists.
     19
     20        To validate this change, we add a new test that evaluates the same code
     21        under the non-arrow function context and the arrow function context.
     22
     23        After introducing CacheKey, we observed 1% regression compared to the RefPtr<StringImpl> keyed case.
     24        This is because HashMap<RefPtr<T>, ...>::get(T*) is specially optimized; this path is inlined while the normal ::get() is not inlined.
     25        To avoid this performance regression, we introduce HashMap::fastGet, that aggressively encourages inlining.
     26        The relationship between fastGet() and get() is similar to fastAdd() and add().
     27        After applying this change, the evaluation shows no performance regression in comparison with the RefPtr<StringImpl> keyed case.
     28
     29        * bytecode/EvalCodeCache.h:
     30        (JSC::EvalCodeCache::CacheKey::CacheKey):
     31        (JSC::EvalCodeCache::CacheKey::hash):
     32        (JSC::EvalCodeCache::CacheKey::isEmptyValue):
     33        (JSC::EvalCodeCache::CacheKey::operator==):
     34        (JSC::EvalCodeCache::CacheKey::isHashTableDeletedValue):
     35        (JSC::EvalCodeCache::CacheKey::Hash::hash):
     36        (JSC::EvalCodeCache::CacheKey::Hash::equal):
     37        (JSC::EvalCodeCache::tryGet):
     38        (JSC::EvalCodeCache::getSlow):
     39        (JSC::EvalCodeCache::isCacheable):
     40        * interpreter/Interpreter.cpp:
     41        (JSC::eval):
     42        * tests/stress/eval-in-arrow-function.js: Added.
     43        (shouldBe):
     44        (i):
     45
    1462016-03-29  Joseph Pecoraro  <pecoraro@apple.com>
    247

  • trunk/Source/JavaScriptCore/bytecode/EvalCodeCache.h

    r194449 r198778  
    3535#include "Options.h"
    3636#include "SourceCode.h"
    37 #include "SourceCodeKey.h"
    3837#include <wtf/HashMap.h>
    3938#include <wtf/RefPtr.h>
    … …  
    4645    class EvalCodeCache {
    4746    public:
    48         EvalExecutable* tryGet(bool inStrictContext, const SourceCode& evalSource, ThisTDZMode thisTDZMode, JSScope* scope)
     47        class CacheKey {
     48        public:
     49            CacheKey(const String& source, bool isArrowFunctionContext)
     50                : m_source(source.impl())
     51                , m_isArrowFunctionContext(isArrowFunctionContext)
     52            {
     53            }
     54
     55            CacheKey(WTF::HashTableDeletedValueType)
     56                : m_source(WTF::HashTableDeletedValue)
     57            {
     58            }
     59
     60            CacheKey() = default;
     61
     62            unsigned hash() const { return m_source->hash(); }
     63
     64            bool isEmptyValue() const { return !m_source; }
     65
     66            bool operator==(const CacheKey& other) const
     67            {
     68                return m_source == other.m_source && m_isArrowFunctionContext == other.m_isArrowFunctionContext;
     69            }
     70
     71            bool isHashTableDeletedValue() const { return m_source.isHashTableDeletedValue(); }
     72
     73            struct Hash {
     74                static unsigned hash(const CacheKey& key)
     75                {
     76                    return key.hash();
     77                }
     78                static bool equal(const CacheKey& lhs, const CacheKey& rhs)
     79                {
     80                    return StringHash::equal(lhs.m_source, rhs.m_source) && lhs.m_isArrowFunctionContext == rhs.m_isArrowFunctionContext;
     81                }
     82                static const bool safeToCompareToEmptyOrDeleted = false;
     83            };
     84
     85            typedef SimpleClassHashTraits<CacheKey> HashTraits;
     86
     87        private:
     88            RefPtr<StringImpl> m_source;
     89            bool m_isArrowFunctionContext { false };
     90        };
     91
     92        EvalExecutable* tryGet(bool inStrictContext, const String& evalSource, bool isArrowFunctionContext, JSScope* scope)
    4993        {
    5094            if (isCacheable(inStrictContext, evalSource, scope)) {
    5195                ASSERT(!inStrictContext);
    52                 SourceCodeKey sourceCodeKey(evalSource, String(), SourceCodeKey::EvalType, JSParserBuiltinMode::NotBuiltin, JSParserStrictMode::NotStrict, thisTDZMode);
    53                 return m_cacheMap.get(sourceCodeKey).get();
     96                return m_cacheMap.fastGet(CacheKey(evalSource, isArrowFunctionContext)).get();
    5497            }
    5598            return nullptr;
    5699        }
    57100       
    58         EvalExecutable* getSlow(ExecState* exec, JSCell* owner, bool inStrictContext, ThisTDZMode thisTDZMode, DerivedContextType derivedContextType, bool isArrowFunctionContext, const SourceCode& evalSource, JSScope* scope)
     101        EvalExecutable* getSlow(ExecState* exec, JSCell* owner, bool inStrictContext, ThisTDZMode thisTDZMode, DerivedContextType derivedContextType, bool isArrowFunctionContext, const String& evalSource, JSScope* scope)
    59102        {
    60103            VariableEnvironment variablesUnderTDZ;
    61104            JSScope::collectVariablesUnderTDZ(scope, variablesUnderTDZ);
    62             EvalExecutable* evalExecutable = EvalExecutable::create(exec, evalSource, inStrictContext, thisTDZMode, derivedContextType, isArrowFunctionContext, &variablesUnderTDZ);
    63 
     105            EvalExecutable* evalExecutable = EvalExecutable::create(exec, makeSource(evalSource), inStrictContext, thisTDZMode, derivedContextType, isArrowFunctionContext, &variablesUnderTDZ);
    64106            if (!evalExecutable)
    65107                return nullptr;
    … …  
    67109            if (isCacheable(inStrictContext, evalSource, scope) && m_cacheMap.size() < maxCacheEntries) {
    68110                ASSERT(!inStrictContext);
    69                 SourceCodeKey sourceCodeKey(evalSource, String(), SourceCodeKey::EvalType, JSParserBuiltinMode::NotBuiltin, JSParserStrictMode::NotStrict, thisTDZMode);
    70                 m_cacheMap.set(sourceCodeKey, WriteBarrier<EvalExecutable>(exec->vm(), owner, evalExecutable));
     111                ASSERT_WITH_MESSAGE(thisTDZMode == ThisTDZMode::CheckIfNeeded, "Always CheckIfNeeded because the caching is enabled only in the sloppy mode.");
     112                ASSERT_WITH_MESSAGE(derivedContextType == DerivedContextType::None, "derivedContextType is always None because class methods and class constructors are always evaluated as the strict code.");
     113                m_cacheMap.set(CacheKey(evalSource, isArrowFunctionContext), WriteBarrier<EvalExecutable>(exec->vm(), owner, evalExecutable));
    71114            }
    72115           
    … …  
    89132        }
    90133
    91         ALWAYS_INLINE bool isCacheable(bool inStrictContext, const SourceCode& evalSource, JSScope* scope)
     134        ALWAYS_INLINE bool isCacheable(bool inStrictContext, const String& evalSource, JSScope* scope)
    92135        {
    93136            // If eval() is called and it has access to a lexical scope, we can't soundly cache it.
    … …  
    99142        static const int maxCacheEntries = 64;
    100143
    101         typedef HashMap<SourceCodeKey, WriteBarrier<EvalExecutable>, SourceCodeKeyHash, SourceCodeKeyHashTraits> EvalCacheMap;
     144        typedef HashMap<CacheKey, WriteBarrier<EvalExecutable>, CacheKey::Hash, CacheKey::HashTraits> EvalCacheMap;
    102145        EvalCacheMap m_cacheMap;
    103146    };

  • trunk/Source/JavaScriptCore/interpreter/Interpreter.cpp

    r198364 r198778  
    158158    UnlinkedCodeBlock* callerUnlinkedCodeBlock = callerCodeBlock->unlinkedCodeBlock();
    159159
    160     ThisTDZMode thisTDZMode = ThisTDZMode::CheckIfNeeded;
    161     if (callerUnlinkedCodeBlock->constructorKind() == ConstructorKind::Derived)
    162         thisTDZMode = ThisTDZMode::AlwaysCheck;
    163 
    164     SourceCode sourceCode(makeSource(programSource));
    165     EvalExecutable* eval = callerCodeBlock->evalCodeCache().tryGet(callerCodeBlock->isStrictMode(), sourceCode, thisTDZMode, callerScopeChain);
     160    bool isArrowFunctionContext = callerUnlinkedCodeBlock->isArrowFunction() || callerUnlinkedCodeBlock->isArrowFunctionContext();
     161    EvalExecutable* eval = callerCodeBlock->evalCodeCache().tryGet(callerCodeBlock->isStrictMode(), programSource, isArrowFunctionContext, callerScopeChain);
    166162
    167163    if (!eval) {
    … …  
    180176        // If the literal parser bailed, it should not have thrown exceptions.
    181177        ASSERT(!callFrame->vm().exception());
    182         bool isInArrowFunctionContext = callerCodeBlock->unlinkedCodeBlock()->isArrowFunction() || callerCodeBlock->unlinkedCodeBlock()->isArrowFunctionContext();
    183 
    184         DerivedContextType derivedContextType = callerCodeBlock->unlinkedCodeBlock()->derivedContextType();
     178
     179        ThisTDZMode thisTDZMode = ThisTDZMode::CheckIfNeeded;
     180        if (callerUnlinkedCodeBlock->constructorKind() == ConstructorKind::Derived)
     181            thisTDZMode = ThisTDZMode::AlwaysCheck;
     182
     183        DerivedContextType derivedContextType = callerUnlinkedCodeBlock->derivedContextType();
    185184       
    186         if (!isInArrowFunctionContext && callerCodeBlock->unlinkedCodeBlock()->isClassContext()) {
    187             derivedContextType = callerCodeBlock->unlinkedCodeBlock()->isConstructor()
     185        if (!isArrowFunctionContext && callerUnlinkedCodeBlock->isClassContext()) {
     186            derivedContextType = callerUnlinkedCodeBlock->isConstructor()
    188187                ? DerivedContextType::DerivedConstructorContext
    189188                : DerivedContextType::DerivedMethodContext;
    190189        }
    191190
    192         eval = callerCodeBlock->evalCodeCache().getSlow(callFrame, callerCodeBlock, callerCodeBlock->isStrictMode(), thisTDZMode, derivedContextType, isInArrowFunctionContext, sourceCode, callerScopeChain);
    193 
     191        eval = callerCodeBlock->evalCodeCache().getSlow(callFrame, callerCodeBlock, callerCodeBlock->isStrictMode(), thisTDZMode, derivedContextType, isArrowFunctionContext, programSource, callerScopeChain);
    194192        if (!eval)
    195193            return jsUndefined();

  • trunk/Source/WTF/ChangeLog

    r198673 r198778  
     12016-03-29  Yusuke Suzuki  <utatane.tea@gmail.com>
     2
     3        REGRESSION(r192914): 10% regression on Sunspider's date-format-tofte
     4        https://bugs.webkit.org/show_bug.cgi?id=155559
     5
     6        Reviewed by Saam Barati.
     7
     8        Add HashTable::inlineLookup and HashMap::fastGet.
     9
     10        * wtf/HashMap.h:
     11        * wtf/HashTable.h:
     12
    1132016-03-25  Alex Christensen  <achristensen@webkit.org>
    214

  • trunk/Source/WTF/wtf/HashMap.h

    r197123 r198778  
    103103    MappedPeekType get(const KeyType&) const;
    104104
     105    // Same as get(), but aggressively inlined.
     106    MappedPeekType fastGet(const KeyType&) const;
     107
    105108    // Replaces the value but not the key if the key is already present.
    106109    // Return value includes both an iterator to the key location,
    … …  
    393396}
    394397
     398template<typename T, typename U, typename V, typename W, typename MappedTraits>
     399ALWAYS_INLINE auto HashMap<T, U, V, W, MappedTraits>::fastGet(const KeyType& key) const -> MappedPeekType
     400{
     401    KeyValuePairType* entry = const_cast<HashTableType&>(m_impl).template inlineLookup<typename HashTableType::IdentityTranslatorType>(key);
     402    if (!entry)
     403        return MappedTraits::peek(MappedTraits::emptyValue());
     404    return MappedTraits::peek(entry->value);
     405}
     406
    395407template<typename T, typename U, typename V, typename W, typename X>
    396408inline bool HashMap<T, U, V, W, X>::remove(iterator it)
    … …  
    458470inline auto HashMap<T, U, V, W, X>::inlineGet(typename GetPtrHelper<K>::PtrType key) const -> typename std::enable_if<IsSmartPtr<K>::value, MappedPeekType>::type
    459471{
    460     KeyValuePairType* entry = const_cast<HashTableType&>(m_impl).template lookup<HashMapTranslator<KeyValuePairTraits, HashFunctions>>(key);
     472    KeyValuePairType* entry = const_cast<HashTableType&>(m_impl).template inlineLookup<HashMapTranslator<KeyValuePairTraits, HashFunctions>>(key);
    461473    if (!entry)
    462474        return MappedTraits::peek(MappedTraits::emptyValue());

  • trunk/Source/WTF/wtf/HashTable.h

    r195629 r198778  
    413413        ValueType* lookup(const Key& key) { return lookup<IdentityTranslatorType>(key); }
    414414        template<typename HashTranslator, typename T> ValueType* lookup(const T&);
     415        template<typename HashTranslator, typename T> ValueType* inlineLookup(const T&);
    415416
    416417#if !ASSERT_DISABLED
    … …  
    595596    template<typename HashTranslator, typename T>
    596597    inline auto HashTable<Key, Value, Extractor, HashFunctions, Traits, KeyTraits>::lookup(const T& key) -> ValueType*
     598    {
     599        return inlineLookup<HashTranslator>(key);
     600    }
     601
     602    template<typename Key, typename Value, typename Extractor, typename HashFunctions, typename Traits, typename KeyTraits>
     603    template<typename HashTranslator, typename T>
     604    ALWAYS_INLINE auto HashTable<Key, Value, Extractor, HashFunctions, Traits, KeyTraits>::inlineLookup(const T& key) -> ValueType*
    597605    {
    598606        checkKey<HashTranslator>(key);
Note: See TracChangeset for help on using the changeset viewer.