Title: [207577] trunk/Source/WTF
- Revision
- 207577
- Author
- fpi...@apple.com
- Date
- 2016-10-19 16:37:20 -0700 (Wed, 19 Oct 2016)
Log Message
REGRESSION (r207480): 3 Dromaeo tests failing
https://bugs.webkit.org/show_bug.cgi?id=163633
Reviewed by Mark Lam.
It's a ParkingLot bug: if we timeout and get unparked at the same time, then the unparking
thread will clear our address eventually - but not immediately. This causes a nasty
assertion failure. The tricky thing is that when we detect this, we need to wait until that
unparking thread does the deed. Otherwise, they will still do it at some later time.
Alternatively, we could use some kind of versioning to detect this - increment the version
when you park, so that unparking threads will know if they are time travelers. That seems
more yucky.
I don't think it matters too much what we do here, so long as it's simple and correct, since
this requires a race that is rare enough that it didn't happen once in my testing, and I was
pretty thorough. For example, it didn't happen once in 15 runs of JetStream. The race is
rare because it requires the timeout to happen right as someone else is unparking. Since
it's so rare, its probably OK that the unparked thread waits just a tiny moment until the
unparking thread is done.
* wtf/ParkingLot.cpp:
(WTF::ParkingLot::parkConditionallyImpl):
(WTF::ParkingLot::unparkOneImpl):
Modified Paths
Diff
Modified: trunk/Source/WTF/ChangeLog (207576 => 207577)
--- trunk/Source/WTF/ChangeLog 2016-10-19 23:34:45 UTC (rev 207576)
+++ trunk/Source/WTF/ChangeLog 2016-10-19 23:37:20 UTC (rev 207577)
@@ -1,5 +1,32 @@
2016-10-19 Filip Pizlo <fpi...@apple.com>
+ REGRESSION (r207480): 3 Dromaeo tests failing
+ https://bugs.webkit.org/show_bug.cgi?id=163633
+
+ Reviewed by Mark Lam.
+
+ It's a ParkingLot bug: if we timeout and get unparked at the same time, then the unparking
+ thread will clear our address eventually - but not immediately. This causes a nasty
+ assertion failure. The tricky thing is that when we detect this, we need to wait until that
+ unparking thread does the deed. Otherwise, they will still do it at some later time.
+
+ Alternatively, we could use some kind of versioning to detect this - increment the version
+ when you park, so that unparking threads will know if they are time travelers. That seems
+ more yucky.
+
+ I don't think it matters too much what we do here, so long as it's simple and correct, since
+ this requires a race that is rare enough that it didn't happen once in my testing, and I was
+ pretty thorough. For example, it didn't happen once in 15 runs of JetStream. The race is
+ rare because it requires the timeout to happen right as someone else is unparking. Since
+ it's so rare, its probably OK that the unparked thread waits just a tiny moment until the
+ unparking thread is done.
+
+ * wtf/ParkingLot.cpp:
+ (WTF::ParkingLot::parkConditionallyImpl):
+ (WTF::ParkingLot::unparkOneImpl):
+
+2016-10-19 Filip Pizlo <fpi...@apple.com>
+
Baseline JIT should use AutomaticThread
https://bugs.webkit.org/show_bug.cgi?id=163686
Modified: trunk/Source/WTF/wtf/ParkingLot.cpp (207576 => 207577)
--- trunk/Source/WTF/wtf/ParkingLot.cpp 2016-10-19 23:34:45 UTC (rev 207576)
+++ trunk/Source/WTF/wtf/ParkingLot.cpp 2016-10-19 23:37:20 UTC (rev 207577)
@@ -640,10 +640,14 @@
// Make sure that no matter what, me->address is null after this point.
{
- std::lock_guard<std::mutex> locker(me->parkingLock);
+ std::unique_lock<std::mutex> locker(me->parkingLock);
if (!didDequeue) {
- // If we were unparked then our address would have been reset by the unparker.
- RELEASE_ASSERT(!me->address);
+ // If we did not dequeue ourselves, then someone else did. They will set our address to
+ // null. We don't want to proceed until they do this, because otherwise, they may set
+ // our address to null in some distant future when we're already trying to wait for
+ // other things.
+ while (me->address)
+ me->parkingCondition.wait(locker);
}
me->address = nullptr;
}
@@ -735,6 +739,7 @@
std::unique_lock<std::mutex> locker(threadData->parkingLock);
threadData->address = nullptr;
}
+ // At this point, the threadData may die. Good thing we have a RefPtr<> on it.
threadData->parkingCondition.notify_one();
}
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes
