⚠ Archived content — this site is no longer maintained.   Current WebKit documentation is at docs.webkit.org.

Changeset 201605 in webkit


Ignore:
Timestamp:
Jun 2, 2016, 11:41:16 AM (10 years ago)
Author:
fpizlo@apple.com
Message:

Make it easier to use NoLockingNecessary
​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):

Location:
trunk/Source
Files:
5 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/JavaScriptCore/ChangeLog

    r201590 r201605  
     12016-06-02  Filip Pizlo  <fpizlo@apple.com>
     2
     3        Make it easier to use NoLockingNecessary
     4        https://bugs.webkit.org/show_bug.cgi?id=158306
     5
     6        Reviewed by Keith Miller.
     7       
     8        Adapt to the new NoLockingNecessary API. More details in the WTF ChangeLog.
     9
     10        * bytecompiler/BytecodeGenerator.cpp:
     11        (JSC::BytecodeGenerator::BytecodeGenerator):
     12        (JSC::BytecodeGenerator::initializeArrowFunctionContextScopeIfNeeded):
     13        (JSC::BytecodeGenerator::instantiateLexicalVariables):
     14        (JSC::BytecodeGenerator::emitPrefillStackTDZVariables):
     15        (JSC::BytecodeGenerator::initializeBlockScopedFunctions):
     16        (JSC::BytecodeGenerator::hoistSloppyModeFunctionIfNecessary):
     17        (JSC::BytecodeGenerator::popLexicalScopeInternal):
     18        (JSC::BytecodeGenerator::prepareLexicalScopeForNextForLoopIteration):
     19        (JSC::BytecodeGenerator::variable):
     20        (JSC::BytecodeGenerator::createVariable):
     21        (JSC::BytecodeGenerator::emitResolveScope):
     22        (JSC::BytecodeGenerator::emitPushFunctionNameScope):
     23        * runtime/ConcurrentJITLock.h:
     24        (JSC::ConcurrentJITLockerBase::ConcurrentJITLockerBase):
     25        (JSC::ConcurrentJITLocker::ConcurrentJITLocker):
     26
    1272016-06-01  Filip Pizlo  <fpizlo@apple.com>
    228

  • trunk/Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp

    r201542 r201605  
    369369        // activation.
    370370       
    371         ConcurrentJITLocker locker(ConcurrentJITLocker::NoLockingNecessary);
    372371        if (capturesAnyArgumentByName) {
    373372            functionSymbolTable->setArgumentsLength(vm, parameters.size());
    … …  
    378377            // way we lift the value into the scope.
    379378            for (unsigned i = 0; i < parameters.size(); ++i) {
    380                 ScopeOffset offset = functionSymbolTable->takeNextScopeOffset(locker);
     379                ScopeOffset offset = functionSymbolTable->takeNextScopeOffset(NoLockingNecessary);
    381380                functionSymbolTable->setArgumentOffset(vm, i, offset);
    382381                if (UniquedStringImpl* name = visibleNameForParameter(parameters.at(i).first)) {
    … …  
    388387                    // So, we just disable it.
    389388                    entry.disableWatching();
    390                     functionSymbolTable->set(locker, name, entry);
     389                    functionSymbolTable->set(NoLockingNecessary, name, entry);
    391390                }
    392391                emitOpcode(op_put_to_scope);
    … …  
    409408            for (unsigned i = 0; i < parameters.size(); ++i) {
    410409                if (UniquedStringImpl* name = visibleNameForParameter(parameters.at(i).first))
    411                     functionSymbolTable->set(locker, name, SymbolTableEntry(VarOffset(DirectArgumentsOffset(i))));
     410                    functionSymbolTable->set(NoLockingNecessary, name, SymbolTableEntry(VarOffset(DirectArgumentsOffset(i))));
    412411            }
    413412           
    … …  
    420419        // because when default parameter expressions exist, they belong in their own lexical environment
    421420        // separate from the "var" lexical environment.
    422         ConcurrentJITLocker locker(ConcurrentJITLocker::NoLockingNecessary);
    423421        for (unsigned i = 0; i < parameters.size(); ++i) {
    424422            UniquedStringImpl* name = visibleNameForParameter(parameters.at(i).first);
    … …  
    429427                // This is the easy case - just tell the symbol table about the argument. It will
    430428                // be accessed directly.
    431                 functionSymbolTable->set(locker, name, SymbolTableEntry(VarOffset(virtualRegisterForArgument(1 + i))));
     429                functionSymbolTable->set(NoLockingNecessary, name, SymbolTableEntry(VarOffset(virtualRegisterForArgument(1 + i))));
    432430                continue;
    433431            }
    434432           
    435             ScopeOffset offset = functionSymbolTable->takeNextScopeOffset(locker);
     433            ScopeOffset offset = functionSymbolTable->takeNextScopeOffset(NoLockingNecessary);
    436434            const Identifier& ident =
    437435                static_cast<const BindingNode*>(parameters.at(i).first)->boundProperty();
    438             functionSymbolTable->set(locker, name, SymbolTableEntry(VarOffset(offset)));
     436            functionSymbolTable->set(NoLockingNecessary, name, SymbolTableEntry(VarOffset(offset)));
    439437           
    440438            emitOpcode(op_put_to_scope);
    … …  
    906904        ScopeOffset offset;
    907905       
    908         ConcurrentJITLocker locker(ConcurrentJITLocker::NoLockingNecessary);
    909906        if (isThisUsedInInnerArrowFunction()) {
    910             offset = functionSymbolTable->takeNextScopeOffset(locker);
    911             functionSymbolTable->set(locker, propertyNames().thisIdentifier.impl(), SymbolTableEntry(VarOffset(offset)));
     907            offset = functionSymbolTable->takeNextScopeOffset(NoLockingNecessary);
     908            functionSymbolTable->set(NoLockingNecessary, propertyNames().thisIdentifier.impl(), SymbolTableEntry(VarOffset(offset)));
    912909        }
    913910
    914911        if (m_codeType == FunctionCode && isNewTargetUsedInInnerArrowFunction()) {
    915912            offset = functionSymbolTable->takeNextScopeOffset();
    916             functionSymbolTable->set(locker, propertyNames().newTargetLocalPrivateName.impl(), SymbolTableEntry(VarOffset(offset)));
     913            functionSymbolTable->set(NoLockingNecessary, propertyNames().newTargetLocalPrivateName.impl(), SymbolTableEntry(VarOffset(offset)));
    917914        }
    918915       
    919916        if (isConstructor() && constructorKind() == ConstructorKind::Derived && isSuperUsedInInnerArrowFunction()) {
    920             offset = functionSymbolTable->takeNextScopeOffset(locker);
    921             functionSymbolTable->set(locker, propertyNames().derivedConstructorPrivateName.impl(), SymbolTableEntry(VarOffset(offset)));
     917            offset = functionSymbolTable->takeNextScopeOffset(NoLockingNecessary);
     918            functionSymbolTable->set(NoLockingNecessary, propertyNames().derivedConstructorPrivateName.impl(), SymbolTableEntry(VarOffset(offset)));
    922919        }
    923920
    … …  
    17651762    bool hasCapturedVariables = false;
    17661763    {
    1767         ConcurrentJITLocker locker(ConcurrentJITLocker::NoLockingNecessary);
    17681764        for (auto& entry : lexicalVariables) {
    17691765            ASSERT(entry.value.isLet() || entry.value.isConst() || entry.value.isFunction());
    17701766            ASSERT(!entry.value.isVar());
    1771             SymbolTableEntry symbolTableEntry = symbolTable->get(locker, entry.key.get());
     1767            SymbolTableEntry symbolTableEntry = symbolTable->get(NoLockingNecessary, entry.key.get());
    17721768            ASSERT(symbolTableEntry.isNull());
    17731769
    … …  
    17821778            VarOffset varOffset;
    17831779            if (varKind == VarKind::Scope) {
    1784                 varOffset = VarOffset(symbolTable->takeNextScopeOffset(locker));
     1780                varOffset = VarOffset(symbolTable->takeNextScopeOffset(NoLockingNecessary));
    17851781                hasCapturedVariables = true;
    17861782            } else {
    … …  
    17961792
    17971793            SymbolTableEntry newEntry(varOffset, entry.value.isConst() ? ReadOnly : 0);
    1798             symbolTable->add(locker, entry.key.get(), newEntry);
     1794            symbolTable->add(NoLockingNecessary, entry.key.get(), newEntry);
    17991795        }
    18001796    }
    … …  
    18061802    // Prefill stack variables with the TDZ empty value.
    18071803    // Scope variables will be initialized to the TDZ empty value when JSLexicalEnvironment is allocated.
    1808     ConcurrentJITLocker locker(ConcurrentJITLocker::NoLockingNecessary);
    18091804    for (auto& entry : lexicalVariables) {
    18101805        // Imported bindings which are not the namespace bindings are not allocated
    … …  
    18181813            continue;
    18191814
    1820         SymbolTableEntry symbolTableEntry = symbolTable->get(locker, entry.key.get());
     1815        SymbolTableEntry symbolTableEntry = symbolTable->get(NoLockingNecessary, entry.key.get());
    18211816        ASSERT(!symbolTableEntry.isNull());
    18221817        VarOffset offset = symbolTableEntry.varOffset();
    … …  
    19541949    RefPtr<RegisterID> temp = newTemporary();
    19551950    int symbolTableIndex = constantSymbolTable ? constantSymbolTable->index() : 0;
    1956     ConcurrentJITLocker locker(ConcurrentJITLocker::NoLockingNecessary);
    19571951    for (FunctionMetadataNode* function : functionStack) {
    19581952        const Identifier& name = function->ident();
    … …  
    19611955        RELEASE_ASSERT(iter->value.isFunction());
    19621956        // We purposefully don't hold the symbol table lock around this loop because emitNewFunctionExpressionCommon may GC.
    1963         SymbolTableEntry entry = symbolTable->get(locker, name.impl());
     1957        SymbolTableEntry entry = symbolTable->get(NoLockingNecessary, name.impl());
    19641958        RELEASE_ASSERT(!entry.isNull());
    19651959        emitNewFunctionExpressionCommon(temp.get(), function);
    … …  
    19861980        SymbolTable* varSymbolTable = varScope.m_symbolTable;
    19871981        ASSERT(varSymbolTable->scopeType() == SymbolTable::ScopeType::VarScope);
    1988         ConcurrentJITLocker locker(ConcurrentJITLocker::NoLockingNecessary);
    1989         SymbolTableEntry entry = varSymbolTable->get(locker, functionName.impl());
     1982        SymbolTableEntry entry = varSymbolTable->get(NoLockingNecessary, functionName.impl());
    19901983        ASSERT(!entry.isNull());
    19911984        bool isLexicallyScoped = false;
    … …  
    20132006    SymbolTable* symbolTable = stackEntry.m_symbolTable;
    20142007    bool hasCapturedVariables = false;
    2015     ConcurrentJITLocker locker(ConcurrentJITLocker::NoLockingNecessary);
    20162008    for (auto& entry : environment) {
    20172009        if (entry.value.isCaptured()) {
    … …  
    20192011            continue;
    20202012        }
    2021         SymbolTableEntry symbolTableEntry = symbolTable->get(locker, entry.key.get());
     2013        SymbolTableEntry symbolTableEntry = symbolTable->get(NoLockingNecessary, entry.key.get());
    20222014        ASSERT(!symbolTableEntry.isNull());
    20232015        VarOffset offset = symbolTableEntry.varOffset();
    … …  
    20672059        activationValuesToCopyOver.reserveInitialCapacity(symbolTable->scopeSize());
    20682060
    2069         ConcurrentJITLocker locker(ConcurrentJITLocker::NoLockingNecessary);
    2070         for (auto end = symbolTable->end(locker), ptr = symbolTable->begin(locker); ptr != end; ++ptr) {
     2061        for (auto end = symbolTable->end(NoLockingNecessary), ptr = symbolTable->begin(NoLockingNecessary); ptr != end; ++ptr) {
    20712062            if (!ptr->value.varOffset().isScope())
    20722063                continue;
    … …  
    21002091
    21012092    {
    2102         ConcurrentJITLocker locker(ConcurrentJITLocker::NoLockingNecessary);
    21032093        for (auto pair : activationValuesToCopyOver) {
    21042094            const Identifier& identifier = pair.second;
    2105             SymbolTableEntry entry = symbolTable->get(locker, identifier.impl());
     2095            SymbolTableEntry entry = symbolTable->get(NoLockingNecessary, identifier.impl());
    21062096            RELEASE_ASSERT(!entry.isNull());
    21072097            RegisterID* transitionValue = pair.first;
    … …  
    21382128    //     }
    21392129    // }
    2140     ConcurrentJITLocker locker(ConcurrentJITLocker::NoLockingNecessary);
    21412130    for (unsigned i = m_symbolTableStack.size(); i--; ) {
    21422131        SymbolTableStackEntry& stackEntry = m_symbolTableStack[i];
    … …  
    21442133            return Variable(property);
    21452134        SymbolTable* symbolTable = stackEntry.m_symbolTable;
    2146         SymbolTableEntry symbolTableEntry = symbolTable->get(locker, property.impl());
     2135        SymbolTableEntry symbolTableEntry = symbolTable->get(NoLockingNecessary, property.impl());
    21472136        if (symbolTableEntry.isNull())
    21482137            continue;
    … …  
    21842173{
    21852174    ASSERT(property != propertyNames().thisIdentifier);
    2186     ConcurrentJITLocker locker(ConcurrentJITLocker::NoLockingNecessary);
    2187     SymbolTableEntry entry = symbolTable->get(locker, property.impl());
     2175    SymbolTableEntry entry = symbolTable->get(NoLockingNecessary, property.impl());
    21882176   
    21892177    if (!entry.isNull()) {
    … …  
    22092197    VarOffset varOffset;
    22102198    if (varKind == VarKind::Scope)
    2211         varOffset = VarOffset(symbolTable->takeNextScopeOffset(locker));
     2199        varOffset = VarOffset(symbolTable->takeNextScopeOffset(NoLockingNecessary));
    22122200    else {
    22132201        ASSERT(varKind == VarKind::Stack);
    … …  
    22152203    }
    22162204    SymbolTableEntry newEntry(varOffset, 0);
    2217     symbolTable->add(locker, property.impl(), newEntry);
     2205    symbolTable->add(NoLockingNecessary, property.impl(), newEntry);
    22182206   
    22192207    if (varKind == VarKind::Stack) {
    … …  
    22682256        // requires weird things because it is a shameful pile of nonsense, but block scoping would make
    22692257        // that code sensible and obviate the need for us to do bad things.
    2270         ConcurrentJITLocker locker(ConcurrentJITLocker::NoLockingNecessary);
    22712258        for (unsigned i = m_symbolTableStack.size(); i--; ) {
    22722259            SymbolTableStackEntry& stackEntry = m_symbolTableStack[i];
    … …  
    22752262            RELEASE_ASSERT(!stackEntry.m_isWithScope);
    22762263
    2277             if (stackEntry.m_symbolTable->get(locker, variable.ident().impl()).isNull())
     2264            if (stackEntry.m_symbolTable->get(NoLockingNecessary, variable.ident().impl()).isNull())
    22782265                continue;
    22792266           
    … …  
    37853772    ASSERT_UNUSED(numVars, m_codeBlock->m_numVars == static_cast<int>(numVars + 1)); // Should have only created one new "var" for the function name scope.
    37863773    bool shouldTreatAsLexicalVariable = isStrictMode();
    3787     ConcurrentJITLocker locker(ConcurrentJITLocker::NoLockingNecessary);
    3788     Variable functionVar = variableForLocalEntry(property, m_symbolTableStack.last().m_symbolTable->get(locker, property.impl()), m_symbolTableStack.last().m_symbolTableConstantIndex, shouldTreatAsLexicalVariable);
     3774    Variable functionVar = variableForLocalEntry(property, m_symbolTableStack.last().m_symbolTable->get(NoLockingNecessary, property.impl()), m_symbolTableStack.last().m_symbolTableConstantIndex, shouldTreatAsLexicalVariable);
    37893775    emitPutToScope(m_symbolTableStack.last().m_scope, functionVar, callee, ThrowIfNotFound, InitializationMode::NotInitialization);
    37903776}

  • trunk/Source/JavaScriptCore/runtime/ConcurrentJITLock.h

    r199848 r201605  
    11/*
    2  * Copyright (C) 2013 Apple Inc. All rights reserved.
     2 * Copyright (C) 2013, 2016 Apple Inc. All rights reserved.
    33 *
    44 * Redistribution and use in source and binary forms, with or without
    … …  
    5454    }
    5555
    56     enum NoLockingNecessaryTag { NoLockingNecessary };
    5756    explicit ConcurrentJITLockerBase(NoLockingNecessaryTag)
    58         : m_locker(ConcurrentJITLockerImpl::NoLockingNecessary)
     57        : m_locker(NoLockingNecessary)
    5958    {
    6059    }
    … …  
    126125    }
    127126
    128     ConcurrentJITLocker(ConcurrentJITLockerBase::NoLockingNecessaryTag)
    129         : ConcurrentJITLockerBase(ConcurrentJITLockerBase::NoLockingNecessary)
     127    ConcurrentJITLocker(NoLockingNecessaryTag)
     128        : ConcurrentJITLockerBase(NoLockingNecessary)
    130129#if ENABLE(CONCURRENT_JIT) && !defined(NDEBUG)
    131130        , m_disallowGC(Nullopt)

  • trunk/Source/WTF/ChangeLog

    r201594 r201605  
     12016-06-02  Filip Pizlo  <fpizlo@apple.com>
     2
     3        Make it easier to use NoLockingNecessary
     4        https://bugs.webkit.org/show_bug.cgi?id=158306
     5
     6        Reviewed by Keith Miller.
     7       
     8        An idiom that we borrowed from LLVM is that if a function requires a lock to be held, we
     9        have it take a const Locker& as its first argument. This may not communicate which lock is
     10        to be held, but it does help us to remember that some lock must be held. So far, it's been
     11        relatively easy to then figure out which lock. We've had bugs where we forgot to hold a
     12        lock but I don't remember the last time we had a bug where we grabbed the wrong lock.
     13       
     14        But sometimes, we know at the point where we call such a method that we actually don't
     15        need to hold any lock. This usually happens during object construction. If we're
     16        constructing some object then we usually know that we have not escaped it yet, so we don't
     17        need to waste time acquiring its lock. We could solve this by having a separate set of
     18        methods that don't do or require locking. This would be cumbersome, since usually for
     19        every variant that takes const Locker&, there is already one that doesn't, and that one
     20        will grab the lock for you. So this means having a third variant, that also doesn't take a
     21        const Locker&, but does no locking. That's pretty weird.
     22       
     23        So, we introduced NoLockingNecessary for situations like this. The idiom went like so:
     24       
     25            Locker<Whatever> locker(Locker<Whatever>::NoLockingNecessary)
     26            stuff->foo(locker);
     27       
     28        Usually though, there would be some distance between where the locker is defined and where
     29        it's used, so when you just look at stuff->foo(locker) in isolation you don't know if this
     30        is a real locker or a NoLockingNecessary cast. Also, requiring two lines for this just
     31        adds code.
     32       
     33        This change makes this easier. Now you can just do:
     34       
     35            stuff->foo(NoLockingNecessary).
     36       
     37        This is because NoLockingNecessary has been pulled out into the WTF namespace (and is
     38        usinged from the global namespace) and the Locker<> constructor that takes
     39        NoLockingNecessaryTag is now implicit.
     40       
     41        The only possible downside of this change is that people might use this idiom more
     42        frequently now that it's easier to use. I don't think that's a bad thing. I'm now
     43        convinced that this is not a bad idiom. When I was fixing an unrelated bug, I almost went
     44        the way of adding more locking to some core JSC data structures, and in the process, I
     45        needed to use NoLockingNecessary. It's clear that this is a general-purpose idiom and we
     46        should not impose artificial constraints on its use.
     47
     48        * wtf/Locker.h:
     49        (WTF::Locker::Locker):
     50        (WTF::Locker::~Locker):
     51
    1522016-06-01  Brady Eidson  <beidson@apple.com>
    253

  • trunk/Source/WTF/wtf/Locker.h

    r199848 r201605  
    11/*
    2  * Copyright (C) 2008, 2013 Apple Inc. All rights reserved.
     2 * Copyright (C) 2008, 2013, 2016 Apple Inc. All rights reserved.
    33 *
    44 * Redistribution and use in source and binary forms, with or without
    … …  
    3333namespace WTF {
    3434
     35enum NoLockingNecessaryTag { NoLockingNecessary };
     36
    3537template <typename T> class Locker {
    3638    WTF_MAKE_NONCOPYABLE(Locker);
    … …  
    3941    explicit Locker(T* lockable) : m_lockable(lockable) { lock(); }
    4042
    41     enum NoLockingNecessaryTag { NoLockingNecessary };
    4243    // You should be wary of using this constructor. It's only applicable
    4344    // in places where there is a locking protocol for a particular object
    … …  
    4546    // this often happens when an object is newly allocated and it can not
    4647    // be accessed concurrently.
    47     explicit Locker(NoLockingNecessaryTag) : m_lockable(nullptr) { }
     48    Locker(NoLockingNecessaryTag) : m_lockable(nullptr) { }
    4849
    4950    ~Locker()
    … …  
    7172
    7273using WTF::Locker;
     74using WTF::NoLockingNecessaryTag;
     75using WTF::NoLockingNecessary;
    7376
    7477#endif
Note: See TracChangeset for help on using the changeset viewer.