Make it easier to use NoLockingNecessary
authorfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 2 Jun 2016 18:41:16 +0000 (18:41 +0000)
committerfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 2 Jun 2016 18:41:16 +0000 (18:41 +0000)
https://bugs.webkit.org/show_bug.cgi?id=158306

Reviewed by Keith Miller.

Source/JavaScriptCore:

Adapt to the new NoLockingNecessary API. More details in the WTF ChangeLog.

* bytecompiler/BytecodeGenerator.cpp:
(JSC::BytecodeGenerator::BytecodeGenerator):
(JSC::BytecodeGenerator::initializeArrowFunctionContextScopeIfNeeded):
(JSC::BytecodeGenerator::instantiateLexicalVariables):
(JSC::BytecodeGenerator::emitPrefillStackTDZVariables):
(JSC::BytecodeGenerator::initializeBlockScopedFunctions):
(JSC::BytecodeGenerator::hoistSloppyModeFunctionIfNecessary):
(JSC::BytecodeGenerator::popLexicalScopeInternal):
(JSC::BytecodeGenerator::prepareLexicalScopeForNextForLoopIteration):
(JSC::BytecodeGenerator::variable):
(JSC::BytecodeGenerator::createVariable):
(JSC::BytecodeGenerator::emitResolveScope):
(JSC::BytecodeGenerator::emitPushFunctionNameScope):
* runtime/ConcurrentJITLock.h:
(JSC::ConcurrentJITLockerBase::ConcurrentJITLockerBase):
(JSC::ConcurrentJITLocker::ConcurrentJITLocker):

Source/WTF:

An idiom that we borrowed from LLVM is that if a function requires a lock to be held, we
have it take a const Locker& as its first argument. This may not communicate which lock is
to be held, but it does help us to remember that some lock must be held. So far, it's been
relatively easy to then figure out which lock. We've had bugs where we forgot to hold a
lock but I don't remember the last time we had a bug where we grabbed the wrong lock.

But sometimes, we know at the point where we call such a method that we actually don't
need to hold any lock. This usually happens during object construction. If we're
constructing some object then we usually know that we have not escaped it yet, so we don't
need to waste time acquiring its lock. We could solve this by having a separate set of
methods that don't do or require locking. This would be cumbersome, since usually for
every variant that takes const Locker&, there is already one that doesn't, and that one
will grab the lock for you. So this means having a third variant, that also doesn't take a
const Locker&, but does no locking. That's pretty weird.

So, we introduced NoLockingNecessary for situations like this. The idiom went like so:

    Locker<Whatever> locker(Locker<Whatever>::NoLockingNecessary)
    stuff->foo(locker);

Usually though, there would be some distance between where the locker is defined and where
it's used, so when you just look at stuff->foo(locker) in isolation you don't know if this
is a real locker or a NoLockingNecessary cast. Also, requiring two lines for this just
adds code.

This change makes this easier. Now you can just do:

    stuff->foo(NoLockingNecessary).

This is because NoLockingNecessary has been pulled out into the WTF namespace (and is
usinged from the global namespace) and the Locker<> constructor that takes
NoLockingNecessaryTag is now implicit.

The only possible downside of this change is that people might use this idiom more
frequently now that it's easier to use. I don't think that's a bad thing. I'm now
convinced that this is not a bad idiom. When I was fixing an unrelated bug, I almost went
the way of adding more locking to some core JSC data structures, and in the process, I
needed to use NoLockingNecessary. It's clear that this is a general-purpose idiom and we
should not impose artificial constraints on its use.

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

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp
Source/JavaScriptCore/runtime/ConcurrentJITLock.h
Source/WTF/ChangeLog
Source/WTF/wtf/Locker.h

index f89467c..4e1c8ce 100644 (file)
@@ -1,3 +1,29 @@
+2016-06-02  Filip Pizlo  <fpizlo@apple.com>
+
+        Make it easier to use NoLockingNecessary
+        https://bugs.webkit.org/show_bug.cgi?id=158306
+
+        Reviewed by Keith Miller.
+        
+        Adapt to the new NoLockingNecessary API. More details in the WTF ChangeLog.
+
+        * bytecompiler/BytecodeGenerator.cpp:
+        (JSC::BytecodeGenerator::BytecodeGenerator):
+        (JSC::BytecodeGenerator::initializeArrowFunctionContextScopeIfNeeded):
+        (JSC::BytecodeGenerator::instantiateLexicalVariables):
+        (JSC::BytecodeGenerator::emitPrefillStackTDZVariables):
+        (JSC::BytecodeGenerator::initializeBlockScopedFunctions):
+        (JSC::BytecodeGenerator::hoistSloppyModeFunctionIfNecessary):
+        (JSC::BytecodeGenerator::popLexicalScopeInternal):
+        (JSC::BytecodeGenerator::prepareLexicalScopeForNextForLoopIteration):
+        (JSC::BytecodeGenerator::variable):
+        (JSC::BytecodeGenerator::createVariable):
+        (JSC::BytecodeGenerator::emitResolveScope):
+        (JSC::BytecodeGenerator::emitPushFunctionNameScope):
+        * runtime/ConcurrentJITLock.h:
+        (JSC::ConcurrentJITLockerBase::ConcurrentJITLockerBase):
+        (JSC::ConcurrentJITLocker::ConcurrentJITLocker):
+
 2016-06-01  Filip Pizlo  <fpizlo@apple.com>
 
         Structure::previousID() races with Structure::allocateRareData()
index dbc710f..25b8708 100644 (file)
@@ -368,7 +368,6 @@ BytecodeGenerator::BytecodeGenerator(VM& vm, FunctionNode* functionNode, Unlinke
         // use DirectArguments. With ScopedArguments, we lift all of our arguments into the
         // activation.
         
-        ConcurrentJITLocker locker(ConcurrentJITLocker::NoLockingNecessary);
         if (capturesAnyArgumentByName) {
             functionSymbolTable->setArgumentsLength(vm, parameters.size());
             
@@ -377,7 +376,7 @@ BytecodeGenerator::BytecodeGenerator(VM& vm, FunctionNode* functionNode, Unlinke
             // 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(locker);
+                ScopeOffset offset = functionSymbolTable->takeNextScopeOffset(NoLockingNecessary);
                 functionSymbolTable->setArgumentOffset(vm, i, offset);
                 if (UniquedStringImpl* name = visibleNameForParameter(parameters.at(i).first)) {
                     VarOffset varOffset(offset);
@@ -387,7 +386,7 @@ BytecodeGenerator::BytecodeGenerator(VM& vm, FunctionNode* functionNode, Unlinke
                     // parameters when "arguments" is in play is unlikely to be super profitable.
                     // So, we just disable it.
                     entry.disableWatching();
-                    functionSymbolTable->set(locker, name, entry);
+                    functionSymbolTable->set(NoLockingNecessary, name, entry);
                 }
                 emitOpcode(op_put_to_scope);
                 instructions().append(m_lexicalEnvironmentRegister->index());
@@ -408,7 +407,7 @@ BytecodeGenerator::BytecodeGenerator(VM& vm, FunctionNode* functionNode, Unlinke
             // 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(locker, name, SymbolTableEntry(VarOffset(DirectArgumentsOffset(i))));
+                    functionSymbolTable->set(NoLockingNecessary, name, SymbolTableEntry(VarOffset(DirectArgumentsOffset(i))));
             }
             
             emitOpcode(op_create_direct_arguments);
@@ -419,7 +418,6 @@ BytecodeGenerator::BytecodeGenerator(VM& vm, FunctionNode* functionNode, Unlinke
         // 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)
@@ -428,14 +426,14 @@ BytecodeGenerator::BytecodeGenerator(VM& vm, FunctionNode* functionNode, Unlinke
             if (!captures(name)) {
                 // This is the easy case - just tell the symbol table about the argument. It will
                 // be accessed directly.
-                functionSymbolTable->set(locker, name, SymbolTableEntry(VarOffset(virtualRegisterForArgument(1 + i))));
+                functionSymbolTable->set(NoLockingNecessary, name, SymbolTableEntry(VarOffset(virtualRegisterForArgument(1 + i))));
                 continue;
             }
             
-            ScopeOffset offset = functionSymbolTable->takeNextScopeOffset(locker);
+            ScopeOffset offset = functionSymbolTable->takeNextScopeOffset(NoLockingNecessary);
             const Identifier& ident =
                 static_cast<const BindingNode*>(parameters.at(i).first)->boundProperty();
-            functionSymbolTable->set(locker, name, SymbolTableEntry(VarOffset(offset)));
+            functionSymbolTable->set(NoLockingNecessary, name, SymbolTableEntry(VarOffset(offset)));
             
             emitOpcode(op_put_to_scope);
             instructions().append(m_lexicalEnvironmentRegister->index());
@@ -905,20 +903,19 @@ void BytecodeGenerator::initializeArrowFunctionContextScopeIfNeeded(SymbolTable*
         
         ScopeOffset offset;
         
-        ConcurrentJITLocker locker(ConcurrentJITLocker::NoLockingNecessary);
         if (isThisUsedInInnerArrowFunction()) {
-            offset = functionSymbolTable->takeNextScopeOffset(locker);
-            functionSymbolTable->set(locker, propertyNames().thisIdentifier.impl(), SymbolTableEntry(VarOffset(offset)));
+            offset = functionSymbolTable->takeNextScopeOffset(NoLockingNecessary);
+            functionSymbolTable->set(NoLockingNecessary, propertyNames().thisIdentifier.impl(), SymbolTableEntry(VarOffset(offset)));
         }
 
         if (m_codeType == FunctionCode && isNewTargetUsedInInnerArrowFunction()) {
             offset = functionSymbolTable->takeNextScopeOffset();
-            functionSymbolTable->set(locker, propertyNames().newTargetLocalPrivateName.impl(), SymbolTableEntry(VarOffset(offset)));
+            functionSymbolTable->set(NoLockingNecessary, propertyNames().newTargetLocalPrivateName.impl(), SymbolTableEntry(VarOffset(offset)));
         }
         
         if (isConstructor() && constructorKind() == ConstructorKind::Derived && isSuperUsedInInnerArrowFunction()) {
-            offset = functionSymbolTable->takeNextScopeOffset(locker);
-            functionSymbolTable->set(locker, propertyNames().derivedConstructorPrivateName.impl(), SymbolTableEntry(VarOffset(offset)));
+            offset = functionSymbolTable->takeNextScopeOffset(NoLockingNecessary);
+            functionSymbolTable->set(NoLockingNecessary, propertyNames().derivedConstructorPrivateName.impl(), SymbolTableEntry(VarOffset(offset)));
         }
 
         return;
@@ -1764,11 +1761,10 @@ bool BytecodeGenerator::instantiateLexicalVariables(const VariableEnvironment& l
 {
     bool hasCapturedVariables = false;
     {
-        ConcurrentJITLocker locker(ConcurrentJITLocker::NoLockingNecessary);
         for (auto& entry : lexicalVariables) {
             ASSERT(entry.value.isLet() || entry.value.isConst() || entry.value.isFunction());
             ASSERT(!entry.value.isVar());
-            SymbolTableEntry symbolTableEntry = symbolTable->get(locker, entry.key.get());
+            SymbolTableEntry symbolTableEntry = symbolTable->get(NoLockingNecessary, entry.key.get());
             ASSERT(symbolTableEntry.isNull());
 
             // Imported bindings which are not the namespace bindings are not allocated
@@ -1781,7 +1777,7 @@ bool BytecodeGenerator::instantiateLexicalVariables(const VariableEnvironment& l
             VarKind varKind = lookUpVarKind(entry.key.get(), entry.value);
             VarOffset varOffset;
             if (varKind == VarKind::Scope) {
-                varOffset = VarOffset(symbolTable->takeNextScopeOffset(locker));
+                varOffset = VarOffset(symbolTable->takeNextScopeOffset(NoLockingNecessary));
                 hasCapturedVariables = true;
             } else {
                 ASSERT(varKind == VarKind::Stack);
@@ -1795,7 +1791,7 @@ bool BytecodeGenerator::instantiateLexicalVariables(const VariableEnvironment& l
             }
 
             SymbolTableEntry newEntry(varOffset, entry.value.isConst() ? ReadOnly : 0);
-            symbolTable->add(locker, entry.key.get(), newEntry);
+            symbolTable->add(NoLockingNecessary, entry.key.get(), newEntry);
         }
     }
     return hasCapturedVariables;
@@ -1805,7 +1801,6 @@ void BytecodeGenerator::emitPrefillStackTDZVariables(const VariableEnvironment&
 {
     // 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.
@@ -1817,7 +1812,7 @@ void BytecodeGenerator::emitPrefillStackTDZVariables(const VariableEnvironment&
         if (entry.value.isFunction())
             continue;
 
-        SymbolTableEntry symbolTableEntry = symbolTable->get(locker, entry.key.get());
+        SymbolTableEntry symbolTableEntry = symbolTable->get(NoLockingNecessary, entry.key.get());
         ASSERT(!symbolTableEntry.isNull());
         VarOffset offset = symbolTableEntry.varOffset();
         if (offset.isScope())
@@ -1953,14 +1948,13 @@ void BytecodeGenerator::initializeBlockScopedFunctions(VariableEnvironment& envi
     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(locker, name.impl()); 
+        SymbolTableEntry entry = symbolTable->get(NoLockingNecessary, name.impl()); 
         RELEASE_ASSERT(!entry.isNull());
         emitNewFunctionExpressionCommon(temp.get(), function);
         bool isLexicallyScoped = true;
@@ -1985,8 +1979,7 @@ void BytecodeGenerator::hoistSloppyModeFunctionIfNecessary(const Identifier& fun
         SymbolTableStackEntry& varScope = m_symbolTableStack[*m_varScopeSymbolTableIndex];
         SymbolTable* varSymbolTable = varScope.m_symbolTable;
         ASSERT(varSymbolTable->scopeType() == SymbolTable::ScopeType::VarScope);
-        ConcurrentJITLocker locker(ConcurrentJITLocker::NoLockingNecessary);
-        SymbolTableEntry entry = varSymbolTable->get(locker, functionName.impl());
+        SymbolTableEntry entry = varSymbolTable->get(NoLockingNecessary, functionName.impl());
         ASSERT(!entry.isNull());
         bool isLexicallyScoped = false;
         emitPutToScope(varScope.m_scope, variableForLocalEntry(functionName, entry, varScope.m_symbolTableConstantIndex, isLexicallyScoped), currentValue.get(), DoNotThrowIfNotFound, InitializationMode::NotInitialization);
@@ -2012,13 +2005,12 @@ void BytecodeGenerator::popLexicalScopeInternal(VariableEnvironment& environment
     SymbolTableStackEntry stackEntry = m_symbolTableStack.takeLast();
     SymbolTable* symbolTable = stackEntry.m_symbolTable;
     bool hasCapturedVariables = false;
-    ConcurrentJITLocker locker(ConcurrentJITLocker::NoLockingNecessary);
     for (auto& entry : environment) {
         if (entry.value.isCaptured()) {
             hasCapturedVariables = true;
             continue;
         }
-        SymbolTableEntry symbolTableEntry = symbolTable->get(locker, entry.key.get());
+        SymbolTableEntry symbolTableEntry = symbolTable->get(NoLockingNecessary, entry.key.get());
         ASSERT(!symbolTableEntry.isNull());
         VarOffset offset = symbolTableEntry.varOffset();
         ASSERT(offset.isStack());
@@ -2066,8 +2058,7 @@ void BytecodeGenerator::prepareLexicalScopeForNextForLoopIteration(VariableEnvir
     {
         activationValuesToCopyOver.reserveInitialCapacity(symbolTable->scopeSize());
 
-        ConcurrentJITLocker locker(ConcurrentJITLocker::NoLockingNecessary);
-        for (auto end = symbolTable->end(locker), ptr = symbolTable->begin(locker); ptr != end; ++ptr) {
+        for (auto end = symbolTable->end(NoLockingNecessary), ptr = symbolTable->begin(NoLockingNecessary); ptr != end; ++ptr) {
             if (!ptr->value.varOffset().isScope())
                 continue;
 
@@ -2099,10 +2090,9 @@ void BytecodeGenerator::prepareLexicalScopeForNextForLoopIteration(VariableEnvir
     emitMove(scopeRegister(), loopScope);
 
     {
-        ConcurrentJITLocker locker(ConcurrentJITLocker::NoLockingNecessary);
         for (auto pair : activationValuesToCopyOver) {
             const Identifier& identifier = pair.second;
-            SymbolTableEntry entry = symbolTable->get(locker, identifier.impl());
+            SymbolTableEntry entry = symbolTable->get(NoLockingNecessary, identifier.impl());
             RELEASE_ASSERT(!entry.isNull());
             RegisterID* transitionValue = pair.first;
             emitPutToScope(loopScope, variableForLocalEntry(identifier, entry, loopSymbolTable->index(), true), transitionValue, DoNotThrowIfNotFound, InitializationMode::NotInitialization);
@@ -2137,13 +2127,12 @@ Variable BytecodeGenerator::variable(const Identifier& property, ThisResolutionT
     //         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);
         SymbolTable* symbolTable = stackEntry.m_symbolTable;
-        SymbolTableEntry symbolTableEntry = symbolTable->get(locker, property.impl());
+        SymbolTableEntry symbolTableEntry = symbolTable->get(NoLockingNecessary, property.impl());
         if (symbolTableEntry.isNull())
             continue;
         bool resultIsCallee = false;
@@ -2183,8 +2172,7 @@ void BytecodeGenerator::createVariable(
     const Identifier& property, VarKind varKind, SymbolTable* symbolTable, ExistingVariableMode existingVariableMode)
 {
     ASSERT(property != propertyNames().thisIdentifier);
-    ConcurrentJITLocker locker(ConcurrentJITLocker::NoLockingNecessary);
-    SymbolTableEntry entry = symbolTable->get(locker, property.impl());
+    SymbolTableEntry entry = symbolTable->get(NoLockingNecessary, property.impl());
     
     if (!entry.isNull()) {
         if (existingVariableMode == IgnoreExisting)
@@ -2208,13 +2196,13 @@ void BytecodeGenerator::createVariable(
     
     VarOffset varOffset;
     if (varKind == VarKind::Scope)
-        varOffset = VarOffset(symbolTable->takeNextScopeOffset(locker));
+        varOffset = VarOffset(symbolTable->takeNextScopeOffset(NoLockingNecessary));
     else {
         ASSERT(varKind == VarKind::Stack);
         varOffset = VarOffset(virtualRegisterForLocal(m_calleeLocals.size()));
     }
     SymbolTableEntry newEntry(varOffset, 0);
-    symbolTable->add(locker, property.impl(), newEntry);
+    symbolTable->add(NoLockingNecessary, property.impl(), newEntry);
     
     if (varKind == VarKind::Stack) {
         RegisterID* local = addVar();
@@ -2267,14 +2255,13 @@ RegisterID* BytecodeGenerator::emitResolveScope(RegisterID* dst, const Variable&
         // 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(locker, variable.ident().impl()).isNull())
+            if (stackEntry.m_symbolTable->get(NoLockingNecessary, variable.ident().impl()).isNull())
                 continue;
             
             RegisterID* scope = stackEntry.m_scope;
@@ -3784,8 +3771,7 @@ void BytecodeGenerator::emitPushFunctionNameScope(const Identifier& property, Re
     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();
-    ConcurrentJITLocker locker(ConcurrentJITLocker::NoLockingNecessary);
-    Variable functionVar = variableForLocalEntry(property, m_symbolTableStack.last().m_symbolTable->get(locker, property.impl()), m_symbolTableStack.last().m_symbolTableConstantIndex, shouldTreatAsLexicalVariable);
+    Variable functionVar = variableForLocalEntry(property, m_symbolTableStack.last().m_symbolTable->get(NoLockingNecessary, property.impl()), m_symbolTableStack.last().m_symbolTableConstantIndex, shouldTreatAsLexicalVariable);
     emitPutToScope(m_symbolTableStack.last().m_scope, functionVar, callee, ThrowIfNotFound, InitializationMode::NotInitialization);
 }
 
index 27e47f3..f39613e 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2013 Apple Inc. All rights reserved.
+ * Copyright (C) 2013, 2016 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -53,9 +53,8 @@ public:
     {
     }
 
-    enum NoLockingNecessaryTag { NoLockingNecessary };
     explicit ConcurrentJITLockerBase(NoLockingNecessaryTag)
-        : m_locker(ConcurrentJITLockerImpl::NoLockingNecessary)
+        : m_locker(NoLockingNecessary)
     {
     }
 
@@ -125,8 +124,8 @@ public:
     {
     }
 
-    ConcurrentJITLocker(ConcurrentJITLockerBase::NoLockingNecessaryTag)
-        : ConcurrentJITLockerBase(ConcurrentJITLockerBase::NoLockingNecessary)
+    ConcurrentJITLocker(NoLockingNecessaryTag)
+        : ConcurrentJITLockerBase(NoLockingNecessary)
 #if ENABLE(CONCURRENT_JIT) && !defined(NDEBUG)
         , m_disallowGC(Nullopt)
 #endif
index e96e975..501ffda 100644 (file)
@@ -1,3 +1,54 @@
+2016-06-02  Filip Pizlo  <fpizlo@apple.com>
+
+        Make it easier to use NoLockingNecessary
+        https://bugs.webkit.org/show_bug.cgi?id=158306
+
+        Reviewed by Keith Miller.
+        
+        An idiom that we borrowed from LLVM is that if a function requires a lock to be held, we
+        have it take a const Locker& as its first argument. This may not communicate which lock is
+        to be held, but it does help us to remember that some lock must be held. So far, it's been
+        relatively easy to then figure out which lock. We've had bugs where we forgot to hold a
+        lock but I don't remember the last time we had a bug where we grabbed the wrong lock.
+        
+        But sometimes, we know at the point where we call such a method that we actually don't
+        need to hold any lock. This usually happens during object construction. If we're
+        constructing some object then we usually know that we have not escaped it yet, so we don't
+        need to waste time acquiring its lock. We could solve this by having a separate set of
+        methods that don't do or require locking. This would be cumbersome, since usually for
+        every variant that takes const Locker&, there is already one that doesn't, and that one
+        will grab the lock for you. So this means having a third variant, that also doesn't take a
+        const Locker&, but does no locking. That's pretty weird.
+        
+        So, we introduced NoLockingNecessary for situations like this. The idiom went like so:
+        
+            Locker<Whatever> locker(Locker<Whatever>::NoLockingNecessary)
+            stuff->foo(locker);
+        
+        Usually though, there would be some distance between where the locker is defined and where
+        it's used, so when you just look at stuff->foo(locker) in isolation you don't know if this
+        is a real locker or a NoLockingNecessary cast. Also, requiring two lines for this just
+        adds code.
+        
+        This change makes this easier. Now you can just do:
+        
+            stuff->foo(NoLockingNecessary).
+        
+        This is because NoLockingNecessary has been pulled out into the WTF namespace (and is
+        usinged from the global namespace) and the Locker<> constructor that takes
+        NoLockingNecessaryTag is now implicit.
+        
+        The only possible downside of this change is that people might use this idiom more
+        frequently now that it's easier to use. I don't think that's a bad thing. I'm now
+        convinced that this is not a bad idiom. When I was fixing an unrelated bug, I almost went
+        the way of adding more locking to some core JSC data structures, and in the process, I
+        needed to use NoLockingNecessary. It's clear that this is a general-purpose idiom and we
+        should not impose artificial constraints on its use.
+
+        * wtf/Locker.h:
+        (WTF::Locker::Locker):
+        (WTF::Locker::~Locker):
+
 2016-06-01  Brady Eidson  <beidson@apple.com>
 
         Get rid of StringCapture.
index bfcc68b..0d8cbe0 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2008, 2013 Apple Inc. All rights reserved.
+ * Copyright (C) 2008, 2013, 2016 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
 
 namespace WTF {
 
+enum NoLockingNecessaryTag { NoLockingNecessary };
+
 template <typename T> class Locker {
     WTF_MAKE_NONCOPYABLE(Locker);
 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(NoLockingNecessaryTag) : m_lockable(nullptr) { }
 
     ~Locker()
     {
@@ -70,5 +71,7 @@ private:
 }
 
 using WTF::Locker;
+using WTF::NoLockingNecessaryTag;
+using WTF::NoLockingNecessary;
 
 #endif