Cache the results of BytecodeGenerator::getVariablesUnderTDZ
authorsbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 15 Feb 2019 00:06:30 +0000 (00:06 +0000)
committersbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 15 Feb 2019 00:06:30 +0000 (00:06 +0000)
https://bugs.webkit.org/show_bug.cgi?id=194583
<rdar://problem/48028140>

Reviewed by Yusuke Suzuki.

JSTests:

* microbenchmarks/cache-get-variables-under-tdz-in-bytecode-generator.js: Added.

Source/JavaScriptCore:

This patch makes it so that getVariablesUnderTDZ caches a result of
CompactVariableMap::Handle. getVariablesUnderTDZ is costly when
it's called in an environment where there are a lot of variables.
This patch makes it so we cache its results. This is profitable when
getVariablesUnderTDZ is called repeatedly with the same environment
state. This is common since we call this every time we encounter a
function definition/expression node.

* builtins/BuiltinExecutables.cpp:
(JSC::BuiltinExecutables::createExecutable):
* bytecode/UnlinkedFunctionExecutable.cpp:
(JSC::UnlinkedFunctionExecutable::UnlinkedFunctionExecutable):
* bytecode/UnlinkedFunctionExecutable.h:
* bytecompiler/BytecodeGenerator.cpp:
(JSC::BytecodeGenerator::popLexicalScopeInternal):
(JSC::BytecodeGenerator::liftTDZCheckIfPossible):
(JSC::BytecodeGenerator::pushTDZVariables):
(JSC::BytecodeGenerator::getVariablesUnderTDZ):
(JSC::BytecodeGenerator::restoreTDZStack):
* bytecompiler/BytecodeGenerator.h:
(JSC::BytecodeGenerator::makeFunction):
* parser/VariableEnvironment.cpp:
(JSC::CompactVariableMap::Handle::Handle):
(JSC::CompactVariableMap::Handle::operator=):
* parser/VariableEnvironment.h:
(JSC::CompactVariableMap::Handle::operator bool const):
* runtime/CodeCache.cpp:
(JSC::CodeCache::getUnlinkedGlobalFunctionExecutable):

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

JSTests/ChangeLog
JSTests/microbenchmarks/cache-get-variables-under-tdz-in-bytecode-generator.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/builtins/BuiltinExecutables.cpp
Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.cpp
Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h
Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp
Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h
Source/JavaScriptCore/parser/VariableEnvironment.cpp
Source/JavaScriptCore/parser/VariableEnvironment.h
Source/JavaScriptCore/runtime/CodeCache.cpp

index 86ee206..2ea089b 100644 (file)
@@ -1,3 +1,13 @@
+2019-02-14  Saam Barati  <sbarati@apple.com>
+
+        Cache the results of BytecodeGenerator::getVariablesUnderTDZ
+        https://bugs.webkit.org/show_bug.cgi?id=194583
+        <rdar://problem/48028140>
+
+        Reviewed by Yusuke Suzuki.
+
+        * microbenchmarks/cache-get-variables-under-tdz-in-bytecode-generator.js: Added.
+
 2019-02-08  Yusuke Suzuki  <ysuzuki@apple.com>
 
         [JSC] String.fromCharCode's slow path always generates 16bit string
diff --git a/JSTests/microbenchmarks/cache-get-variables-under-tdz-in-bytecode-generator.js b/JSTests/microbenchmarks/cache-get-variables-under-tdz-in-bytecode-generator.js
new file mode 100644 (file)
index 0000000..dc189d6
--- /dev/null
@@ -0,0 +1,20 @@
+//@ runDefault
+
+let script = "(() => {";
+for (let i = 0; i < 1000; ++i) {
+    script += `let x${i} = ${i};\n`;
+}
+
+for (let i = 0; i < 1000; ++i) {
+    script += `function f${i}() { return "foo"; }\n`;
+}
+
+script += "})();"
+
+let start = Date.now();
+for (let i = 0; i < 10; ++i) {
+    $.evalScript(`cacheBust = ${Math.random()}; ${script}`);
+}
+
+if (false)
+    print(Date.now() - start);
index a81b992..0bf3b11 100644 (file)
@@ -1,3 +1,40 @@
+2019-02-14  Saam Barati  <sbarati@apple.com>
+
+        Cache the results of BytecodeGenerator::getVariablesUnderTDZ
+        https://bugs.webkit.org/show_bug.cgi?id=194583
+        <rdar://problem/48028140>
+
+        Reviewed by Yusuke Suzuki.
+
+        This patch makes it so that getVariablesUnderTDZ caches a result of
+        CompactVariableMap::Handle. getVariablesUnderTDZ is costly when
+        it's called in an environment where there are a lot of variables.
+        This patch makes it so we cache its results. This is profitable when
+        getVariablesUnderTDZ is called repeatedly with the same environment
+        state. This is common since we call this every time we encounter a
+        function definition/expression node.
+
+        * builtins/BuiltinExecutables.cpp:
+        (JSC::BuiltinExecutables::createExecutable):
+        * bytecode/UnlinkedFunctionExecutable.cpp:
+        (JSC::UnlinkedFunctionExecutable::UnlinkedFunctionExecutable):
+        * bytecode/UnlinkedFunctionExecutable.h:
+        * bytecompiler/BytecodeGenerator.cpp:
+        (JSC::BytecodeGenerator::popLexicalScopeInternal):
+        (JSC::BytecodeGenerator::liftTDZCheckIfPossible):
+        (JSC::BytecodeGenerator::pushTDZVariables):
+        (JSC::BytecodeGenerator::getVariablesUnderTDZ):
+        (JSC::BytecodeGenerator::restoreTDZStack):
+        * bytecompiler/BytecodeGenerator.h:
+        (JSC::BytecodeGenerator::makeFunction):
+        * parser/VariableEnvironment.cpp:
+        (JSC::CompactVariableMap::Handle::Handle):
+        (JSC::CompactVariableMap::Handle::operator=):
+        * parser/VariableEnvironment.h:
+        (JSC::CompactVariableMap::Handle::operator bool const):
+        * runtime/CodeCache.cpp:
+        (JSC::CodeCache::getUnlinkedGlobalFunctionExecutable):
+
 2019-02-14  Yusuke Suzuki  <ysuzuki@apple.com>
 
         [JSC] Non-JIT entrypoints should share NativeJITCode per entrypoint type
index de47093..2d6fe0e 100644 (file)
@@ -258,7 +258,7 @@ UnlinkedFunctionExecutable* BuiltinExecutables::createExecutable(VM& vm, const S
     }
 
     VariableEnvironment dummyTDZVariables;
-    UnlinkedFunctionExecutable* functionExecutable = UnlinkedFunctionExecutable::create(&vm, source, &metadata, kind, constructAbility, JSParserScriptMode::Classic, dummyTDZVariables, DerivedContextType::None, isBuiltinDefaultClassConstructor);
+    UnlinkedFunctionExecutable* functionExecutable = UnlinkedFunctionExecutable::create(&vm, source, &metadata, kind, constructAbility, JSParserScriptMode::Classic, vm.m_compactVariableMap->get(dummyTDZVariables), DerivedContextType::None, isBuiltinDefaultClassConstructor);
     return functionExecutable;
 }
 
index b53f64c..ce71c6f 100644 (file)
@@ -78,7 +78,7 @@ static UnlinkedFunctionCodeBlock* generateUnlinkedFunctionCodeBlock(
     return result;
 }
 
-UnlinkedFunctionExecutable::UnlinkedFunctionExecutable(VM* vm, Structure* structure, const SourceCode& parentSource, FunctionMetadataNode* node, UnlinkedFunctionKind kind, ConstructAbility constructAbility, JSParserScriptMode scriptMode, VariableEnvironment& parentScopeTDZVariables, DerivedContextType derivedContextType, bool isBuiltinDefaultClassConstructor)
+UnlinkedFunctionExecutable::UnlinkedFunctionExecutable(VM* vm, Structure* structure, const SourceCode& parentSource, FunctionMetadataNode* node, UnlinkedFunctionKind kind, ConstructAbility constructAbility, JSParserScriptMode scriptMode, CompactVariableMap::Handle parentScopeTDZVariables, DerivedContextType derivedContextType, bool isBuiltinDefaultClassConstructor)
     : Base(*vm, structure)
     , m_firstLineOffset(node->firstLine() - parentSource.firstLine().oneBasedInt())
     , m_lineCount(node->lastLine() - node->firstLine())
@@ -107,7 +107,7 @@ UnlinkedFunctionExecutable::UnlinkedFunctionExecutable(VM* vm, Structure* struct
     , m_ecmaName(node->ecmaName())
     , m_inferredName(node->inferredName())
     , m_classSource(node->classSource())
-    , m_parentScopeTDZVariables(vm->m_compactVariableMap->get(parentScopeTDZVariables))
+    , m_parentScopeTDZVariables(WTFMove(parentScopeTDZVariables))
 {
     // Make sure these bitfields are adequately wide.
     ASSERT(m_constructAbility == static_cast<unsigned>(constructAbility));
index 9982d34..7738152 100644 (file)
@@ -67,7 +67,7 @@ public:
         return &vm.unlinkedFunctionExecutableSpace.space;
     }
 
-    static UnlinkedFunctionExecutable* create(VM* vm, const SourceCode& source, FunctionMetadataNode* node, UnlinkedFunctionKind unlinkedFunctionKind, ConstructAbility constructAbility, JSParserScriptMode scriptMode, VariableEnvironment& parentScopeTDZVariables, DerivedContextType derivedContextType, bool isBuiltinDefaultClassConstructor = false)
+    static UnlinkedFunctionExecutable* create(VM* vm, const SourceCode& source, FunctionMetadataNode* node, UnlinkedFunctionKind unlinkedFunctionKind, ConstructAbility constructAbility, JSParserScriptMode scriptMode, CompactVariableMap::Handle parentScopeTDZVariables, DerivedContextType derivedContextType, bool isBuiltinDefaultClassConstructor = false)
     {
         UnlinkedFunctionExecutable* instance = new (NotNull, allocateCell<UnlinkedFunctionExecutable>(vm->heap))
             UnlinkedFunctionExecutable(vm, vm->unlinkedFunctionExecutableStructure.get(), source, node, unlinkedFunctionKind, constructAbility, scriptMode, parentScopeTDZVariables, derivedContextType, isBuiltinDefaultClassConstructor);
@@ -152,7 +152,7 @@ public:
     void setSourceMappingURLDirective(const String& sourceMappingURL) { m_sourceMappingURLDirective = sourceMappingURL; }
 
 private:
-    UnlinkedFunctionExecutable(VM*, Structure*, const SourceCode&, FunctionMetadataNode*, UnlinkedFunctionKind, ConstructAbility, JSParserScriptMode, VariableEnvironment&,  JSC::DerivedContextType, bool isBuiltinDefaultClassConstructor);
+    UnlinkedFunctionExecutable(VM*, Structure*, const SourceCode&, FunctionMetadataNode*, UnlinkedFunctionKind, ConstructAbility, JSParserScriptMode, CompactVariableMap::Handle,  JSC::DerivedContextType, bool isBuiltinDefaultClassConstructor);
     UnlinkedFunctionExecutable(Decoder&, VariableEnvironment&, const CachedFunctionExecutable&);
 
     unsigned m_firstLineOffset;
index cc76f23..c1acc41 100644 (file)
@@ -2195,6 +2195,7 @@ void BytecodeGenerator::popLexicalScopeInternal(VariableEnvironment& environment
     }
 
     m_TDZStack.removeLast();
+    m_cachedVariablesUnderTDZ = { };
 }
 
 void BytecodeGenerator::prepareLexicalScopeForNextForLoopIteration(VariableEnvironmentNode* node, RegisterID* loopSymbolTable)
@@ -2814,8 +2815,10 @@ void BytecodeGenerator::liftTDZCheckIfPossible(const Variable& variable)
     for (unsigned i = m_TDZStack.size(); i--;) {
         auto iter = m_TDZStack[i].find(identifier);
         if (iter != m_TDZStack[i].end()) {
-            if (iter->value == TDZNecessityLevel::Optimize)
+            if (iter->value == TDZNecessityLevel::Optimize) {
+                m_cachedVariablesUnderTDZ = { };
                 iter->value = TDZNecessityLevel::NotNeeded;
+            }
             break;
         }
     }
@@ -2840,10 +2843,14 @@ void BytecodeGenerator::pushTDZVariables(const VariableEnvironment& environment,
         map.add(entry.key, entry.value.isFunction() ? TDZNecessityLevel::NotNeeded : level);
 
     m_TDZStack.append(WTFMove(map));
+    m_cachedVariablesUnderTDZ = { };
 }
 
-void BytecodeGenerator::getVariablesUnderTDZ(VariableEnvironment& result)
+CompactVariableMap::Handle BytecodeGenerator::getVariablesUnderTDZ()
 {
+    if (m_cachedVariablesUnderTDZ)
+        return m_cachedVariablesUnderTDZ;
+
     // We keep track of variablesThatDontNeedTDZ in this algorithm to prevent
     // reporting that "x" is under TDZ if this function is called at "...".
     //
@@ -2854,18 +2861,21 @@ void BytecodeGenerator::getVariablesUnderTDZ(VariableEnvironment& result)
     //         }
     //         let x;
     //     }
-    //
     SmallPtrSet<UniquedStringImpl*, 16> variablesThatDontNeedTDZ;
+    VariableEnvironment environment;
     for (unsigned i = m_TDZStack.size(); i--; ) {
         auto& map = m_TDZStack[i];
         for (auto& entry : map)  {
             if (entry.value != TDZNecessityLevel::NotNeeded) {
                 if (!variablesThatDontNeedTDZ.contains(entry.key.get()))
-                    result.add(entry.key.get());
+                    environment.add(entry.key.get());
             } else
                 variablesThatDontNeedTDZ.add(entry.key.get());
         }
     }
+
+    m_cachedVariablesUnderTDZ = m_vm->m_compactVariableMap->get(environment);
+    return m_cachedVariablesUnderTDZ;
 }
 
 void BytecodeGenerator::preserveTDZStack(BytecodeGenerator::PreservedTDZStack& preservedStack)
@@ -2876,6 +2886,7 @@ void BytecodeGenerator::preserveTDZStack(BytecodeGenerator::PreservedTDZStack& p
 void BytecodeGenerator::restoreTDZStack(const BytecodeGenerator::PreservedTDZStack& preservedStack)
 {
     m_TDZStack = preservedStack.m_preservedTDZStack;
+    m_cachedVariablesUnderTDZ = { };
 }
 
 RegisterID* BytecodeGenerator::emitNewObject(RegisterID* dst)
index e31f06e..b0ac06f 100644 (file)
@@ -1137,8 +1137,7 @@ namespace JSC {
                     newDerivedContextType = DerivedContextType::DerivedMethodContext;
             }
 
-            VariableEnvironment variablesUnderTDZ;
-            getVariablesUnderTDZ(variablesUnderTDZ);
+            CompactVariableMap::Handle variablesUnderTDZ = getVariablesUnderTDZ();
 
             // FIXME: These flags, ParserModes and propagation to XXXCodeBlocks should be reorganized.
             // https://bugs.webkit.org/show_bug.cgi?id=151547
@@ -1147,10 +1146,10 @@ namespace JSC {
             if (parseMode == SourceParseMode::MethodMode && metadata->constructorKind() != ConstructorKind::None)
                 constructAbility = ConstructAbility::CanConstruct;
 
-            return UnlinkedFunctionExecutable::create(m_vm, m_scopeNode->source(), metadata, isBuiltinFunction() ? UnlinkedBuiltinFunction : UnlinkedNormalFunction, constructAbility, scriptMode(), variablesUnderTDZ, newDerivedContextType);
+            return UnlinkedFunctionExecutable::create(m_vm, m_scopeNode->source(), metadata, isBuiltinFunction() ? UnlinkedBuiltinFunction : UnlinkedNormalFunction, constructAbility, scriptMode(), WTFMove(variablesUnderTDZ), newDerivedContextType);
         }
 
-        void getVariablesUnderTDZ(VariableEnvironment&);
+        CompactVariableMap::Handle getVariablesUnderTDZ();
 
         RegisterID* emitConstructVarargs(RegisterID* dst, RegisterID* func, RegisterID* thisRegister, RegisterID* arguments, RegisterID* firstFreeRegister, int32_t firstVarArgOffset, const JSTextPosition& divot, const JSTextPosition& divotStart, const JSTextPosition& divotEnd, DebuggableCall);
         template<typename CallOp>
@@ -1313,6 +1312,8 @@ namespace JSC {
         bool m_needsToUpdateArrowFunctionContext;
         DerivedContextType m_derivedContextType { DerivedContextType::None };
 
+        CompactVariableMap::Handle m_cachedVariablesUnderTDZ;
+
         using CatchEntry = std::tuple<TryData*, VirtualRegister, VirtualRegister>;
         Vector<CatchEntry> m_catchesToEmit;
     };
index a395ff5..a93ae7a 100644 (file)
@@ -183,4 +183,27 @@ CompactVariableMap::Handle::~Handle()
     }
 }
 
+CompactVariableMap::Handle::Handle(const CompactVariableMap::Handle& other)
+{
+    *this = other;
+}
+
+CompactVariableMap::Handle& CompactVariableMap::Handle::operator=(const Handle& other)
+{
+    m_map = other.m_map;
+    m_environment = other.m_environment;
+
+    if (!m_map) {
+        ASSERT(!m_environment);
+        // This happens if `other` were moved into a different handle.
+        return *this;
+    }
+
+    auto iter = m_map->m_map.find(CompactVariableMapKey { *m_environment });
+    RELEASE_ASSERT(iter != m_map->m_map.end());
+    ++iter->value;
+
+    return *this;
+}
+
 } // namespace JSC
index d84b2e1..ee6cce6 100644 (file)
@@ -204,8 +204,9 @@ namespace JSC {
 class CompactVariableMap : public RefCounted<CompactVariableMap> {
 public:
     class Handle {
-        WTF_MAKE_NONCOPYABLE(Handle); // If we wanted to make this copyable, we'd need to do a hashtable lookup and bump the reference count of the map entry.
     public:
+        Handle() = default;
+
         Handle(CompactVariableEnvironment& environment, CompactVariableMap& map)
             : m_environment(&environment)
             , m_map(&map)
@@ -218,15 +219,21 @@ public:
             ASSERT(!other.m_map);
             other.m_environment = nullptr;
         }
+
+        Handle(const Handle&);
+        Handle& operator=(const Handle&);
+
         ~Handle();
 
+        explicit operator bool() const { return !!m_map; }
+
         const CompactVariableEnvironment& environment() const
         {
             return *m_environment;
         }
 
     private:
-        CompactVariableEnvironment* m_environment;
+        CompactVariableEnvironment* m_environment { nullptr };
         RefPtr<CompactVariableMap> m_map;
     };
 
index a0f20d4..9072aaf 100644 (file)
@@ -147,7 +147,7 @@ UnlinkedFunctionExecutable* CodeCache::getUnlinkedGlobalFunctionExecutable(VM& v
     // in the global lexical environment, which we always TDZ check accesses from.
     VariableEnvironment emptyTDZVariables;
     ConstructAbility constructAbility = constructAbilityForParseMode(metadata->parseMode());
-    UnlinkedFunctionExecutable* functionExecutable = UnlinkedFunctionExecutable::create(&vm, source, metadata, UnlinkedNormalFunction, constructAbility, JSParserScriptMode::Classic, emptyTDZVariables, DerivedContextType::None);
+    UnlinkedFunctionExecutable* functionExecutable = UnlinkedFunctionExecutable::create(&vm, source, metadata, UnlinkedNormalFunction, constructAbility, JSParserScriptMode::Classic, vm.m_compactVariableMap->get(emptyTDZVariables), DerivedContextType::None);
 
     functionExecutable->setSourceURLDirective(source.provider()->sourceURL());
     functionExecutable->setSourceMappingURLDirective(source.provider()->sourceMappingURL());