[webkit-changes] [199848] trunk/Source

Thu, 21 Apr 2016 17:09:59 -0700

Title: [199848] trunk/Source
Revision
199848
Author
sbar...@apple.com
Date
2016-04-21 17:09:36 -0700 (Thu, 21 Apr 2016)

Log Message

Lets do less locking of symbol tables in the BytecodeGenerator where we don't have race conditions
https://bugs.webkit.org/show_bug.cgi?id=156821

Reviewed by Filip Pizlo.

Source/_javascript_Core:

The BytecodeGenerator allocates all the SymbolTables that it uses.
This is before any concurrent compiler thread can use that SymbolTable.
This means we don't actually need to lock for any operations of the
SymbolTable. This patch makes this change by removing all locking.
To do this, I've introduced a new constructor for ConcurrentJITLocker
which implies no locking is necessary. You instantiate such a ConcurrentJITLocker like so:
`ConcurrentJITLocker locker(ConcurrentJITLocker::NoLockingNecessary);`

This patch also removes all uses of Strong<SymbolTable> from the bytecode
generator and instead wraps bytecode generation in a DeferGC.

* bytecode/UnlinkedFunctionExecutable.cpp:
(JSC::generateUnlinkedFunctionCodeBlock):
* bytecompiler/BytecodeGenerator.cpp:
(JSC::BytecodeGenerator::BytecodeGenerator):
(JSC::BytecodeGenerator::initializeDefaultParameterValuesAndSetupFunctionScopeStack):
(JSC::BytecodeGenerator::initializeArrowFunctionContextScopeIfNeeded):
(JSC::BytecodeGenerator::instantiateLexicalVariables):
(JSC::BytecodeGenerator::emitPrefillStackTDZVariables):
(JSC::BytecodeGenerator::pushLexicalScopeInternal):
(JSC::BytecodeGenerator::initializeBlockScopedFunctions):
(JSC::BytecodeGenerator::hoistSloppyModeFunctionIfNecessary):
(JSC::BytecodeGenerator::popLexicalScopeInternal):
(JSC::BytecodeGenerator::prepareLexicalScopeForNextForLoopIteration):
(JSC::BytecodeGenerator::variable):
(JSC::BytecodeGenerator::createVariable):
(JSC::BytecodeGenerator::emitResolveScope):
(JSC::BytecodeGenerator::emitPushWithScope):
(JSC::BytecodeGenerator::emitPushFunctionNameScope):
* bytecompiler/BytecodeGenerator.h:
(JSC::BytecodeGenerator::constructorKind):
(JSC::BytecodeGenerator::superBinding):
(JSC::BytecodeGenerator::generate):
* runtime/CodeCache.cpp:
(JSC::CodeCache::getGlobalCodeBlock):
* runtime/ConcurrentJITLock.h:
(JSC::ConcurrentJITLockerBase::ConcurrentJITLockerBase):
(JSC::ConcurrentJITLockerBase::~ConcurrentJITLockerBase):
(JSC::ConcurrentJITLocker::ConcurrentJITLocker):

Source/WTF:

This patch introduces a new constructor for Locker which implies no
locking is necessary. You instantiate such a locker like so:
`Locker<Lock> locker(Locker<Lock>::NoLockingNecessary);`

This is useful to for very specific places when it is not yet
required to engage in a specified locking protocol. As an example,
we use this in JSC when we allocate a particular object that
engages in a locking protocol with the concurrent compiler thread,
but before a concurrent compiler thread that could have access
to the object exists.

* wtf/Locker.h:
(WTF::Locker::Locker):
(WTF::Locker::~Locker):

Modified Paths

Diff

Modified: trunk/Source/_javascript_Core/ChangeLog (199847 => 199848)


--- trunk/Source/_javascript_Core/ChangeLog	2016-04-22 00:08:28 UTC (rev 199847)
+++ trunk/Source/_javascript_Core/ChangeLog	2016-04-22 00:09:36 UTC (rev 199848)
@@ -1,5 +1,52 @@
 2016-04-21  Saam barati  <sbar...@apple.com>
 
+        Lets do less locking of symbol tables in the BytecodeGenerator where we don't have race conditions
+        https://bugs.webkit.org/show_bug.cgi?id=156821
+
+        Reviewed by Filip Pizlo.
+
+        The BytecodeGenerator allocates all the SymbolTables that it uses.
+        This is before any concurrent compiler thread can use that SymbolTable.
+        This means we don't actually need to lock for any operations of the
+        SymbolTable. This patch makes this change by removing all locking.
+        To do this, I've introduced a new constructor for ConcurrentJITLocker
+        which implies no locking is necessary. You instantiate such a ConcurrentJITLocker like so:
+        `ConcurrentJITLocker locker(ConcurrentJITLocker::NoLockingNecessary);`
+
+        This patch also removes all uses of Strong<SymbolTable> from the bytecode
+        generator and instead wraps bytecode generation in a DeferGC.
+
+        * bytecode/UnlinkedFunctionExecutable.cpp:
+        (JSC::generateUnlinkedFunctionCodeBlock):
+        * bytecompiler/BytecodeGenerator.cpp:
+        (JSC::BytecodeGenerator::BytecodeGenerator):
+        (JSC::BytecodeGenerator::initializeDefaultParameterValuesAndSetupFunctionScopeStack):
+        (JSC::BytecodeGenerator::initializeArrowFunctionContextScopeIfNeeded):
+        (JSC::BytecodeGenerator::instantiateLexicalVariables):
+        (JSC::BytecodeGenerator::emitPrefillStackTDZVariables):
+        (JSC::BytecodeGenerator::pushLexicalScopeInternal):
+        (JSC::BytecodeGenerator::initializeBlockScopedFunctions):
+        (JSC::BytecodeGenerator::hoistSloppyModeFunctionIfNecessary):
+        (JSC::BytecodeGenerator::popLexicalScopeInternal):
+        (JSC::BytecodeGenerator::prepareLexicalScopeForNextForLoopIteration):
+        (JSC::BytecodeGenerator::variable):
+        (JSC::BytecodeGenerator::createVariable):
+        (JSC::BytecodeGenerator::emitResolveScope):
+        (JSC::BytecodeGenerator::emitPushWithScope):
+        (JSC::BytecodeGenerator::emitPushFunctionNameScope):
+        * bytecompiler/BytecodeGenerator.h:
+        (JSC::BytecodeGenerator::constructorKind):
+        (JSC::BytecodeGenerator::superBinding):
+        (JSC::BytecodeGenerator::generate):
+        * runtime/CodeCache.cpp:
+        (JSC::CodeCache::getGlobalCodeBlock):
+        * runtime/ConcurrentJITLock.h:
+        (JSC::ConcurrentJITLockerBase::ConcurrentJITLockerBase):
+        (JSC::ConcurrentJITLockerBase::~ConcurrentJITLockerBase):
+        (JSC::ConcurrentJITLocker::ConcurrentJITLocker):
+
+2016-04-21  Saam barati  <sbar...@apple.com>
+
         Remove some unnecessary RefPtrs in the parser
         https://bugs.webkit.org/show_bug.cgi?id=156865

Modified: trunk/Source/_javascript_Core/bytecode/UnlinkedFunctionExecutable.cpp (199847 => 199848)


--- trunk/Source/_javascript_Core/bytecode/UnlinkedFunctionExecutable.cpp	2016-04-22 00:08:28 UTC (rev 199847)
+++ trunk/Source/_javascript_Core/bytecode/UnlinkedFunctionExecutable.cpp	2016-04-22 00:09:36 UTC (rev 199848)
@@ -70,8 +70,8 @@
 
     UnlinkedFunctionCodeBlock* result = UnlinkedFunctionCodeBlock::create(&vm, FunctionCode, ExecutableInfo(function->usesEval(), function->isStrictMode(), kind == CodeForConstruct, functionKind == UnlinkedBuiltinFunction, executable->constructorKind(), executable->superBinding(), parseMode, executable->derivedContextType(), false, isClassContext, EvalContextType::FunctionEvalContext));
 
-    auto generator(std::make_unique<BytecodeGenerator>(vm, function.get(), result, debuggerMode, profilerMode, executable->parentScopeTDZVariables()));
-    error = generator->generate();
+    error = BytecodeGenerator::generate(vm, function.get(), result, debuggerMode, profilerMode, executable->parentScopeTDZVariables());
+
     if (error.isValid())
         return nullptr;
     return result;

Modified: trunk/Source/_javascript_Core/bytecompiler/BytecodeGenerator.cpp (199847 => 199848)


--- trunk/Source/_javascript_Core/bytecompiler/BytecodeGenerator.cpp	2016-04-22 00:08:28 UTC (rev 199847)
+++ trunk/Source/_javascript_Core/bytecompiler/BytecodeGenerator.cpp	2016-04-22 00:09:36 UTC (rev 199848)
@@ -365,6 +365,7 @@
         // use DirectArguments. With ScopedArguments, we lift all of our arguments into the
         // activation.
         
+        ConcurrentJITLocker locker(ConcurrentJITLocker::NoLockingNecessary);
         if (capturesAnyArgumentByName) {
             functionSymbolTable->setArgumentsLength(vm, parameters.size());
             
@@ -373,7 +374,7 @@
             // in the symbol table - or it just gets space reserved in the symbol table. Either
             // way we lift the value into the scope.
             for (unsigned i = 0; i < parameters.size(); ++i) {
-                ScopeOffset offset = functionSymbolTable->takeNextScopeOffset();
+                ScopeOffset offset = functionSymbolTable->takeNextScopeOffset(locker);
                 functionSymbolTable->setArgumentOffset(vm, i, offset);
                 if (UniquedStringImpl* name = visibleNameForParameter(parameters.at(i).first)) {
                     VarOffset varOffset(offset);
@@ -383,7 +384,7 @@
                     // parameters when "arguments" is in play is unlikely to be super profitable.
                     // So, we just disable it.
                     entry.disableWatching();
-                    functionSymbolTable->set(name, entry);
+                    functionSymbolTable->set(locker, name, entry);
                 }
                 emitOpcode(op_put_to_scope);
                 instructions().append(m_lexicalEnvironmentRegister->index());
@@ -404,7 +405,7 @@
             // that the symbol table knows that this is happening.
             for (unsigned i = 0; i < parameters.size(); ++i) {
                 if (UniquedStringImpl* name = visibleNameForParameter(parameters.at(i).first))
-                    functionSymbolTable->set(name, SymbolTableEntry(VarOffset(DirectArgumentsOffset(i))));
+                    functionSymbolTable->set(locker, name, SymbolTableEntry(VarOffset(DirectArgumentsOffset(i))));
             }
             
             emitOpcode(op_create_direct_arguments);
@@ -415,6 +416,7 @@
         // captured, lift them into the scope. We cannot do this if we have default parameter expressions
         // because when default parameter expressions exist, they belong in their own lexical environment
         // separate from the "var" lexical environment.
+        ConcurrentJITLocker locker(ConcurrentJITLocker::NoLockingNecessary);
         for (unsigned i = 0; i < parameters.size(); ++i) {
             UniquedStringImpl* name = visibleNameForParameter(parameters.at(i).first);
             if (!name)
@@ -423,14 +425,14 @@
             if (!captures(name)) {
                 // This is the easy case - just tell the symbol table about the argument. It will
                 // be accessed directly.
-                functionSymbolTable->set(name, SymbolTableEntry(VarOffset(virtualRegisterForArgument(1 + i))));
+                functionSymbolTable->set(locker, name, SymbolTableEntry(VarOffset(virtualRegisterForArgument(1 + i))));
                 continue;
             }
             
-            ScopeOffset offset = functionSymbolTable->takeNextScopeOffset();
+            ScopeOffset offset = functionSymbolTable->takeNextScopeOffset(locker);
             const Identifier& ident =
                 static_cast<const BindingNode*>(parameters.at(i).first)->boundProperty();
-            functionSymbolTable->set(name, SymbolTableEntry(VarOffset(offset)));
+            functionSymbolTable->set(locker, name, SymbolTableEntry(VarOffset(offset)));
             
             emitOpcode(op_put_to_scope);
             instructions().append(m_lexicalEnvironmentRegister->index());
@@ -721,7 +723,7 @@
 
     pushTDZVariables(lexicalVariables, TDZCheckOptimization::Optimize);
     bool isWithScope = false;
-    m_symbolTableStack.append(SymbolTableStackEntry { Strong<SymbolTable>(*m_vm, moduleEnvironmentSymbolTable), m_topMostScope, isWithScope, constantSymbolTable->index() });
+    m_symbolTableStack.append(SymbolTableStackEntry { moduleEnvironmentSymbolTable, m_topMostScope, isWithScope, constantSymbolTable->index() });
     emitPrefillStackTDZVariables(lexicalVariables, moduleEnvironmentSymbolTable);
 
     // makeFunction assumes that there's correct TDZ stack entries.
@@ -859,7 +861,7 @@
 
     if (m_lexicalEnvironmentRegister)
         pushScopedControlFlowContext();
-    m_symbolTableStack.append(SymbolTableStackEntry{ Strong<SymbolTable>(*m_vm, functionSymbolTable), m_lexicalEnvironmentRegister, false, symbolTableConstantIndex });
+    m_symbolTableStack.append(SymbolTableStackEntry{ functionSymbolTable, m_lexicalEnvironmentRegister, false, symbolTableConstantIndex });
 
     m_varScopeSymbolTableIndex = m_symbolTableStack.size() - 1;
 
@@ -883,19 +885,20 @@
         if (!m_codeBlock->isArrowFunction()) {
             ScopeOffset offset;
             
+            ConcurrentJITLocker locker(ConcurrentJITLocker::NoLockingNecessary);
             if (isThisUsedInInnerArrowFunction()) {
-                offset = symbolTable->takeNextScopeOffset();
-                symbolTable->set(propertyNames().thisIdentifier.impl(), SymbolTableEntry(VarOffset(offset)));
+                offset = symbolTable->takeNextScopeOffset(locker);
+                symbolTable->set(locker, propertyNames().thisIdentifier.impl(), SymbolTableEntry(VarOffset(offset)));
             }
 
             if (m_codeType == FunctionCode && isNewTargetUsedInInnerArrowFunction()) {
                 offset = symbolTable->takeNextScopeOffset();
-                symbolTable->set(propertyNames().newTargetLocalPrivateName.impl(), SymbolTableEntry(VarOffset(offset)));
+                symbolTable->set(locker, propertyNames().newTargetLocalPrivateName.impl(), SymbolTableEntry(VarOffset(offset)));
             }
             
             if (isConstructor() && constructorKind() == ConstructorKind::Derived && isSuperUsedInInnerArrowFunction()) {
-                offset = symbolTable->takeNextScopeOffset();
-                symbolTable->set(propertyNames().derivedConstructorPrivateName.impl(), SymbolTableEntry(VarOffset(offset)));
+                offset = symbolTable->takeNextScopeOffset(locker);
+                symbolTable->set(locker, propertyNames().derivedConstructorPrivateName.impl(), SymbolTableEntry(VarOffset(offset)));
             }
         }
 
@@ -1735,7 +1738,7 @@
 {
     bool hasCapturedVariables = false;
     {
-        ConcurrentJITLocker locker(symbolTable->m_lock);
+        ConcurrentJITLocker locker(ConcurrentJITLocker::NoLockingNecessary);
         for (auto& entry : lexicalVariables) {
             ASSERT(entry.value.isLet() || entry.value.isConst() || entry.value.isFunction());
             ASSERT(!entry.value.isVar());
@@ -1776,6 +1779,7 @@
 {
     // Prefill stack variables with the TDZ empty value.
     // Scope variables will be initialized to the TDZ empty value when JSLexicalEnvironment is allocated.
+    ConcurrentJITLocker locker(ConcurrentJITLocker::NoLockingNecessary);
     for (auto& entry : lexicalVariables) {
         // Imported bindings which are not the namespace bindings are not allocated
         // in the module environment as usual variables' way.
@@ -1787,7 +1791,7 @@
         if (entry.value.isFunction())
             continue;
 
-        SymbolTableEntry symbolTableEntry = symbolTable->get(entry.key.get());
+        SymbolTableEntry symbolTableEntry = symbolTable->get(locker, entry.key.get());
         ASSERT(!symbolTableEntry.isNull());
         VarOffset offset = symbolTableEntry.varOffset();
         if (offset.isScope())
@@ -1820,7 +1824,7 @@
     if (m_shouldEmitDebugHooks)
         environment.markAllVariablesAsCaptured();
 
-    Strong<SymbolTable> symbolTable(*m_vm, SymbolTable::create(*m_vm));
+    SymbolTable* symbolTable = SymbolTable::create(*m_vm);
     switch (scopeType) {
     case ScopeType::CatchScope:
         symbolTable->setScopeType(SymbolTable::ScopeType::CatchScope);
@@ -1840,13 +1844,13 @@
         return entry.isCaptured() ? VarKind::Scope : VarKind::Stack;
     };
 
-    bool hasCapturedVariables = instantiateLexicalVariables(environment, symbolTable.get(), scopeRegisterType, lookUpVarKind);
+    bool hasCapturedVariables = instantiateLexicalVariables(environment, symbolTable, scopeRegisterType, lookUpVarKind);
 
     RegisterID* newScope = nullptr;
     RegisterID* constantSymbolTable = nullptr;
     int symbolTableConstantIndex = 0;
     if (vm()->typeProfiler()) {
-        constantSymbolTable = addConstantValue(symbolTable.get());
+        constantSymbolTable = addConstantValue(symbolTable);
         symbolTableConstantIndex = constantSymbolTable->index();
     }
     if (hasCapturedVariables) {
@@ -1880,7 +1884,7 @@
         pushTDZVariables(environment, tdzCheckOptimization);
 
     if (tdzRequirement == TDZRequirement::UnderTDZ)
-        emitPrefillStackTDZVariables(environment, symbolTable.get());
+        emitPrefillStackTDZVariables(environment, symbolTable);
 }
 
 void BytecodeGenerator::initializeBlockScopedFunctions(VariableEnvironment& environment, FunctionStack& functionStack, RegisterID* constantSymbolTable)
@@ -1919,17 +1923,18 @@
     if (!functionStack.size())
         return;
 
-    SymbolTable* symbolTable = m_symbolTableStack.last().m_symbolTable.get();
+    SymbolTable* symbolTable = m_symbolTableStack.last().m_symbolTable;
     RegisterID* scope = m_symbolTableStack.last().m_scope;
     RefPtr<RegisterID> temp = newTemporary();
     int symbolTableIndex = constantSymbolTable ? constantSymbolTable->index() : 0;
+    ConcurrentJITLocker locker(ConcurrentJITLocker::NoLockingNecessary);
     for (FunctionMetadataNode* function : functionStack) {
         const Identifier& name = function->ident();
         auto iter = environment.find(name.impl());
         RELEASE_ASSERT(iter != environment.end());
         RELEASE_ASSERT(iter->value.isFunction());
         // We purposefully don't hold the symbol table lock around this loop because emitNewFunctionExpressionCommon may GC.
-        SymbolTableEntry entry = symbolTable->get(name.impl()); 
+        SymbolTableEntry entry = symbolTable->get(locker, name.impl()); 
         RELEASE_ASSERT(!entry.isNull());
         emitNewFunctionExpressionCommon(temp.get(), function);
         bool isLexicallyScoped = true;
@@ -1952,9 +1957,10 @@
         ASSERT(m_varScopeSymbolTableIndex);
         ASSERT(*m_varScopeSymbolTableIndex < m_symbolTableStack.size());
         SymbolTableStackEntry& varScope = m_symbolTableStack[*m_varScopeSymbolTableIndex];
-        SymbolTable* varSymbolTable = varScope.m_symbolTable.get();
+        SymbolTable* varSymbolTable = varScope.m_symbolTable;
         ASSERT(varSymbolTable->scopeType() == SymbolTable::ScopeType::VarScope);
-        SymbolTableEntry entry = varSymbolTable->get(functionName.impl());
+        ConcurrentJITLocker locker(ConcurrentJITLocker::NoLockingNecessary);
+        SymbolTableEntry entry = varSymbolTable->get(locker, functionName.impl());
         ASSERT(!entry.isNull());
         bool isLexicallyScoped = false;
         emitPutToScope(varScope.m_scope, variableForLocalEntry(functionName, entry, varScope.m_symbolTableConstantIndex, isLexicallyScoped), currentValue.get(), DoNotThrowIfNotFound, InitializationMode::NotInitialization);
@@ -1978,9 +1984,9 @@
         environment.markAllVariablesAsCaptured();
 
     SymbolTableStackEntry stackEntry = m_symbolTableStack.takeLast();
-    Strong<SymbolTable> symbolTable = stackEntry.m_symbolTable;
-    ConcurrentJITLocker locker(symbolTable->m_lock);
+    SymbolTable* symbolTable = stackEntry.m_symbolTable;
     bool hasCapturedVariables = false;
+    ConcurrentJITLocker locker(ConcurrentJITLocker::NoLockingNecessary);
     for (auto& entry : environment) {
         if (entry.value.isCaptured()) {
             hasCapturedVariables = true;
@@ -2025,16 +2031,16 @@
     // gets a new activation.
 
     SymbolTableStackEntry stackEntry = m_symbolTableStack.last();
-    Strong<SymbolTable> symbolTable = stackEntry.m_symbolTable;
+    SymbolTable* symbolTable = stackEntry.m_symbolTable;
     RegisterID* loopScope = stackEntry.m_scope;
     ASSERT(symbolTable->scopeSize());
     ASSERT(loopScope);
     Vector<std::pair<RegisterID*, Identifier>> activationValuesToCopyOver;
 
     {
-        ConcurrentJITLocker locker(symbolTable->m_lock);
         activationValuesToCopyOver.reserveInitialCapacity(symbolTable->scopeSize());
 
+        ConcurrentJITLocker locker(ConcurrentJITLocker::NoLockingNecessary);
         for (auto end = symbolTable->end(locker), ptr = symbolTable->begin(locker); ptr != end; ++ptr) {
             if (!ptr->value.varOffset().isScope())
                 continue;
@@ -2067,7 +2073,7 @@
     emitMove(scopeRegister(), loopScope);
 
     {
-        ConcurrentJITLocker locker(symbolTable->m_lock);
+        ConcurrentJITLocker locker(ConcurrentJITLocker::NoLockingNecessary);
         for (auto pair : activationValuesToCopyOver) {
             const Identifier& identifier = pair.second;
             SymbolTableEntry entry = symbolTable->get(locker, identifier.impl());
@@ -2105,12 +2111,13 @@
     //         doSomethingWith(x);
     //     }
     // }
+    ConcurrentJITLocker locker(ConcurrentJITLocker::NoLockingNecessary);
     for (unsigned i = m_symbolTableStack.size(); i--; ) {
         SymbolTableStackEntry& stackEntry = m_symbolTableStack[i];
         if (stackEntry.m_isWithScope)
             return Variable(property);
-        Strong<SymbolTable>& symbolTable = stackEntry.m_symbolTable;
-        SymbolTableEntry symbolTableEntry = symbolTable->get(property.impl());
+        SymbolTable* symbolTable = stackEntry.m_symbolTable;
+        SymbolTableEntry symbolTableEntry = symbolTable->get(locker, property.impl());
         if (symbolTableEntry.isNull())
             continue;
         bool resultIsCallee = false;
@@ -2150,7 +2157,7 @@
     const Identifier& property, VarKind varKind, SymbolTable* symbolTable, ExistingVariableMode existingVariableMode)
 {
     ASSERT(property != propertyNames().thisIdentifier);
-    ConcurrentJITLocker locker(symbolTable->m_lock);
+    ConcurrentJITLocker locker(ConcurrentJITLocker::NoLockingNecessary);
     SymbolTableEntry entry = symbolTable->get(locker, property.impl());
     
     if (!entry.isNull()) {
@@ -2227,20 +2234,21 @@
     case VarKind::DirectArgument:
         return argumentsRegister();
         
-    case VarKind::Scope:
+    case VarKind::Scope: {
         // This always refers to the activation that *we* allocated, and not the current scope that code
         // lives in. Note that this will change once we have proper support for block scoping. Once that
         // changes, it will be correct for this code to return scopeRegister(). The only reason why we
         // don't do that already is that m_lexicalEnvironment is required by ConstDeclNode. ConstDeclNode
         // requires weird things because it is a shameful pile of nonsense, but block scoping would make
         // that code sensible and obviate the need for us to do bad things.
+        ConcurrentJITLocker locker(ConcurrentJITLocker::NoLockingNecessary);
         for (unsigned i = m_symbolTableStack.size(); i--; ) {
             SymbolTableStackEntry& stackEntry = m_symbolTableStack[i];
             // We should not resolve a variable to VarKind::Scope if a "with" scope lies in between the current
             // scope and the resolved scope.
             RELEASE_ASSERT(!stackEntry.m_isWithScope);
 
-            if (stackEntry.m_symbolTable->get(variable.ident().impl()).isNull())
+            if (stackEntry.m_symbolTable->get(locker, variable.ident().impl()).isNull())
                 continue;
             
             RegisterID* scope = stackEntry.m_scope;
@@ -2251,6 +2259,7 @@
         RELEASE_ASSERT_NOT_REACHED();
         return nullptr;
         
+    }
     case VarKind::Invalid:
         // Indicates non-local resolution.
         
@@ -3274,7 +3283,7 @@
     instructions().append(scopeRegister()->index());
 
     emitMove(scopeRegister(), newScope);
-    m_symbolTableStack.append(SymbolTableStackEntry{ Strong<SymbolTable>(), newScope, true, 0 });
+    m_symbolTableStack.append(SymbolTableStackEntry{ nullptr, newScope, true, 0 });
 
     return newScope;
 }
@@ -3738,7 +3747,8 @@
     pushLexicalScopeInternal(nameScopeEnvironment, TDZCheckOptimization::Optimize, NestedScopeType::IsNotNested, nullptr, TDZRequirement::NotUnderTDZ, ScopeType::FunctionNameScope, ScopeRegisterType::Var);
     ASSERT_UNUSED(numVars, m_codeBlock->m_numVars == static_cast<int>(numVars + 1)); // Should have only created one new "var" for the function name scope.
     bool shouldTreatAsLexicalVariable = isStrictMode();
-    Variable functionVar = variableForLocalEntry(property, m_symbolTableStack.last().m_symbolTable->get(property.impl()), m_symbolTableStack.last().m_symbolTableConstantIndex, shouldTreatAsLexicalVariable);
+    ConcurrentJITLocker locker(ConcurrentJITLocker::NoLockingNecessary);
+    Variable functionVar = variableForLocalEntry(property, m_symbolTableStack.last().m_symbolTable->get(locker, property.impl()), m_symbolTableStack.last().m_symbolTableConstantIndex, shouldTreatAsLexicalVariable);
     emitPutToScope(m_symbolTableStack.last().m_scope, functionVar, callee, ThrowIfNotFound, InitializationMode::NotInitialization);
 }

Modified: trunk/Source/_javascript_Core/bytecompiler/BytecodeGenerator.h (199847 => 199848)


--- trunk/Source/_javascript_Core/bytecompiler/BytecodeGenerator.h	2016-04-22 00:08:28 UTC (rev 199847)
+++ trunk/Source/_javascript_Core/bytecompiler/BytecodeGenerator.h	2016-04-22 00:09:36 UTC (rev 199848)
@@ -289,7 +289,13 @@
         ConstructorKind constructorKind() const { return m_codeBlock->constructorKind(); }
         SuperBinding superBinding() const { return m_codeBlock->superBinding(); }
 
-        ParserError generate();
+        template<typename... Args>
+        static ParserError generate(VM& vm, Args&& ...args)
+        {
+            DeferGC deferGC(vm.heap);
+            auto bytecodeGenerator = std::make_unique<BytecodeGenerator>(vm, std::forward<Args>(args)...);
+            return bytecodeGenerator->generate(); 
+        }
 
         bool isArgumentNumber(const Identifier&, int);
 
@@ -737,6 +743,7 @@
         int labelScopeDepth() const;
 
     private:
+        ParserError generate();
         void reclaimFreeRegisters();
         Variable variableForLocalEntry(const Identifier&, const SymbolTableEntry&, int symbolTableConstantIndex, bool isLexicallyScoped);
 
@@ -862,7 +869,7 @@
         bool m_shouldEmitProfileHooks;
 
         struct SymbolTableStackEntry {
-            Strong<SymbolTable> m_symbolTable;
+            SymbolTable* m_symbolTable;
             RegisterID* m_scope;
             bool m_isWithScope;
             int m_symbolTableConstantIndex;

Modified: trunk/Source/_javascript_Core/runtime/CodeCache.cpp (199847 => 199848)


--- trunk/Source/_javascript_Core/runtime/CodeCache.cpp	2016-04-22 00:08:28 UTC (rev 199847)
+++ trunk/Source/_javascript_Core/runtime/CodeCache.cpp	2016-04-22 00:09:36 UTC (rev 199848)
@@ -119,8 +119,8 @@
     UnlinkedCodeBlockType* unlinkedCodeBlock = UnlinkedCodeBlockType::create(&vm, executable->executableInfo());
     unlinkedCodeBlock->recordParse(rootNode->features(), rootNode->hasCapturedVariables(), rootNode->firstLine() - source.firstLine(), lineCount, unlinkedEndColumn);
 
-    auto generator = std::make_unique<BytecodeGenerator>(vm, rootNode.get(), unlinkedCodeBlock, debuggerMode, profilerMode, variablesUnderTDZ);
-    error = generator->generate();
+    error = BytecodeGenerator::generate(vm, rootNode.get(), unlinkedCodeBlock, debuggerMode, profilerMode, variablesUnderTDZ);
+
     if (error.isValid())
         return nullptr;

Modified: trunk/Source/_javascript_Core/runtime/ConcurrentJITLock.h (199847 => 199848)


--- trunk/Source/_javascript_Core/runtime/ConcurrentJITLock.h	2016-04-22 00:08:28 UTC (rev 199847)
+++ trunk/Source/_javascript_Core/runtime/ConcurrentJITLock.h	2016-04-22 00:09:36 UTC (rev 199848)
@@ -29,6 +29,7 @@
 #include "DeferGC.h"
 #include <wtf/Lock.h>
 #include <wtf/NoLock.h>
+#include <wtf/Optional.h>
 
 namespace JSC {
 
@@ -52,6 +53,12 @@
     {
     }
 
+    enum NoLockingNecessaryTag { NoLockingNecessary };
+    explicit ConcurrentJITLockerBase(NoLockingNecessaryTag)
+        : m_locker(ConcurrentJITLockerImpl::NoLockingNecessary)
+    {
+    }
+
     ~ConcurrentJITLockerBase()
     {
     }
@@ -104,17 +111,31 @@
 public:
     ConcurrentJITLocker(ConcurrentJITLock& lockable)
         : ConcurrentJITLockerBase(lockable)
+#if ENABLE(CONCURRENT_JIT) && !defined(NDEBUG)
+        , m_disallowGC(InPlace)
+#endif
     {
     }
 
     ConcurrentJITLocker(ConcurrentJITLock* lockable)
         : ConcurrentJITLockerBase(lockable)
+#if ENABLE(CONCURRENT_JIT) && !defined(NDEBUG)
+        , m_disallowGC(InPlace)
+#endif
     {
     }
 
+    ConcurrentJITLocker(ConcurrentJITLockerBase::NoLockingNecessaryTag)
+        : ConcurrentJITLockerBase(ConcurrentJITLockerBase::NoLockingNecessary)
 #if ENABLE(CONCURRENT_JIT) && !defined(NDEBUG)
+        , m_disallowGC(Nullopt)
+#endif
+    {
+    }
+
+#if ENABLE(CONCURRENT_JIT) && !defined(NDEBUG)
 private:
-    DisallowGC m_disallowGC;
+    Optional<DisallowGC> m_disallowGC;
 #endif
 };

Modified: trunk/Source/WTF/ChangeLog (199847 => 199848)


--- trunk/Source/WTF/ChangeLog	2016-04-22 00:08:28 UTC (rev 199847)
+++ trunk/Source/WTF/ChangeLog	2016-04-22 00:09:36 UTC (rev 199848)
@@ -1,3 +1,25 @@
+2016-04-21  Saam barati  <sbar...@apple.com>
+
+        Lets do less locking of symbol tables in the BytecodeGenerator where we don't have race conditions
+        https://bugs.webkit.org/show_bug.cgi?id=156821
+
+        Reviewed by Filip Pizlo.
+
+        This patch introduces a new constructor for Locker which implies no
+        locking is necessary. You instantiate such a locker like so:
+        `Locker<Lock> locker(Locker<Lock>::NoLockingNecessary);`
+
+        This is useful to for very specific places when it is not yet
+        required to engage in a specified locking protocol. As an example,
+        we use this in JSC when we allocate a particular object that
+        engages in a locking protocol with the concurrent compiler thread,
+        but before a concurrent compiler thread that could have access
+        to the object exists.
+
+        * wtf/Locker.h:
+        (WTF::Locker::Locker):
+        (WTF::Locker::~Locker):
+
 2016-04-19  Michael Saboff  <msab...@apple.com>
 
         iTunes crashing _javascript_Core.dll

Modified: trunk/Source/WTF/wtf/Locker.h (199847 => 199848)


--- trunk/Source/WTF/wtf/Locker.h	2016-04-22 00:08:28 UTC (rev 199847)
+++ trunk/Source/WTF/wtf/Locker.h	2016-04-22 00:09:36 UTC (rev 199848)
@@ -37,6 +37,15 @@
 public:
     explicit Locker(T& lockable) : m_lockable(&lockable) { lock(); }
     explicit Locker(T* lockable) : m_lockable(lockable) { lock(); }
+
+    enum NoLockingNecessaryTag { NoLockingNecessary };
+    // You should be wary of using this constructor. It's only applicable
+    // in places where there is a locking protocol for a particular object
+    // but it's not necessary to engage in that protocol yet. For example,
+    // this often happens when an object is newly allocated and it can not
+    // be accessed concurrently.
+    explicit Locker(NoLockingNecessaryTag) : m_lockable(nullptr) { }
+
     ~Locker()
     {
         if (m_lockable)

_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to