Modified: trunk/Source/_javascript_Core/ChangeLog (197380 => 197381)
--- trunk/Source/_javascript_Core/ChangeLog 2016-03-01 02:30:46 UTC (rev 197380)
+++ trunk/Source/_javascript_Core/ChangeLog 2016-03-01 03:18:59 UTC (rev 197381)
@@ -1,3 +1,38 @@
+2016-02-29 Filip Pizlo <fpi...@apple.com>
+
+ regress/script-tests/double-pollution-putbyoffset.js.ftl-eager timed out because of a lock ordering deadlock involving InferredType and CodeBlock
+ https://bugs.webkit.org/show_bug.cgi?id=154841
+
+ Reviewed by Benjamin Poulain.
+
+ Here's the deadlock:
+
+ Main thread:
+ 1) Change an InferredType. This acquires InferredType::m_lock.
+ 2) Fire watchpoint set. This triggers CodeBlock invalidation, which acquires
+ CodeBlock::m_lock.
+
+ DFG thread:
+ 1) Iterate over the information in a CodeBlock. This acquires CodeBlock::m_lock.
+ 2) Ask an InferredType for its descriptor(). This acquires InferredType::m_lock.
+
+ I think that the DFG thread's ordering should be legal, because the best logic for lock
+ hierarchies is that locks that protect the largest set of stuff should be acquired first.
+
+ This means that the main thread shouldn't be holding the InferredType::m_lock when firing
+ watchpoint sets. That's what this patch ensures.
+
+ At the time of writing, this test was deadlocking for me on trunk 100% of the time. With
+ this change I cannot get it to deadlock.
+
+ * runtime/InferredType.cpp:
+ (JSC::InferredType::willStoreValueSlow):
+ (JSC::InferredType::makeTopSlow):
+ (JSC::InferredType::set):
+ (JSC::InferredType::removeStructure):
+ (JSC::InferredType::InferredStructureWatchpoint::fireInternal):
+ * runtime/InferredType.h:
+
2016-02-29 Yusuke Suzuki <utatane....@gmail.com>
[DFG][FTL][B3] Support floor and ceil
Modified: trunk/Source/_javascript_Core/runtime/InferredType.cpp (197380 => 197381)
--- trunk/Source/_javascript_Core/runtime/InferredType.cpp 2016-03-01 02:30:46 UTC (rev 197380)
+++ trunk/Source/_javascript_Core/runtime/InferredType.cpp 2016-03-01 03:18:59 UTC (rev 197381)
@@ -400,28 +400,44 @@
bool InferredType::willStoreValueSlow(VM& vm, PropertyName propertyName, JSValue value)
{
- ConcurrentJITLocker locker(m_lock);
- Descriptor myType = descriptor(locker);
- Descriptor otherType = Descriptor::forValue(value);
+ Descriptor oldType;
+ Descriptor myType;
+ bool result;
+ {
+ ConcurrentJITLocker locker(m_lock);
+ oldType = descriptor(locker);
+ myType = Descriptor::forValue(value);
- myType.merge(otherType);
+ myType.merge(oldType);
+
+ ASSERT(oldType != myType); // The type must have changed if we're on the slow path.
- ASSERT(descriptor(locker) != myType); // The type must have changed if we're on the slow path.
-
- set(locker, vm, propertyName, value, myType);
-
- return kind(locker) != Top;
+ bool setResult = set(locker, vm, myType);
+ result = kind(locker) != Top;
+ if (!setResult)
+ return result;
+ }
+
+ InferredTypeFireDetail detail(this, propertyName.uid(), oldType, myType, value);
+ m_watchpointSet.fireAll(detail);
+ return result;
}
void InferredType::makeTopSlow(VM& vm, PropertyName propertyName)
{
- ConcurrentJITLocker locker(m_lock);
- set(locker, vm, propertyName, JSValue(), Top);
+ Descriptor oldType;
+ {
+ ConcurrentJITLocker locker(m_lock);
+ oldType = descriptor(locker);
+ if (!set(locker, vm, Top))
+ return;
+ }
+
+ InferredTypeFireDetail detail(this, propertyName.uid(), oldType, Top, JSValue());
+ m_watchpointSet.fireAll(detail);
}
-void InferredType::set(
- const ConcurrentJITLocker& locker, VM& vm, PropertyName propertyName, JSValue offendingValue,
- Descriptor newDescriptor)
+bool InferredType::set(const ConcurrentJITLocker& locker, VM& vm, Descriptor newDescriptor)
{
// We will trigger write barriers while holding our lock. Currently, write barriers don't GC, but that
// could change. If it does, we don't want to deadlock. Note that we could have used
@@ -431,8 +447,10 @@
// Be defensive: if we're not really changing the type, then we don't have to do anything.
if (descriptor(locker) == newDescriptor)
- return;
+ return false;
+ bool shouldFireWatchpointSet = false;
+
// The new descriptor must be more general than the previous one.
ASSERT(newDescriptor.subsumes(descriptor(locker)));
@@ -446,15 +464,12 @@
// We cannot have been invalidated, since if we were, then we'd already be at Top.
ASSERT(m_watchpointSet.state() != IsInvalidated);
- InferredTypeFireDetail detail(
- this, propertyName.uid(), descriptor(locker), newDescriptor, offendingValue);
-
// We're about to do expensive things because some compiler thread decided to watch this type and
// then the type changed. Assume that this property is crazy, and don't ever do any more things for
// it.
newDescriptor = Top;
- m_watchpointSet.fireAll(detail);
+ shouldFireWatchpointSet = true;
}
// Remove the old InferredStructure object if we no longer need it.
@@ -477,6 +492,8 @@
// Assert that we did things.
ASSERT(descriptor(locker) == newDescriptor);
+
+ return shouldFireWatchpointSet;
}
void InferredType::removeStructure()
@@ -486,12 +503,20 @@
VM& vm = *Heap::heap(this)->vm();
- ConcurrentJITLocker locker(m_lock);
-
- Descriptor newDescriptor = descriptor(locker);
- newDescriptor.removeStructure();
-
- set(locker, vm, PropertyName(nullptr), JSValue(), newDescriptor);
+ Descriptor oldDescriptor;
+ Descriptor newDescriptor;
+ {
+ ConcurrentJITLocker locker(m_lock);
+ oldDescriptor = descriptor(locker);
+ newDescriptor = oldDescriptor;
+ newDescriptor.removeStructure();
+
+ if (!set(locker, vm, newDescriptor))
+ return;
+ }
+
+ InferredTypeFireDetail detail(this, nullptr, oldDescriptor, newDescriptor, JSValue());
+ m_watchpointSet.fireAll(detail);
}
void InferredType::InferredStructureWatchpoint::fireInternal(const FireDetail&)
Modified: trunk/Source/_javascript_Core/runtime/InferredType.h (197380 => 197381)
--- trunk/Source/_javascript_Core/runtime/InferredType.h 2016-03-01 02:30:46 UTC (rev 197380)
+++ trunk/Source/_javascript_Core/runtime/InferredType.h 2016-03-01 03:18:59 UTC (rev 197381)
@@ -229,7 +229,11 @@
bool willStoreValueSlow(VM&, PropertyName, JSValue);
void makeTopSlow(VM&, PropertyName);
- void set(const ConcurrentJITLocker&, VM&, PropertyName, JSValue, Descriptor);
+
+ // Helper for willStoreValueSlow() and makeTopSlow(). This returns true if we should fire the
+ // watchpoint set.
+ bool set(const ConcurrentJITLocker&, VM&, Descriptor);
+
void removeStructure();
mutable ConcurrentJITLock m_lock;
