<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[199848] trunk/Source</title>
</head>
<body>
<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; }
#msg dl a { font-weight: bold}
#msg dl a:link { color:#fc3; }
#msg dl a:active { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/199848">199848</a></dd>
<dt>Author</dt> <dd>sbarati@apple.com</dd>
<dt>Date</dt> <dd>2016-04-21 17:09:36 -0700 (Thu, 21 Apr 2016)</dd>
</dl>
<h3>Log Message</h3>
<pre>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/JavaScriptCore:
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):</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkSourceJavaScriptCoreChangeLog">trunk/Source/JavaScriptCore/ChangeLog</a></li>
<li><a href="#trunkSourceJavaScriptCorebytecodeUnlinkedFunctionExecutablecpp">trunk/Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.cpp</a></li>
<li><a href="#trunkSourceJavaScriptCorebytecompilerBytecodeGeneratorcpp">trunk/Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp</a></li>
<li><a href="#trunkSourceJavaScriptCorebytecompilerBytecodeGeneratorh">trunk/Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h</a></li>
<li><a href="#trunkSourceJavaScriptCoreruntimeCodeCachecpp">trunk/Source/JavaScriptCore/runtime/CodeCache.cpp</a></li>
<li><a href="#trunkSourceJavaScriptCoreruntimeConcurrentJITLockh">trunk/Source/JavaScriptCore/runtime/ConcurrentJITLock.h</a></li>
<li><a href="#trunkSourceWTFChangeLog">trunk/Source/WTF/ChangeLog</a></li>
<li><a href="#trunkSourceWTFwtfLockerh">trunk/Source/WTF/wtf/Locker.h</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkSourceJavaScriptCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/ChangeLog (199847 => 199848)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/ChangeLog 2016-04-22 00:08:28 UTC (rev 199847)
+++ trunk/Source/JavaScriptCore/ChangeLog 2016-04-22 00:09:36 UTC (rev 199848)
</span><span class="lines">@@ -1,5 +1,52 @@
</span><span class="cx"> 2016-04-21 Saam barati <sbarati@apple.com>
</span><span class="cx">
</span><ins>+ 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 <sbarati@apple.com>
+
</ins><span class="cx"> Remove some unnecessary RefPtrs in the parser
</span><span class="cx"> https://bugs.webkit.org/show_bug.cgi?id=156865
</span><span class="cx">
</span></span></pre></div>
<a id="trunkSourceJavaScriptCorebytecodeUnlinkedFunctionExecutablecpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.cpp (199847 => 199848)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.cpp 2016-04-22 00:08:28 UTC (rev 199847)
+++ trunk/Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.cpp 2016-04-22 00:09:36 UTC (rev 199848)
</span><span class="lines">@@ -70,8 +70,8 @@
</span><span class="cx">
</span><span class="cx"> 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));
</span><span class="cx">
</span><del>- auto generator(std::make_unique<BytecodeGenerator>(vm, function.get(), result, debuggerMode, profilerMode, executable->parentScopeTDZVariables()));
- error = generator->generate();
</del><ins>+ error = BytecodeGenerator::generate(vm, function.get(), result, debuggerMode, profilerMode, executable->parentScopeTDZVariables());
+
</ins><span class="cx"> if (error.isValid())
</span><span class="cx"> return nullptr;
</span><span class="cx"> return result;
</span></span></pre></div>
<a id="trunkSourceJavaScriptCorebytecompilerBytecodeGeneratorcpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp (199847 => 199848)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp 2016-04-22 00:08:28 UTC (rev 199847)
+++ trunk/Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp 2016-04-22 00:09:36 UTC (rev 199848)
</span><span class="lines">@@ -365,6 +365,7 @@
</span><span class="cx"> // use DirectArguments. With ScopedArguments, we lift all of our arguments into the
</span><span class="cx"> // activation.
</span><span class="cx">
</span><ins>+ ConcurrentJITLocker locker(ConcurrentJITLocker::NoLockingNecessary);
</ins><span class="cx"> if (capturesAnyArgumentByName) {
</span><span class="cx"> functionSymbolTable->setArgumentsLength(vm, parameters.size());
</span><span class="cx">
</span><span class="lines">@@ -373,7 +374,7 @@
</span><span class="cx"> // in the symbol table - or it just gets space reserved in the symbol table. Either
</span><span class="cx"> // way we lift the value into the scope.
</span><span class="cx"> for (unsigned i = 0; i < parameters.size(); ++i) {
</span><del>- ScopeOffset offset = functionSymbolTable->takeNextScopeOffset();
</del><ins>+ ScopeOffset offset = functionSymbolTable->takeNextScopeOffset(locker);
</ins><span class="cx"> functionSymbolTable->setArgumentOffset(vm, i, offset);
</span><span class="cx"> if (UniquedStringImpl* name = visibleNameForParameter(parameters.at(i).first)) {
</span><span class="cx"> VarOffset varOffset(offset);
</span><span class="lines">@@ -383,7 +384,7 @@
</span><span class="cx"> // parameters when "arguments" is in play is unlikely to be super profitable.
</span><span class="cx"> // So, we just disable it.
</span><span class="cx"> entry.disableWatching();
</span><del>- functionSymbolTable->set(name, entry);
</del><ins>+ functionSymbolTable->set(locker, name, entry);
</ins><span class="cx"> }
</span><span class="cx"> emitOpcode(op_put_to_scope);
</span><span class="cx"> instructions().append(m_lexicalEnvironmentRegister->index());
</span><span class="lines">@@ -404,7 +405,7 @@
</span><span class="cx"> // that the symbol table knows that this is happening.
</span><span class="cx"> for (unsigned i = 0; i < parameters.size(); ++i) {
</span><span class="cx"> if (UniquedStringImpl* name = visibleNameForParameter(parameters.at(i).first))
</span><del>- functionSymbolTable->set(name, SymbolTableEntry(VarOffset(DirectArgumentsOffset(i))));
</del><ins>+ functionSymbolTable->set(locker, name, SymbolTableEntry(VarOffset(DirectArgumentsOffset(i))));
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> emitOpcode(op_create_direct_arguments);
</span><span class="lines">@@ -415,6 +416,7 @@
</span><span class="cx"> // captured, lift them into the scope. We cannot do this if we have default parameter expressions
</span><span class="cx"> // because when default parameter expressions exist, they belong in their own lexical environment
</span><span class="cx"> // separate from the "var" lexical environment.
</span><ins>+ ConcurrentJITLocker locker(ConcurrentJITLocker::NoLockingNecessary);
</ins><span class="cx"> for (unsigned i = 0; i < parameters.size(); ++i) {
</span><span class="cx"> UniquedStringImpl* name = visibleNameForParameter(parameters.at(i).first);
</span><span class="cx"> if (!name)
</span><span class="lines">@@ -423,14 +425,14 @@
</span><span class="cx"> if (!captures(name)) {
</span><span class="cx"> // This is the easy case - just tell the symbol table about the argument. It will
</span><span class="cx"> // be accessed directly.
</span><del>- functionSymbolTable->set(name, SymbolTableEntry(VarOffset(virtualRegisterForArgument(1 + i))));
</del><ins>+ functionSymbolTable->set(locker, name, SymbolTableEntry(VarOffset(virtualRegisterForArgument(1 + i))));
</ins><span class="cx"> continue;
</span><span class="cx"> }
</span><span class="cx">
</span><del>- ScopeOffset offset = functionSymbolTable->takeNextScopeOffset();
</del><ins>+ ScopeOffset offset = functionSymbolTable->takeNextScopeOffset(locker);
</ins><span class="cx"> const Identifier& ident =
</span><span class="cx"> static_cast<const BindingNode*>(parameters.at(i).first)->boundProperty();
</span><del>- functionSymbolTable->set(name, SymbolTableEntry(VarOffset(offset)));
</del><ins>+ functionSymbolTable->set(locker, name, SymbolTableEntry(VarOffset(offset)));
</ins><span class="cx">
</span><span class="cx"> emitOpcode(op_put_to_scope);
</span><span class="cx"> instructions().append(m_lexicalEnvironmentRegister->index());
</span><span class="lines">@@ -721,7 +723,7 @@
</span><span class="cx">
</span><span class="cx"> pushTDZVariables(lexicalVariables, TDZCheckOptimization::Optimize);
</span><span class="cx"> bool isWithScope = false;
</span><del>- m_symbolTableStack.append(SymbolTableStackEntry { Strong<SymbolTable>(*m_vm, moduleEnvironmentSymbolTable), m_topMostScope, isWithScope, constantSymbolTable->index() });
</del><ins>+ m_symbolTableStack.append(SymbolTableStackEntry { moduleEnvironmentSymbolTable, m_topMostScope, isWithScope, constantSymbolTable->index() });
</ins><span class="cx"> emitPrefillStackTDZVariables(lexicalVariables, moduleEnvironmentSymbolTable);
</span><span class="cx">
</span><span class="cx"> // makeFunction assumes that there's correct TDZ stack entries.
</span><span class="lines">@@ -859,7 +861,7 @@
</span><span class="cx">
</span><span class="cx"> if (m_lexicalEnvironmentRegister)
</span><span class="cx"> pushScopedControlFlowContext();
</span><del>- m_symbolTableStack.append(SymbolTableStackEntry{ Strong<SymbolTable>(*m_vm, functionSymbolTable), m_lexicalEnvironmentRegister, false, symbolTableConstantIndex });
</del><ins>+ m_symbolTableStack.append(SymbolTableStackEntry{ functionSymbolTable, m_lexicalEnvironmentRegister, false, symbolTableConstantIndex });
</ins><span class="cx">
</span><span class="cx"> m_varScopeSymbolTableIndex = m_symbolTableStack.size() - 1;
</span><span class="cx">
</span><span class="lines">@@ -883,19 +885,20 @@
</span><span class="cx"> if (!m_codeBlock->isArrowFunction()) {
</span><span class="cx"> ScopeOffset offset;
</span><span class="cx">
</span><ins>+ ConcurrentJITLocker locker(ConcurrentJITLocker::NoLockingNecessary);
</ins><span class="cx"> if (isThisUsedInInnerArrowFunction()) {
</span><del>- offset = symbolTable->takeNextScopeOffset();
- symbolTable->set(propertyNames().thisIdentifier.impl(), SymbolTableEntry(VarOffset(offset)));
</del><ins>+ offset = symbolTable->takeNextScopeOffset(locker);
+ symbolTable->set(locker, propertyNames().thisIdentifier.impl(), SymbolTableEntry(VarOffset(offset)));
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> if (m_codeType == FunctionCode && isNewTargetUsedInInnerArrowFunction()) {
</span><span class="cx"> offset = symbolTable->takeNextScopeOffset();
</span><del>- symbolTable->set(propertyNames().newTargetLocalPrivateName.impl(), SymbolTableEntry(VarOffset(offset)));
</del><ins>+ symbolTable->set(locker, propertyNames().newTargetLocalPrivateName.impl(), SymbolTableEntry(VarOffset(offset)));
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> if (isConstructor() && constructorKind() == ConstructorKind::Derived && isSuperUsedInInnerArrowFunction()) {
</span><del>- offset = symbolTable->takeNextScopeOffset();
- symbolTable->set(propertyNames().derivedConstructorPrivateName.impl(), SymbolTableEntry(VarOffset(offset)));
</del><ins>+ offset = symbolTable->takeNextScopeOffset(locker);
+ symbolTable->set(locker, propertyNames().derivedConstructorPrivateName.impl(), SymbolTableEntry(VarOffset(offset)));
</ins><span class="cx"> }
</span><span class="cx"> }
</span><span class="cx">
</span><span class="lines">@@ -1735,7 +1738,7 @@
</span><span class="cx"> {
</span><span class="cx"> bool hasCapturedVariables = false;
</span><span class="cx"> {
</span><del>- ConcurrentJITLocker locker(symbolTable->m_lock);
</del><ins>+ ConcurrentJITLocker locker(ConcurrentJITLocker::NoLockingNecessary);
</ins><span class="cx"> for (auto& entry : lexicalVariables) {
</span><span class="cx"> ASSERT(entry.value.isLet() || entry.value.isConst() || entry.value.isFunction());
</span><span class="cx"> ASSERT(!entry.value.isVar());
</span><span class="lines">@@ -1776,6 +1779,7 @@
</span><span class="cx"> {
</span><span class="cx"> // Prefill stack variables with the TDZ empty value.
</span><span class="cx"> // Scope variables will be initialized to the TDZ empty value when JSLexicalEnvironment is allocated.
</span><ins>+ ConcurrentJITLocker locker(ConcurrentJITLocker::NoLockingNecessary);
</ins><span class="cx"> for (auto& entry : lexicalVariables) {
</span><span class="cx"> // Imported bindings which are not the namespace bindings are not allocated
</span><span class="cx"> // in the module environment as usual variables' way.
</span><span class="lines">@@ -1787,7 +1791,7 @@
</span><span class="cx"> if (entry.value.isFunction())
</span><span class="cx"> continue;
</span><span class="cx">
</span><del>- SymbolTableEntry symbolTableEntry = symbolTable->get(entry.key.get());
</del><ins>+ SymbolTableEntry symbolTableEntry = symbolTable->get(locker, entry.key.get());
</ins><span class="cx"> ASSERT(!symbolTableEntry.isNull());
</span><span class="cx"> VarOffset offset = symbolTableEntry.varOffset();
</span><span class="cx"> if (offset.isScope())
</span><span class="lines">@@ -1820,7 +1824,7 @@
</span><span class="cx"> if (m_shouldEmitDebugHooks)
</span><span class="cx"> environment.markAllVariablesAsCaptured();
</span><span class="cx">
</span><del>- Strong<SymbolTable> symbolTable(*m_vm, SymbolTable::create(*m_vm));
</del><ins>+ SymbolTable* symbolTable = SymbolTable::create(*m_vm);
</ins><span class="cx"> switch (scopeType) {
</span><span class="cx"> case ScopeType::CatchScope:
</span><span class="cx"> symbolTable->setScopeType(SymbolTable::ScopeType::CatchScope);
</span><span class="lines">@@ -1840,13 +1844,13 @@
</span><span class="cx"> return entry.isCaptured() ? VarKind::Scope : VarKind::Stack;
</span><span class="cx"> };
</span><span class="cx">
</span><del>- bool hasCapturedVariables = instantiateLexicalVariables(environment, symbolTable.get(), scopeRegisterType, lookUpVarKind);
</del><ins>+ bool hasCapturedVariables = instantiateLexicalVariables(environment, symbolTable, scopeRegisterType, lookUpVarKind);
</ins><span class="cx">
</span><span class="cx"> RegisterID* newScope = nullptr;
</span><span class="cx"> RegisterID* constantSymbolTable = nullptr;
</span><span class="cx"> int symbolTableConstantIndex = 0;
</span><span class="cx"> if (vm()->typeProfiler()) {
</span><del>- constantSymbolTable = addConstantValue(symbolTable.get());
</del><ins>+ constantSymbolTable = addConstantValue(symbolTable);
</ins><span class="cx"> symbolTableConstantIndex = constantSymbolTable->index();
</span><span class="cx"> }
</span><span class="cx"> if (hasCapturedVariables) {
</span><span class="lines">@@ -1880,7 +1884,7 @@
</span><span class="cx"> pushTDZVariables(environment, tdzCheckOptimization);
</span><span class="cx">
</span><span class="cx"> if (tdzRequirement == TDZRequirement::UnderTDZ)
</span><del>- emitPrefillStackTDZVariables(environment, symbolTable.get());
</del><ins>+ emitPrefillStackTDZVariables(environment, symbolTable);
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> void BytecodeGenerator::initializeBlockScopedFunctions(VariableEnvironment& environment, FunctionStack& functionStack, RegisterID* constantSymbolTable)
</span><span class="lines">@@ -1919,17 +1923,18 @@
</span><span class="cx"> if (!functionStack.size())
</span><span class="cx"> return;
</span><span class="cx">
</span><del>- SymbolTable* symbolTable = m_symbolTableStack.last().m_symbolTable.get();
</del><ins>+ SymbolTable* symbolTable = m_symbolTableStack.last().m_symbolTable;
</ins><span class="cx"> RegisterID* scope = m_symbolTableStack.last().m_scope;
</span><span class="cx"> RefPtr<RegisterID> temp = newTemporary();
</span><span class="cx"> int symbolTableIndex = constantSymbolTable ? constantSymbolTable->index() : 0;
</span><ins>+ ConcurrentJITLocker locker(ConcurrentJITLocker::NoLockingNecessary);
</ins><span class="cx"> for (FunctionMetadataNode* function : functionStack) {
</span><span class="cx"> const Identifier& name = function->ident();
</span><span class="cx"> auto iter = environment.find(name.impl());
</span><span class="cx"> RELEASE_ASSERT(iter != environment.end());
</span><span class="cx"> RELEASE_ASSERT(iter->value.isFunction());
</span><span class="cx"> // We purposefully don't hold the symbol table lock around this loop because emitNewFunctionExpressionCommon may GC.
</span><del>- SymbolTableEntry entry = symbolTable->get(name.impl());
</del><ins>+ SymbolTableEntry entry = symbolTable->get(locker, name.impl());
</ins><span class="cx"> RELEASE_ASSERT(!entry.isNull());
</span><span class="cx"> emitNewFunctionExpressionCommon(temp.get(), function);
</span><span class="cx"> bool isLexicallyScoped = true;
</span><span class="lines">@@ -1952,9 +1957,10 @@
</span><span class="cx"> ASSERT(m_varScopeSymbolTableIndex);
</span><span class="cx"> ASSERT(*m_varScopeSymbolTableIndex < m_symbolTableStack.size());
</span><span class="cx"> SymbolTableStackEntry& varScope = m_symbolTableStack[*m_varScopeSymbolTableIndex];
</span><del>- SymbolTable* varSymbolTable = varScope.m_symbolTable.get();
</del><ins>+ SymbolTable* varSymbolTable = varScope.m_symbolTable;
</ins><span class="cx"> ASSERT(varSymbolTable->scopeType() == SymbolTable::ScopeType::VarScope);
</span><del>- SymbolTableEntry entry = varSymbolTable->get(functionName.impl());
</del><ins>+ ConcurrentJITLocker locker(ConcurrentJITLocker::NoLockingNecessary);
+ SymbolTableEntry entry = varSymbolTable->get(locker, functionName.impl());
</ins><span class="cx"> ASSERT(!entry.isNull());
</span><span class="cx"> bool isLexicallyScoped = false;
</span><span class="cx"> emitPutToScope(varScope.m_scope, variableForLocalEntry(functionName, entry, varScope.m_symbolTableConstantIndex, isLexicallyScoped), currentValue.get(), DoNotThrowIfNotFound, InitializationMode::NotInitialization);
</span><span class="lines">@@ -1978,9 +1984,9 @@
</span><span class="cx"> environment.markAllVariablesAsCaptured();
</span><span class="cx">
</span><span class="cx"> SymbolTableStackEntry stackEntry = m_symbolTableStack.takeLast();
</span><del>- Strong<SymbolTable> symbolTable = stackEntry.m_symbolTable;
- ConcurrentJITLocker locker(symbolTable->m_lock);
</del><ins>+ SymbolTable* symbolTable = stackEntry.m_symbolTable;
</ins><span class="cx"> bool hasCapturedVariables = false;
</span><ins>+ ConcurrentJITLocker locker(ConcurrentJITLocker::NoLockingNecessary);
</ins><span class="cx"> for (auto& entry : environment) {
</span><span class="cx"> if (entry.value.isCaptured()) {
</span><span class="cx"> hasCapturedVariables = true;
</span><span class="lines">@@ -2025,16 +2031,16 @@
</span><span class="cx"> // gets a new activation.
</span><span class="cx">
</span><span class="cx"> SymbolTableStackEntry stackEntry = m_symbolTableStack.last();
</span><del>- Strong<SymbolTable> symbolTable = stackEntry.m_symbolTable;
</del><ins>+ SymbolTable* symbolTable = stackEntry.m_symbolTable;
</ins><span class="cx"> RegisterID* loopScope = stackEntry.m_scope;
</span><span class="cx"> ASSERT(symbolTable->scopeSize());
</span><span class="cx"> ASSERT(loopScope);
</span><span class="cx"> Vector<std::pair<RegisterID*, Identifier>> activationValuesToCopyOver;
</span><span class="cx">
</span><span class="cx"> {
</span><del>- ConcurrentJITLocker locker(symbolTable->m_lock);
</del><span class="cx"> activationValuesToCopyOver.reserveInitialCapacity(symbolTable->scopeSize());
</span><span class="cx">
</span><ins>+ ConcurrentJITLocker locker(ConcurrentJITLocker::NoLockingNecessary);
</ins><span class="cx"> for (auto end = symbolTable->end(locker), ptr = symbolTable->begin(locker); ptr != end; ++ptr) {
</span><span class="cx"> if (!ptr->value.varOffset().isScope())
</span><span class="cx"> continue;
</span><span class="lines">@@ -2067,7 +2073,7 @@
</span><span class="cx"> emitMove(scopeRegister(), loopScope);
</span><span class="cx">
</span><span class="cx"> {
</span><del>- ConcurrentJITLocker locker(symbolTable->m_lock);
</del><ins>+ ConcurrentJITLocker locker(ConcurrentJITLocker::NoLockingNecessary);
</ins><span class="cx"> for (auto pair : activationValuesToCopyOver) {
</span><span class="cx"> const Identifier& identifier = pair.second;
</span><span class="cx"> SymbolTableEntry entry = symbolTable->get(locker, identifier.impl());
</span><span class="lines">@@ -2105,12 +2111,13 @@
</span><span class="cx"> // doSomethingWith(x);
</span><span class="cx"> // }
</span><span class="cx"> // }
</span><ins>+ ConcurrentJITLocker locker(ConcurrentJITLocker::NoLockingNecessary);
</ins><span class="cx"> for (unsigned i = m_symbolTableStack.size(); i--; ) {
</span><span class="cx"> SymbolTableStackEntry& stackEntry = m_symbolTableStack[i];
</span><span class="cx"> if (stackEntry.m_isWithScope)
</span><span class="cx"> return Variable(property);
</span><del>- Strong<SymbolTable>& symbolTable = stackEntry.m_symbolTable;
- SymbolTableEntry symbolTableEntry = symbolTable->get(property.impl());
</del><ins>+ SymbolTable* symbolTable = stackEntry.m_symbolTable;
+ SymbolTableEntry symbolTableEntry = symbolTable->get(locker, property.impl());
</ins><span class="cx"> if (symbolTableEntry.isNull())
</span><span class="cx"> continue;
</span><span class="cx"> bool resultIsCallee = false;
</span><span class="lines">@@ -2150,7 +2157,7 @@
</span><span class="cx"> const Identifier& property, VarKind varKind, SymbolTable* symbolTable, ExistingVariableMode existingVariableMode)
</span><span class="cx"> {
</span><span class="cx"> ASSERT(property != propertyNames().thisIdentifier);
</span><del>- ConcurrentJITLocker locker(symbolTable->m_lock);
</del><ins>+ ConcurrentJITLocker locker(ConcurrentJITLocker::NoLockingNecessary);
</ins><span class="cx"> SymbolTableEntry entry = symbolTable->get(locker, property.impl());
</span><span class="cx">
</span><span class="cx"> if (!entry.isNull()) {
</span><span class="lines">@@ -2227,20 +2234,21 @@
</span><span class="cx"> case VarKind::DirectArgument:
</span><span class="cx"> return argumentsRegister();
</span><span class="cx">
</span><del>- case VarKind::Scope:
</del><ins>+ case VarKind::Scope: {
</ins><span class="cx"> // This always refers to the activation that *we* allocated, and not the current scope that code
</span><span class="cx"> // lives in. Note that this will change once we have proper support for block scoping. Once that
</span><span class="cx"> // changes, it will be correct for this code to return scopeRegister(). The only reason why we
</span><span class="cx"> // don't do that already is that m_lexicalEnvironment is required by ConstDeclNode. ConstDeclNode
</span><span class="cx"> // requires weird things because it is a shameful pile of nonsense, but block scoping would make
</span><span class="cx"> // that code sensible and obviate the need for us to do bad things.
</span><ins>+ ConcurrentJITLocker locker(ConcurrentJITLocker::NoLockingNecessary);
</ins><span class="cx"> for (unsigned i = m_symbolTableStack.size(); i--; ) {
</span><span class="cx"> SymbolTableStackEntry& stackEntry = m_symbolTableStack[i];
</span><span class="cx"> // We should not resolve a variable to VarKind::Scope if a "with" scope lies in between the current
</span><span class="cx"> // scope and the resolved scope.
</span><span class="cx"> RELEASE_ASSERT(!stackEntry.m_isWithScope);
</span><span class="cx">
</span><del>- if (stackEntry.m_symbolTable->get(variable.ident().impl()).isNull())
</del><ins>+ if (stackEntry.m_symbolTable->get(locker, variable.ident().impl()).isNull())
</ins><span class="cx"> continue;
</span><span class="cx">
</span><span class="cx"> RegisterID* scope = stackEntry.m_scope;
</span><span class="lines">@@ -2251,6 +2259,7 @@
</span><span class="cx"> RELEASE_ASSERT_NOT_REACHED();
</span><span class="cx"> return nullptr;
</span><span class="cx">
</span><ins>+ }
</ins><span class="cx"> case VarKind::Invalid:
</span><span class="cx"> // Indicates non-local resolution.
</span><span class="cx">
</span><span class="lines">@@ -3274,7 +3283,7 @@
</span><span class="cx"> instructions().append(scopeRegister()->index());
</span><span class="cx">
</span><span class="cx"> emitMove(scopeRegister(), newScope);
</span><del>- m_symbolTableStack.append(SymbolTableStackEntry{ Strong<SymbolTable>(), newScope, true, 0 });
</del><ins>+ m_symbolTableStack.append(SymbolTableStackEntry{ nullptr, newScope, true, 0 });
</ins><span class="cx">
</span><span class="cx"> return newScope;
</span><span class="cx"> }
</span><span class="lines">@@ -3738,7 +3747,8 @@
</span><span class="cx"> pushLexicalScopeInternal(nameScopeEnvironment, TDZCheckOptimization::Optimize, NestedScopeType::IsNotNested, nullptr, TDZRequirement::NotUnderTDZ, ScopeType::FunctionNameScope, ScopeRegisterType::Var);
</span><span class="cx"> ASSERT_UNUSED(numVars, m_codeBlock->m_numVars == static_cast<int>(numVars + 1)); // Should have only created one new "var" for the function name scope.
</span><span class="cx"> bool shouldTreatAsLexicalVariable = isStrictMode();
</span><del>- Variable functionVar = variableForLocalEntry(property, m_symbolTableStack.last().m_symbolTable->get(property.impl()), m_symbolTableStack.last().m_symbolTableConstantIndex, shouldTreatAsLexicalVariable);
</del><ins>+ ConcurrentJITLocker locker(ConcurrentJITLocker::NoLockingNecessary);
+ Variable functionVar = variableForLocalEntry(property, m_symbolTableStack.last().m_symbolTable->get(locker, property.impl()), m_symbolTableStack.last().m_symbolTableConstantIndex, shouldTreatAsLexicalVariable);
</ins><span class="cx"> emitPutToScope(m_symbolTableStack.last().m_scope, functionVar, callee, ThrowIfNotFound, InitializationMode::NotInitialization);
</span><span class="cx"> }
</span><span class="cx">
</span></span></pre></div>
<a id="trunkSourceJavaScriptCorebytecompilerBytecodeGeneratorh"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h (199847 => 199848)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h 2016-04-22 00:08:28 UTC (rev 199847)
+++ trunk/Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h 2016-04-22 00:09:36 UTC (rev 199848)
</span><span class="lines">@@ -289,7 +289,13 @@
</span><span class="cx"> ConstructorKind constructorKind() const { return m_codeBlock->constructorKind(); }
</span><span class="cx"> SuperBinding superBinding() const { return m_codeBlock->superBinding(); }
</span><span class="cx">
</span><del>- ParserError generate();
</del><ins>+ 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();
+ }
</ins><span class="cx">
</span><span class="cx"> bool isArgumentNumber(const Identifier&, int);
</span><span class="cx">
</span><span class="lines">@@ -737,6 +743,7 @@
</span><span class="cx"> int labelScopeDepth() const;
</span><span class="cx">
</span><span class="cx"> private:
</span><ins>+ ParserError generate();
</ins><span class="cx"> void reclaimFreeRegisters();
</span><span class="cx"> Variable variableForLocalEntry(const Identifier&, const SymbolTableEntry&, int symbolTableConstantIndex, bool isLexicallyScoped);
</span><span class="cx">
</span><span class="lines">@@ -862,7 +869,7 @@
</span><span class="cx"> bool m_shouldEmitProfileHooks;
</span><span class="cx">
</span><span class="cx"> struct SymbolTableStackEntry {
</span><del>- Strong<SymbolTable> m_symbolTable;
</del><ins>+ SymbolTable* m_symbolTable;
</ins><span class="cx"> RegisterID* m_scope;
</span><span class="cx"> bool m_isWithScope;
</span><span class="cx"> int m_symbolTableConstantIndex;
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreruntimeCodeCachecpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/runtime/CodeCache.cpp (199847 => 199848)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/runtime/CodeCache.cpp 2016-04-22 00:08:28 UTC (rev 199847)
+++ trunk/Source/JavaScriptCore/runtime/CodeCache.cpp 2016-04-22 00:09:36 UTC (rev 199848)
</span><span class="lines">@@ -119,8 +119,8 @@
</span><span class="cx"> UnlinkedCodeBlockType* unlinkedCodeBlock = UnlinkedCodeBlockType::create(&vm, executable->executableInfo());
</span><span class="cx"> unlinkedCodeBlock->recordParse(rootNode->features(), rootNode->hasCapturedVariables(), rootNode->firstLine() - source.firstLine(), lineCount, unlinkedEndColumn);
</span><span class="cx">
</span><del>- auto generator = std::make_unique<BytecodeGenerator>(vm, rootNode.get(), unlinkedCodeBlock, debuggerMode, profilerMode, variablesUnderTDZ);
- error = generator->generate();
</del><ins>+ error = BytecodeGenerator::generate(vm, rootNode.get(), unlinkedCodeBlock, debuggerMode, profilerMode, variablesUnderTDZ);
+
</ins><span class="cx"> if (error.isValid())
</span><span class="cx"> return nullptr;
</span><span class="cx">
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreruntimeConcurrentJITLockh"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/runtime/ConcurrentJITLock.h (199847 => 199848)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/runtime/ConcurrentJITLock.h 2016-04-22 00:08:28 UTC (rev 199847)
+++ trunk/Source/JavaScriptCore/runtime/ConcurrentJITLock.h 2016-04-22 00:09:36 UTC (rev 199848)
</span><span class="lines">@@ -29,6 +29,7 @@
</span><span class="cx"> #include "DeferGC.h"
</span><span class="cx"> #include <wtf/Lock.h>
</span><span class="cx"> #include <wtf/NoLock.h>
</span><ins>+#include <wtf/Optional.h>
</ins><span class="cx">
</span><span class="cx"> namespace JSC {
</span><span class="cx">
</span><span class="lines">@@ -52,6 +53,12 @@
</span><span class="cx"> {
</span><span class="cx"> }
</span><span class="cx">
</span><ins>+ enum NoLockingNecessaryTag { NoLockingNecessary };
+ explicit ConcurrentJITLockerBase(NoLockingNecessaryTag)
+ : m_locker(ConcurrentJITLockerImpl::NoLockingNecessary)
+ {
+ }
+
</ins><span class="cx"> ~ConcurrentJITLockerBase()
</span><span class="cx"> {
</span><span class="cx"> }
</span><span class="lines">@@ -104,17 +111,31 @@
</span><span class="cx"> public:
</span><span class="cx"> ConcurrentJITLocker(ConcurrentJITLock& lockable)
</span><span class="cx"> : ConcurrentJITLockerBase(lockable)
</span><ins>+#if ENABLE(CONCURRENT_JIT) && !defined(NDEBUG)
+ , m_disallowGC(InPlace)
+#endif
</ins><span class="cx"> {
</span><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> ConcurrentJITLocker(ConcurrentJITLock* lockable)
</span><span class="cx"> : ConcurrentJITLockerBase(lockable)
</span><ins>+#if ENABLE(CONCURRENT_JIT) && !defined(NDEBUG)
+ , m_disallowGC(InPlace)
+#endif
</ins><span class="cx"> {
</span><span class="cx"> }
</span><span class="cx">
</span><ins>+ ConcurrentJITLocker(ConcurrentJITLockerBase::NoLockingNecessaryTag)
+ : ConcurrentJITLockerBase(ConcurrentJITLockerBase::NoLockingNecessary)
</ins><span class="cx"> #if ENABLE(CONCURRENT_JIT) && !defined(NDEBUG)
</span><ins>+ , m_disallowGC(Nullopt)
+#endif
+ {
+ }
+
+#if ENABLE(CONCURRENT_JIT) && !defined(NDEBUG)
</ins><span class="cx"> private:
</span><del>- DisallowGC m_disallowGC;
</del><ins>+ Optional<DisallowGC> m_disallowGC;
</ins><span class="cx"> #endif
</span><span class="cx"> };
</span><span class="cx">
</span></span></pre></div>
<a id="trunkSourceWTFChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/WTF/ChangeLog (199847 => 199848)</h4>
<pre class="diff"><span>
<span class="info">--- 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)
</span><span class="lines">@@ -1,3 +1,25 @@
</span><ins>+2016-04-21 Saam barati <sbarati@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):
+
</ins><span class="cx"> 2016-04-19 Michael Saboff <msaboff@apple.com>
</span><span class="cx">
</span><span class="cx"> iTunes crashing JavaScriptCore.dll
</span></span></pre></div>
<a id="trunkSourceWTFwtfLockerh"></a>
<div class="modfile"><h4>Modified: trunk/Source/WTF/wtf/Locker.h (199847 => 199848)</h4>
<pre class="diff"><span>
<span class="info">--- 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)
</span><span class="lines">@@ -37,6 +37,15 @@
</span><span class="cx"> public:
</span><span class="cx"> explicit Locker(T& lockable) : m_lockable(&lockable) { lock(); }
</span><span class="cx"> explicit Locker(T* lockable) : m_lockable(lockable) { lock(); }
</span><ins>+
+ 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) { }
+
</ins><span class="cx"> ~Locker()
</span><span class="cx"> {
</span><span class="cx"> if (m_lockable)
</span></span></pre>
</div>
</div>
</body>
</html>