Canonicalize aquiring the JSCell lock.
authorkeith_miller@apple.com <keith_miller@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 31 Jan 2018 18:57:13 +0000 (18:57 +0000)
committerkeith_miller@apple.com <keith_miller@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 31 Jan 2018 18:57:13 +0000 (18:57 +0000)
https://bugs.webkit.org/show_bug.cgi?id=182320

Reviewed by Michael Saboff.

It's currently kinda annoying to figure out where
we aquire the a JSCell's lock. This patch adds a
helper to make it easier to grep...

* bytecode/UnlinkedCodeBlock.cpp:
(JSC::UnlinkedCodeBlock::visitChildren):
(JSC::UnlinkedCodeBlock::setInstructions):
(JSC::UnlinkedCodeBlock::shrinkToFit):
* runtime/ErrorInstance.cpp:
(JSC::ErrorInstance::finishCreation):
(JSC::ErrorInstance::materializeErrorInfoIfNeeded):
(JSC::ErrorInstance::visitChildren):
* runtime/JSArray.cpp:
(JSC::JSArray::shiftCountWithArrayStorage):
(JSC::JSArray::unshiftCountWithArrayStorage):
* runtime/JSCell.h:
(JSC::JSCell::cellLock):
* runtime/JSObject.cpp:
(JSC::JSObject::visitButterflyImpl):
(JSC::JSObject::convertContiguousToArrayStorage):
* runtime/JSPropertyNameEnumerator.cpp:
(JSC::JSPropertyNameEnumerator::visitChildren):
* runtime/SparseArrayValueMap.cpp:
(JSC::SparseArrayValueMap::add):
(JSC::SparseArrayValueMap::remove):
(JSC::SparseArrayValueMap::visitChildren):

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp
Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h
Source/JavaScriptCore/runtime/ErrorInstance.cpp
Source/JavaScriptCore/runtime/JSArray.cpp
Source/JavaScriptCore/runtime/JSCell.cpp
Source/JavaScriptCore/runtime/JSCell.h
Source/JavaScriptCore/runtime/JSCellInlines.h
Source/JavaScriptCore/runtime/JSObject.cpp
Source/JavaScriptCore/runtime/JSPropertyNameEnumerator.cpp
Source/JavaScriptCore/runtime/SparseArrayValueMap.cpp

index 28c1743..a45cb48 100644 (file)
@@ -1,3 +1,37 @@
+2018-01-31  Keith Miller  <keith_miller@apple.com>
+
+        Canonicalize aquiring the JSCell lock.
+        https://bugs.webkit.org/show_bug.cgi?id=182320
+
+        Reviewed by Michael Saboff.
+
+        It's currently kinda annoying to figure out where
+        we aquire the a JSCell's lock. This patch adds a
+        helper to make it easier to grep...
+
+        * bytecode/UnlinkedCodeBlock.cpp:
+        (JSC::UnlinkedCodeBlock::visitChildren):
+        (JSC::UnlinkedCodeBlock::setInstructions):
+        (JSC::UnlinkedCodeBlock::shrinkToFit):
+        * runtime/ErrorInstance.cpp:
+        (JSC::ErrorInstance::finishCreation):
+        (JSC::ErrorInstance::materializeErrorInfoIfNeeded):
+        (JSC::ErrorInstance::visitChildren):
+        * runtime/JSArray.cpp:
+        (JSC::JSArray::shiftCountWithArrayStorage):
+        (JSC::JSArray::unshiftCountWithArrayStorage):
+        * runtime/JSCell.h:
+        (JSC::JSCell::cellLock):
+        * runtime/JSObject.cpp:
+        (JSC::JSObject::visitButterflyImpl):
+        (JSC::JSObject::convertContiguousToArrayStorage):
+        * runtime/JSPropertyNameEnumerator.cpp:
+        (JSC::JSPropertyNameEnumerator::visitChildren):
+        * runtime/SparseArrayValueMap.cpp:
+        (JSC::SparseArrayValueMap::add):
+        (JSC::SparseArrayValueMap::remove):
+        (JSC::SparseArrayValueMap::visitChildren):
+
 2018-01-31  Saam Barati  <sbarati@apple.com>
 
         JSC incorrectly interpreting script, sets Global Property instead of Global Lexical variable (LiteralParser / JSONP path)
index 71f1ca5..cefe3ce 100644 (file)
@@ -94,7 +94,7 @@ void UnlinkedCodeBlock::visitChildren(JSCell* cell, SlotVisitor& visitor)
     UnlinkedCodeBlock* thisObject = jsCast<UnlinkedCodeBlock*>(cell);
     ASSERT_GC_OBJECT_INHERITS(thisObject, info());
     Base::visitChildren(thisObject, visitor);
-    auto locker = holdLock(*thisObject);
+    auto locker = holdLock(thisObject->cellLock());
     for (FunctionExpressionVector::iterator ptr = thisObject->m_functionDecls.begin(), end = thisObject->m_functionDecls.end(); ptr != end; ++ptr)
         visitor.append(*ptr);
     for (FunctionExpressionVector::iterator ptr = thisObject->m_functionExprs.begin(), end = thisObject->m_functionExprs.end(); ptr != end; ++ptr)
@@ -317,7 +317,7 @@ void UnlinkedCodeBlock::setInstructions(std::unique_ptr<UnlinkedInstructionStrea
 {
     ASSERT(instructions);
     {
-        auto locker = holdLock(*this);
+        auto locker = holdLock(cellLock());
         m_unlinkedInstructions = WTFMove(instructions);
     }
     Heap::heap(this)->reportExtraMemoryAllocated(m_unlinkedInstructions->sizeInBytes());
@@ -392,7 +392,7 @@ void UnlinkedCodeBlock::applyModification(BytecodeRewriter& rewriter, UnpackedIn
 
 void UnlinkedCodeBlock::shrinkToFit()
 {
-    auto locker = holdLock(*this);
+    auto locker = holdLock(cellLock());
     
     m_jumpTargets.shrinkToFit();
     m_identifiers.shrinkToFit();
index b24dfdc..c7591e1 100644 (file)
@@ -160,7 +160,7 @@ public:
     {
         createRareDataIfNecessary();
         VM& vm = *this->vm();
-        auto locker = lockDuringMarking(vm.heap, *this);
+        auto locker = lockDuringMarking(vm.heap, cellLock());
         unsigned size = m_rareData->m_regexps.size();
         m_rareData->m_regexps.append(WriteBarrier<RegExp>(vm, this, r));
         return size;
@@ -191,7 +191,7 @@ public:
     void addSetConstant(IdentifierSet& set)
     {
         VM& vm = *this->vm();
-        auto locker = lockDuringMarking(vm.heap, *this);
+        auto locker = lockDuringMarking(vm.heap, cellLock());
         unsigned result = m_constantRegisters.size();
         m_constantRegisters.append(WriteBarrier<Unknown>());
         m_constantsSourceCodeRepresentation.append(SourceCodeRepresentation::Other);
@@ -201,7 +201,7 @@ public:
     unsigned addConstant(JSValue v, SourceCodeRepresentation sourceCodeRepresentation = SourceCodeRepresentation::Other)
     {
         VM& vm = *this->vm();
-        auto locker = lockDuringMarking(vm.heap, *this);
+        auto locker = lockDuringMarking(vm.heap, cellLock());
         unsigned result = m_constantRegisters.size();
         m_constantRegisters.append(WriteBarrier<Unknown>());
         m_constantRegisters.last().set(vm, this, v);
@@ -211,7 +211,7 @@ public:
     unsigned addConstant(LinkTimeConstant type)
     {
         VM& vm = *this->vm();
-        auto locker = lockDuringMarking(vm.heap, *this);
+        auto locker = lockDuringMarking(vm.heap, cellLock());
         unsigned result = m_constantRegisters.size();
         ASSERT(result);
         unsigned index = static_cast<unsigned>(type);
@@ -274,7 +274,7 @@ public:
     unsigned addFunctionDecl(UnlinkedFunctionExecutable* n)
     {
         VM& vm = *this->vm();
-        auto locker = lockDuringMarking(vm.heap, *this);
+        auto locker = lockDuringMarking(vm.heap, cellLock());
         unsigned size = m_functionDecls.size();
         m_functionDecls.append(WriteBarrier<UnlinkedFunctionExecutable>());
         m_functionDecls.last().set(vm, this, n);
@@ -285,7 +285,7 @@ public:
     unsigned addFunctionExpr(UnlinkedFunctionExecutable* n)
     {
         VM& vm = *this->vm();
-        auto locker = lockDuringMarking(vm.heap, *this);
+        auto locker = lockDuringMarking(vm.heap, cellLock());
         unsigned size = m_functionExprs.size();
         m_functionExprs.append(WriteBarrier<UnlinkedFunctionExecutable>());
         m_functionExprs.last().set(vm, this, n);
@@ -415,7 +415,7 @@ private:
     void createRareDataIfNecessary()
     {
         if (!m_rareData) {
-            auto locker = lockDuringMarking(*heap(), *this);
+            auto locker = lockDuringMarking(*heap(), cellLock());
             m_rareData = std::make_unique<RareData>();
         }
     }
index 40c6258..c172428 100644 (file)
@@ -117,7 +117,7 @@ void ErrorInstance::finishCreation(ExecState* exec, VM& vm, const String& messag
 
     std::unique_ptr<Vector<StackFrame>> stackTrace = getStackTrace(exec, vm, this, useCurrentFrame);
     {
-        auto locker = holdLock(*this);
+        auto locker = holdLock(cellLock());
         m_stackTrace = WTFMove(stackTrace);
     }
     vm.heap.writeBarrier(this);
@@ -209,7 +209,7 @@ bool ErrorInstance::materializeErrorInfoIfNeeded(VM& vm)
     
     addErrorInfo(vm, m_stackTrace.get(), this);
     {
-        auto locker = holdLock(*this);
+        auto locker = holdLock(cellLock());
         m_stackTrace = nullptr;
     }
     
@@ -234,7 +234,7 @@ void ErrorInstance::visitChildren(JSCell* cell, SlotVisitor& visitor)
     Base::visitChildren(thisObject, visitor);
 
     {
-        auto locker = holdLock(*thisObject);
+        auto locker = holdLock(thisObject->cellLock());
         if (thisObject->m_stackTrace) {
             for (StackFrame& frame : *thisObject->m_stackTrace)
                 frame.visitChildren(visitor);
index 884b0a2..fd6770d 100644 (file)
@@ -802,7 +802,7 @@ bool JSArray::shiftCountWithArrayStorage(VM& vm, unsigned startIndex, unsigned c
         return true;
     
     DisallowGC disallowGC;
-    auto locker = holdLock(*this);
+    auto locker = holdLock(cellLock());
     
     if (startIndex + count > vectorLength)
         count = vectorLength - startIndex;
@@ -1005,7 +1005,7 @@ bool JSArray::unshiftCountWithArrayStorage(ExecState* exec, unsigned startIndex,
     // Need to have GC deferred around the unshiftCountSlowCase(), since that leaves the butterfly in
     // a weird state: some parts of it will be left uninitialized, which we will fill in here.
     DeferGC deferGC(vm.heap);
-    auto locker = holdLock(*this);
+    auto locker = holdLock(cellLock());
     
     if (moveFront && storage->m_indexBias >= count) {
         Butterfly* newButterfly = storage->butterfly()->unshift(structure(), count);
index 24a74be..23c6d41 100644 (file)
@@ -308,13 +308,13 @@ JSValue JSCell::getPrototype(JSObject*, ExecState*)
     RELEASE_ASSERT_NOT_REACHED();
 }
 
-void JSCell::lockSlow()
+void JSCellLock::lockSlow()
 {
     Atomic<IndexingType>* lock = bitwise_cast<Atomic<IndexingType>*>(&m_indexingTypeAndMisc);
     IndexingTypeLockAlgorithm::lockSlow(*lock);
 }
 
-void JSCell::unlockSlow()
+void JSCellLock::unlockSlow()
 {
     Atomic<IndexingType>* lock = bitwise_cast<Atomic<IndexingType>*>(&m_indexingTypeAndMisc);
     IndexingTypeLockAlgorithm::unlockSlow(*lock);
index 4da77ab..64c84ad 100644 (file)
@@ -50,6 +50,7 @@ class PropertyDescriptor;
 class PropertyName;
 class PropertyNameArray;
 class Structure;
+class JSCellLock;
 
 enum class GCDeferralContextArgPresense {
     HasArg,
@@ -120,10 +121,11 @@ public:
     // Each cell has a built-in lock. Currently it's simply available for use if you need it. It's
     // a full-blown WTF::Lock. Note that this lock is currently used in JSArray and that lock's
     // ordering with the Structure lock is that the Structure lock must be acquired first.
-    void lock();
-    bool tryLock();
-    void unlock();
-    bool isLocked() const;
+
+    // We use this abstraction to make it easier to grep for places where we lock cells.
+    // to lock a cell you can just do:
+    // auto locker = holdLock(cell->cellLocker());
+    JSCellLock& cellLock() { return *reinterpret_cast<JSCellLock*>(this); }
     
     JSType type() const;
     IndexingType indexingTypeAndMisc() const;
@@ -272,10 +274,9 @@ protected:
 
 private:
     friend class LLIntOffsetsExtractor;
+    friend class JSCellLock;
 
     JS_EXPORT_PRIVATE JSObject* toObjectSlow(ExecState*, JSGlobalObject*) const;
-    JS_EXPORT_PRIVATE void lockSlow();
-    JS_EXPORT_PRIVATE void unlockSlow();
 
     StructureID m_structureID;
     IndexingType m_indexingTypeAndMisc; // DO NOT store to this field. Always CAS.
@@ -284,6 +285,17 @@ private:
     CellState m_cellState;
 };
 
+class JSCellLock : public JSCell {
+public:
+    void lock();
+    bool tryLock();
+    void unlock();
+    bool isLocked() const;
+private:
+    JS_EXPORT_PRIVATE void lockSlow();
+    JS_EXPORT_PRIVATE void unlockSlow();
+};
+
 template<typename To, typename From>
 inline To jsCast(From* from)
 {
index 00b3940..836d851 100644 (file)
@@ -323,27 +323,27 @@ inline void JSCell::callDestructor(VM& vm)
     zap();
 }
 
-inline void JSCell::lock()
+inline void JSCellLock::lock()
 {
     Atomic<IndexingType>* lock = bitwise_cast<Atomic<IndexingType>*>(&m_indexingTypeAndMisc);
     if (UNLIKELY(!IndexingTypeLockAlgorithm::lockFast(*lock)))
         lockSlow();
 }
 
-inline bool JSCell::tryLock()
+inline bool JSCellLock::tryLock()
 {
     Atomic<IndexingType>* lock = bitwise_cast<Atomic<IndexingType>*>(&m_indexingTypeAndMisc);
     return IndexingTypeLockAlgorithm::tryLock(*lock);
 }
 
-inline void JSCell::unlock()
+inline void JSCellLock::unlock()
 {
     Atomic<IndexingType>* lock = bitwise_cast<Atomic<IndexingType>*>(&m_indexingTypeAndMisc);
     if (UNLIKELY(!IndexingTypeLockAlgorithm::unlockFast(*lock)))
         unlockSlow();
 }
 
-inline bool JSCell::isLocked() const
+inline bool JSCellLock::isLocked() const
 {
     Atomic<IndexingType>* lock = bitwise_cast<Atomic<IndexingType>*>(&m_indexingTypeAndMisc);
     return IndexingTypeLockAlgorithm::isLocked(*lock);
index 5a4e7f9..078f50f 100644 (file)
@@ -383,7 +383,7 @@ ALWAYS_INLINE Structure* JSObject::visitButterflyImpl(SlotVisitor& visitor)
     lastOffset = structure->lastOffset();
     IndexingType indexingType = structure->indexingType();
     Dependency indexingTypeDependency = Dependency::fence(indexingType);
-    Locker<JSCell> locker(NoLockingNecessary);
+    Locker<JSCellLock> locker(NoLockingNecessary);
     switch (indexingType) {
     case ALL_CONTIGUOUS_INDEXING_TYPES:
     case ALL_ARRAY_STORAGE_INDEXING_TYPES:
@@ -391,7 +391,7 @@ ALWAYS_INLINE Structure* JSObject::visitButterflyImpl(SlotVisitor& visitor)
         // that can happen when the butterfly is used for array storage. We conservatively
         // assume that a contiguous butterfly may transform into an array storage one, though
         // this is probably more conservative than necessary.
-        locker = Locker<JSCell>(*this);
+        locker = holdLock(cellLock());
         break;
     default:
         break;
@@ -1358,7 +1358,7 @@ ArrayStorage* JSObject::convertContiguousToArrayStorage(VM& vm, NonPropertyTrans
     // Fortunately, we have the JSCell lock for this purpose!
     
     if (vm.heap.mutatorShouldBeFenced()) {
-        auto locker = holdLock(*this);
+        auto locker = holdLock(cellLock());
         m_butterflyIndexingMask = newStorage->butterfly()->computeIndexingMask();
         setStructureIDDirectly(nuke(structureID()));
         WTF::storeStoreFence();
index 82bb0dc..c366470 100644 (file)
@@ -71,7 +71,7 @@ void JSPropertyNameEnumerator::finishCreation(VM& vm, uint32_t indexedLength, ui
     m_endGenericPropertyIndex = vector.size();
 
     {
-        auto locker = lockDuringMarking(vm.heap, *this);
+        auto locker = lockDuringMarking(vm.heap, cellLock());
         m_propertyNames.resizeToFit(vector.size());
     }
     for (unsigned i = 0; i < vector.size(); ++i) {
@@ -89,7 +89,7 @@ void JSPropertyNameEnumerator::visitChildren(JSCell* cell, SlotVisitor& visitor)
 {
     Base::visitChildren(cell, visitor);
     JSPropertyNameEnumerator* thisObject = jsCast<JSPropertyNameEnumerator*>(cell);
-    auto locker = holdLock(*thisObject);
+    auto locker = holdLock(thisObject->cellLock());
     for (auto& propertyName : thisObject->m_propertyNames)
         visitor.append(propertyName);
     visitor.append(thisObject->m_prototypeChain);
index 247c07c..e992988 100644 (file)
@@ -77,7 +77,7 @@ SparseArrayValueMap::AddResult SparseArrayValueMap::add(JSObject* array, unsigne
     AddResult result;
     size_t capacity;
     {
-        auto locker = holdLock(*this);
+        auto locker = holdLock(cellLock());
         SparseArrayEntry entry;
         entry.setWithoutWriteBarrier(jsUndefined());
         
@@ -95,13 +95,13 @@ SparseArrayValueMap::AddResult SparseArrayValueMap::add(JSObject* array, unsigne
 
 void SparseArrayValueMap::remove(iterator it)
 {
-    auto locker = holdLock(*this);
+    auto locker = holdLock(cellLock());
     m_map.remove(it);
 }
 
 void SparseArrayValueMap::remove(unsigned i)
 {
-    auto locker = holdLock(*this);
+    auto locker = holdLock(cellLock());
     m_map.remove(i);
 }
 
@@ -197,9 +197,9 @@ JSValue SparseArrayEntry::getNonSparseMode() const
 void SparseArrayValueMap::visitChildren(JSCell* thisObject, SlotVisitor& visitor)
 {
     Base::visitChildren(thisObject, visitor);
-    
+
+    auto locker = holdLock(thisObject->cellLock());
     SparseArrayValueMap* thisMap = jsCast<SparseArrayValueMap*>(thisObject);
-    auto locker = holdLock(*thisMap);
     iterator end = thisMap->m_map.end();
     for (iterator it = thisMap->m_map.begin(); it != end; ++it)
         visitor.append(it->value);