fourthTier: Create an equivalent of Structure::get() that can work from a compilation thread
https://bugs.webkit.org/show_bug.cgi?id=114987
Reviewed by Geoffrey Garen.
This completes the work started by r148570. That patch made it possible to do
Structure::get() without modifying Structure. This patch takes this further, and
makes this thread-safe (for non-uncacheable-dictionaries) via
Structure::getConcurrently(). This method not only doesn't modify Structure, but
also ensures that any concurrent attempts to add to, remove from, or steal the
table from that structure doesn't mess up the result of the call. The call may
return invalidOffset even if a property is *just* about to be added, but it will
never do the reverse: if it returns a property then you can be sure that the
structure really does have that property and always will have it.
* bytecode/GetByIdStatus.cpp:
(JSC::GetByIdStatus::computeFromLLInt):
(JSC::GetByIdStatus::computeForChain):
(JSC::GetByIdStatus::computeFor):
* bytecode/PutByIdStatus.cpp:
(JSC::PutByIdStatus::computeFromLLInt):
(JSC::PutByIdStatus::computeFor):
* dfg/DFGFixupPhase.cpp:
(JSC::DFG::FixupPhase::isStringPrototypeMethodSane):
* runtime/PropertyMapHashTable.h:
(PropertyTable):
(JSC::PropertyTable::findConcurrently):
(JSC):
(JSC::PropertyTable::add):
(JSC::PropertyTable::remove):
(JSC::PropertyTable::reinsert):
(JSC::PropertyTable::rehash):
* runtime/PropertyTable.cpp:
(JSC::PropertyTable::PropertyTable):
* runtime/Structure.cpp:
(JSC::Structure::findStructuresAndMapForMaterialization):
(JSC::Structure::getConcurrently):
* runtime/Structure.h:
(Structure):
* runtime/StructureInlines.h:
(JSC::Structure::getConcurrently):
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@153128 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
index 54b5735..55e1796 100644
--- a/Source/JavaScriptCore/ChangeLog
+++ b/Source/JavaScriptCore/ChangeLog
@@ -1,5 +1,53 @@
2013-07-16 Oliver Hunt <oliver@apple.com>
+ Merge dfgFourthTier r148936
+
+ 2013-04-22 Filip Pizlo <fpizlo@apple.com>
+
+ fourthTier: Create an equivalent of Structure::get() that can work from a compilation thread
+ https://bugs.webkit.org/show_bug.cgi?id=114987
+
+ Reviewed by Geoffrey Garen.
+
+ This completes the work started by r148570. That patch made it possible to do
+ Structure::get() without modifying Structure. This patch takes this further, and
+ makes this thread-safe (for non-uncacheable-dictionaries) via
+ Structure::getConcurrently(). This method not only doesn't modify Structure, but
+ also ensures that any concurrent attempts to add to, remove from, or steal the
+ table from that structure doesn't mess up the result of the call. The call may
+ return invalidOffset even if a property is *just* about to be added, but it will
+ never do the reverse: if it returns a property then you can be sure that the
+ structure really does have that property and always will have it.
+
+ * bytecode/GetByIdStatus.cpp:
+ (JSC::GetByIdStatus::computeFromLLInt):
+ (JSC::GetByIdStatus::computeForChain):
+ (JSC::GetByIdStatus::computeFor):
+ * bytecode/PutByIdStatus.cpp:
+ (JSC::PutByIdStatus::computeFromLLInt):
+ (JSC::PutByIdStatus::computeFor):
+ * dfg/DFGFixupPhase.cpp:
+ (JSC::DFG::FixupPhase::isStringPrototypeMethodSane):
+ * runtime/PropertyMapHashTable.h:
+ (PropertyTable):
+ (JSC::PropertyTable::findConcurrently):
+ (JSC):
+ (JSC::PropertyTable::add):
+ (JSC::PropertyTable::remove):
+ (JSC::PropertyTable::reinsert):
+ (JSC::PropertyTable::rehash):
+ * runtime/PropertyTable.cpp:
+ (JSC::PropertyTable::PropertyTable):
+ * runtime/Structure.cpp:
+ (JSC::Structure::findStructuresAndMapForMaterialization):
+ (JSC::Structure::getConcurrently):
+ * runtime/Structure.h:
+ (Structure):
+ * runtime/StructureInlines.h:
+ (JSC::Structure::getConcurrently):
+
+2013-07-16 Oliver Hunt <oliver@apple.com>
+
Merge dfgFourthTier r148850
2013-04-21 Filip Pizlo <fpizlo@apple.com>
diff --git a/Source/JavaScriptCore/bytecode/GetByIdStatus.cpp b/Source/JavaScriptCore/bytecode/GetByIdStatus.cpp
index be64c5b..13fb848 100644
--- a/Source/JavaScriptCore/bytecode/GetByIdStatus.cpp
+++ b/Source/JavaScriptCore/bytecode/GetByIdStatus.cpp
@@ -51,7 +51,7 @@
unsigned attributesIgnored;
JSCell* specificValue;
- PropertyOffset offset = structure->get(
+ PropertyOffset offset = structure->getConcurrently(
*profiledBlock->vm(), ident, attributesIgnored, specificValue);
if (structure->isDictionary())
specificValue = 0;
@@ -92,7 +92,7 @@
unsigned attributesIgnored;
JSCell* specificValue;
- result.m_offset = currentStructure->get(
+ result.m_offset = currentStructure->getConcurrently(
*profiledBlock->vm(), ident, attributesIgnored, specificValue);
if (currentStructure->isDictionary())
specificValue = 0;
@@ -166,7 +166,7 @@
Structure* structure = stubInfo.u.getByIdSelf.baseObjectStructure.get();
unsigned attributesIgnored;
JSCell* specificValue;
- result.m_offset = structure->getWithoutMaterializing(
+ result.m_offset = structure->getConcurrently(
*profiledBlock->vm(), ident, attributesIgnored, specificValue);
if (structure->isDictionary())
specificValue = 0;
@@ -191,7 +191,7 @@
unsigned attributesIgnored;
JSCell* specificValue;
- PropertyOffset myOffset = structure->getWithoutMaterializing(
+ PropertyOffset myOffset = structure->getConcurrently(
*profiledBlock->vm(), ident, attributesIgnored, specificValue);
if (structure->isDictionary())
specificValue = 0;
@@ -276,7 +276,7 @@
result.m_wasSeenInJIT = false; // To my knowledge nobody that uses computeFor(VM&, Structure*, Identifier&) reads this field, but I might as well be honest: no, it wasn't seen in the JIT, since I computed it statically.
unsigned attributes;
JSCell* specificValue;
- result.m_offset = structure->getWithoutMaterializing(vm, ident, attributes, specificValue);
+ result.m_offset = structure->getConcurrently(vm, ident, attributes, specificValue);
if (!isValidOffset(result.m_offset))
return GetByIdStatus(TakesSlowPath); // It's probably a prototype lookup. Give up on life for now, even though we could totally be way smarter about it.
if (attributes & Accessor)
diff --git a/Source/JavaScriptCore/bytecode/PutByIdStatus.cpp b/Source/JavaScriptCore/bytecode/PutByIdStatus.cpp
index a63ccd2..45490b3 100644
--- a/Source/JavaScriptCore/bytecode/PutByIdStatus.cpp
+++ b/Source/JavaScriptCore/bytecode/PutByIdStatus.cpp
@@ -49,7 +49,7 @@
if (instruction[0].u.opcode == LLInt::getOpcode(llint_op_put_by_id)
|| instruction[0].u.opcode == LLInt::getOpcode(llint_op_put_by_id_out_of_line)) {
- PropertyOffset offset = structure->getWithoutMaterializing(*profiledBlock->vm(), ident);
+ PropertyOffset offset = structure->getConcurrently(*profiledBlock->vm(), ident);
if (!isValidOffset(offset))
return PutByIdStatus(NoInformation, 0, 0, 0, invalidOffset);
@@ -68,7 +68,7 @@
ASSERT(newStructure);
ASSERT(chain);
- PropertyOffset offset = newStructure->getWithoutMaterializing(*profiledBlock->vm(), ident);
+ PropertyOffset offset = newStructure->getConcurrently(*profiledBlock->vm(), ident);
if (!isValidOffset(offset))
return PutByIdStatus(NoInformation, 0, 0, 0, invalidOffset);
@@ -106,7 +106,7 @@
case access_put_by_id_replace: {
PropertyOffset offset =
- stubInfo.u.putByIdReplace.baseObjectStructure->getWithoutMaterializing(
+ stubInfo.u.putByIdReplace.baseObjectStructure->getConcurrently(
*profiledBlock->vm(), ident);
if (isValidOffset(offset)) {
return PutByIdStatus(
@@ -122,7 +122,7 @@
case access_put_by_id_transition_direct: {
ASSERT(stubInfo.u.putByIdTransition.previousStructure->transitionWatchpointSetHasBeenInvalidated());
PropertyOffset offset =
- stubInfo.u.putByIdTransition.structure->getWithoutMaterializing(
+ stubInfo.u.putByIdTransition.structure->getConcurrently(
*profiledBlock->vm(), ident);
if (isValidOffset(offset)) {
return PutByIdStatus(
@@ -156,7 +156,7 @@
unsigned attributes;
JSCell* specificValue;
- PropertyOffset offset = structure->getWithoutMaterializing(
+ PropertyOffset offset = structure->getConcurrently(
vm, ident, attributes, specificValue);
if (isValidOffset(offset)) {
if (attributes & (Accessor | ReadOnly))
diff --git a/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp b/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
index 27bb1ac..f4d80aa 100644
--- a/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
+++ b/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
@@ -1083,7 +1083,7 @@
{
unsigned attributesUnused;
JSCell* specificValue;
- PropertyOffset offset = stringPrototypeStructure->get(
+ PropertyOffset offset = stringPrototypeStructure->getConcurrently(
vm(), ident, attributesUnused, specificValue);
if (!isValidOffset(offset))
return false;
diff --git a/Source/JavaScriptCore/runtime/Structure.cpp b/Source/JavaScriptCore/runtime/Structure.cpp
index e365dff..1998eca 100644
--- a/Source/JavaScriptCore/runtime/Structure.cpp
+++ b/Source/JavaScriptCore/runtime/Structure.cpp
@@ -236,19 +236,27 @@
static_cast<Structure*>(cell)->Structure::~Structure();
}
-void Structure::findStructuresAndMapForMaterialization(Vector<Structure*, 8>& structures, PropertyTable*& table)
+void Structure::findStructuresAndMapForMaterialization(Vector<Structure*, 8>& structures, Structure*& structure, PropertyTable*& table)
{
ASSERT(structures.isEmpty());
table = 0;
- for (Structure* structure = this; structure; structure = structure->previousID()) {
- if (structure->propertyTable()) {
- table = structure->propertyTable().get();
+ for (structure = this; structure; structure = structure->previousID()) {
+ structure->m_lock.lock();
+
+ table = structure->propertyTable().get();
+ if (table) {
+ // Leave the structure locked, so that the caller can do things to it atomically
+ // before it loses its property table.
return;
}
structures.append(structure);
+ structure->m_lock.unlock();
}
+
+ ASSERT(!structure);
+ ASSERT(!table);
}
void Structure::materializePropertyMap(VM& vm)
@@ -257,17 +265,27 @@
ASSERT(!propertyTable());
Vector<Structure*, 8> structures;
+ Structure* structure;
PropertyTable* table;
- findStructuresAndMapForMaterialization(structures, table);
+ findStructuresAndMapForMaterialization(structures, structure, table);
+ if (table) {
+ table = table->copy(vm, 0, numberOfSlotsForLastOffset(m_offset, m_inlineCapacity));
+ structure->m_lock.unlock();
+ }
+
+ // Must hold the lock on this structure, since we will be modifying this structure's
+ // property map. We don't want getConcurrently() to see the property map in a half-baked
+ // state.
+ Locker locker(m_lock);
if (!table)
- createPropertyMap(vm, numberOfSlotsForLastOffset(m_offset, m_inlineCapacity));
+ createPropertyMap(locker, vm, numberOfSlotsForLastOffset(m_offset, m_inlineCapacity));
else
- propertyTable().set(vm, this, table->copy(vm, 0, numberOfSlotsForLastOffset(m_offset, m_inlineCapacity)));
+ propertyTable().set(vm, this, table);
for (size_t i = structures.size(); i--;) {
- Structure* structure = structures[i];
+ structure = structures[i];
if (!structure->m_nameInPrevious)
continue;
PropertyMapEntry entry(vm, this, structure->m_nameInPrevious.get(), structure->m_offset, structure->m_attributesInPrevious, structure->m_specificValueInPrevious.get());
@@ -544,8 +562,14 @@
PropertyTable* Structure::takePropertyTableOrCloneIfPinned(VM& vm, Structure* owner)
{
materializePropertyMapIfNecessaryForPinning(vm);
+
if (m_isPinnedPropertyTable)
return propertyTable()->copy(vm, owner, propertyTable()->size() + 1);
+
+ // Hold the lock while stealing the table - so that getConcurrently() on another thread
+ // will either have to bypass this structure, or will get to use the property table
+ // before it is stolen.
+ Locker locker(m_lock);
PropertyTable* takenPropertyTable = propertyTable().get();
propertyTable().clear();
return takenPropertyTable;
@@ -747,24 +771,32 @@
return PropertyTable::create(vm, numberOfSlotsForLastOffset(m_offset, m_inlineCapacity));
}
-PropertyOffset Structure::getWithoutMaterializing(VM&, PropertyName propertyName, unsigned& attributes, JSCell*& specificValue)
+PropertyOffset Structure::getConcurrently(VM&, PropertyName propertyName, unsigned& attributes, JSCell*& specificValue)
{
+ // We can't handle uncacheable dictionaries because we can't handle concurrent remove's
+ // from the property maps.
+ RELEASE_ASSERT(!isUncacheableDictionary());
+
Vector<Structure*, 8> structures;
+ Structure* structure;
PropertyTable* table;
- findStructuresAndMapForMaterialization(structures, table);
+ findStructuresAndMapForMaterialization(structures, structure, table);
if (table) {
PropertyMapEntry* entry = table->find(propertyName.uid()).first;
if (entry) {
attributes = entry->attributes;
specificValue = entry->specificValue.get();
- return entry->offset;
+ PropertyOffset result = entry->offset;
+ structure->m_lock.unlock();
+ return result;
}
+ structure->m_lock.unlock();
}
for (unsigned i = structures.size(); i--;) {
- Structure* structure = structures[i];
+ structure = structures[i];
if (structure->m_nameInPrevious.get() != propertyName.uid())
continue;
@@ -821,6 +853,8 @@
PropertyOffset Structure::putSpecificValue(VM& vm, PropertyName propertyName, unsigned attributes, JSCell* specificValue)
{
+ Locker locker(m_lock);
+
ASSERT(!JSC::isValidOffset(get(vm, propertyName)));
checkConsistency();
@@ -830,7 +864,7 @@
StringImpl* rep = propertyName.uid();
if (!propertyTable())
- createPropertyMap(vm);
+ createPropertyMap(locker, vm);
PropertyOffset newOffset = propertyTable()->nextOffset(m_inlineCapacity);
@@ -842,6 +876,8 @@
PropertyOffset Structure::remove(PropertyName propertyName)
{
+ Locker locker(m_lock);
+
checkConsistency();
StringImpl* rep = propertyName.uid();
@@ -862,7 +898,7 @@
return offset;
}
-void Structure::createPropertyMap(VM& vm, unsigned capacity)
+void Structure::createPropertyMap(const Locker&, VM& vm, unsigned capacity)
{
ASSERT(!propertyTable());
diff --git a/Source/JavaScriptCore/runtime/Structure.h b/Source/JavaScriptCore/runtime/Structure.h
index 3d5ea4b..655c0bd 100644
--- a/Source/JavaScriptCore/runtime/Structure.h
+++ b/Source/JavaScriptCore/runtime/Structure.h
@@ -39,6 +39,8 @@
#include "StructureTransitionTable.h"
#include "JSTypeInfo.h"
#include "Watchpoint.h"
+#include "Weak.h"
+#include <wtf/ByteSpinLock.h>
#include <wtf/PassRefPtr.h>
#include <wtf/RefCounted.h>
#include <wtf/text/StringImpl.h>
@@ -68,6 +70,9 @@
friend class StructureTransitionTable;
typedef JSCell Base;
+
+ typedef ByteSpinLock Lock;
+ typedef ByteSpinLocker Locker;
static Structure* create(VM&, JSGlobalObject*, JSValue prototype, const TypeInfo&, const ClassInfo*, IndexingType = NonArray, unsigned inlineCapacity = 0);
@@ -235,8 +240,8 @@
PropertyOffset get(VM&, const WTF::String& name);
JS_EXPORT_PRIVATE PropertyOffset get(VM&, PropertyName, unsigned& attributes, JSCell*& specificValue);
- PropertyOffset getWithoutMaterializing(VM&, PropertyName);
- PropertyOffset getWithoutMaterializing(VM&, PropertyName, unsigned& attributes, JSCell*& specificValue);
+ PropertyOffset getConcurrently(VM&, PropertyName);
+ PropertyOffset getConcurrently(VM&, PropertyName, unsigned& attributes, JSCell*& specificValue);
bool hasGetterSetterProperties() const { return m_hasGetterSetterProperties; }
bool hasReadOnlyOrGetterSetterPropertiesExcludingProto() const { return m_hasReadOnlyOrGetterSetterPropertiesExcludingProto; }
@@ -360,8 +365,12 @@
Structure(VM&, const Structure*);
static Structure* create(VM&, const Structure*);
-
- void findStructuresAndMapForMaterialization(Vector<Structure*, 8>& structures, PropertyTable*&);
+
+ // This will return the structure that has a usable property table, that property table,
+ // and the list of structures that we visited before we got to it. If it returns a
+ // non-null structure, it will also lock the structure that it returns; it is your job
+ // to unlock it.
+ void findStructuresAndMapForMaterialization(Vector<Structure*, 8>& structures, Structure*&, PropertyTable*&);
typedef enum {
NoneDictionaryKind = 0,
@@ -373,7 +382,7 @@
PropertyOffset putSpecificValue(VM&, PropertyName, unsigned attributes, JSCell* specificValue);
PropertyOffset remove(PropertyName);
- void createPropertyMap(VM&, unsigned keyCount = 0);
+ void createPropertyMap(const Locker&, VM&, unsigned keyCount = 0);
void checkConsistency();
bool despecifyFunction(VM&, PropertyName);
@@ -472,8 +481,10 @@
TypeInfo m_typeInfo;
IndexingType m_indexingType;
-
uint8_t m_inlineCapacity;
+
+ Lock m_lock;
+
unsigned m_dictionaryKind : 2;
bool m_isPinnedPropertyTable : 1;
bool m_hasGetterSetterProperties : 1;
diff --git a/Source/JavaScriptCore/runtime/StructureInlines.h b/Source/JavaScriptCore/runtime/StructureInlines.h
index 49a861c..69064df 100644
--- a/Source/JavaScriptCore/runtime/StructureInlines.h
+++ b/Source/JavaScriptCore/runtime/StructureInlines.h
@@ -78,11 +78,11 @@
return entry ? entry->offset : invalidOffset;
}
-inline PropertyOffset Structure::getWithoutMaterializing(VM& vm, PropertyName propertyName)
+inline PropertyOffset Structure::getConcurrently(VM& vm, PropertyName propertyName)
{
unsigned attributesIgnored;
JSCell* specificValueIgnored;
- return getWithoutMaterializing(
+ return getConcurrently(
vm, propertyName, attributesIgnored, specificValueIgnored);
}
