Attachment 362073 Details for Bug 194583 – patch for landing

[patch] patch for landing

b-backup.diff (text/plain), 16.91 KB, created by Saam Barati on 2019-02-14 15:39:28 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Saam Barati

Created: 2019-02-14 15:39:28 PST

Size: 16.91 KB

patch

obsolete

>Index: JSTests/ChangeLog
>===================================================================
>--- JSTests/ChangeLog	(revision 241566)
>+++ JSTests/ChangeLog	(working copy)
>@@ -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
>Index: JSTests/microbenchmarks/cache-get-variables-under-tdz-in-bytecode-generator.js
>===================================================================
>--- JSTests/microbenchmarks/cache-get-variables-under-tdz-in-bytecode-generator.js	(nonexistent)
>+++ JSTests/microbenchmarks/cache-get-variables-under-tdz-in-bytecode-generator.js	(working copy)
>@@ -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: Source/JavaScriptCore/ChangeLog
>===================================================================
>--- Source/JavaScriptCore/ChangeLog	(revision 241563)
>+++ Source/JavaScriptCore/ChangeLog	(working copy)
>@@ -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: Source/JavaScriptCore/builtins/BuiltinExecutables.cpp
>===================================================================
>--- Source/JavaScriptCore/builtins/BuiltinExecutables.cpp	(revision 241563)
>+++ Source/JavaScriptCore/builtins/BuiltinExecutables.cpp	(working copy)
>@@ -258,7 +258,7 @@ UnlinkedFunctionExecutable* BuiltinExecu
>     }
> 
>     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: Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.cpp
>===================================================================
>--- Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.cpp	(revision 241563)
>+++ Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.cpp	(working copy)
>@@ -78,7 +78,7 @@ static UnlinkedFunctionCodeBlock* genera
>     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::UnlinkedFunc
>     , 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: Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h
>===================================================================
>--- Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h	(revision 241563)
>+++ Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h	(working copy)
>@@ -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: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp
>===================================================================
>--- Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp	(revision 241563)
>+++ Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp	(working copy)
>@@ -2195,6 +2195,7 @@ void BytecodeGenerator::popLexicalScopeI
>     }
> 
>     m_TDZStack.removeLast();
>+    m_cachedVariablesUnderTDZ = { };
> }
> 
> void BytecodeGenerator::prepareLexicalScopeForNextForLoopIteration(VariableEnvironmentNode* node, RegisterID* loopSymbolTable)
>@@ -2814,8 +2815,10 @@ void BytecodeGenerator::liftTDZCheckIfPo
>     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
>         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::getVariablesUnde
>     //         }
>     //         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
> void BytecodeGenerator::restoreTDZStack(const BytecodeGenerator::PreservedTDZStack& preservedStack)
> {
>     m_TDZStack = preservedStack.m_preservedTDZStack;
>+    m_cachedVariablesUnderTDZ = { };
> }
> 
> RegisterID* BytecodeGenerator::emitNewObject(RegisterID* dst)
>Index: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h
>===================================================================
>--- Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h	(revision 241563)
>+++ Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h	(working copy)
>@@ -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: Source/JavaScriptCore/parser/VariableEnvironment.cpp
>===================================================================
>--- Source/JavaScriptCore/parser/VariableEnvironment.cpp	(revision 241563)
>+++ Source/JavaScriptCore/parser/VariableEnvironment.cpp	(working copy)
>@@ -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: Source/JavaScriptCore/parser/VariableEnvironment.h
>===================================================================
>--- Source/JavaScriptCore/parser/VariableEnvironment.h	(revision 241563)
>+++ Source/JavaScriptCore/parser/VariableEnvironment.h	(working copy)
>@@ -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: Source/JavaScriptCore/runtime/CodeCache.cpp
>===================================================================
>--- Source/JavaScriptCore/runtime/CodeCache.cpp	(revision 241563)
>+++ Source/JavaScriptCore/runtime/CodeCache.cpp	(working copy)
>@@ -147,7 +147,7 @@ UnlinkedFunctionExecutable* CodeCache::g
>     // 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());

Actions: View | Formatted Diff | Diff

Attachments on bug 194583: 361900 | 362073