For critical sections that are held for a long time, it makes sense to have a completely fair lock, since the cost of forced context switches is small by comparison to the cost of waiting on a thread that barged into the lock.
Created attachment 282637 [details] maybe I haven't even compiled this yet.
Created attachment 282642 [details] OMG it totally works!
Created attachment 282643 [details] the patch
Attachment 282643 [details] did not pass style-queue: ERROR: Source/WTF/wtf/ParkingLot.cpp:622: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/wtf/ParkingLot.cpp:659: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/wtf/ParkingLot.cpp:698: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/wtf/ParkingLot.cpp:738: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 4 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 282644 [details] the patch
Attachment 282644 [details] did not pass style-queue: ERROR: Source/WTF/wtf/ParkingLot.cpp:622: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/wtf/ParkingLot.cpp:659: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/wtf/ParkingLot.cpp:698: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/wtf/ParkingLot.cpp:738: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 4 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 282644 [details] the patch Attachment 282644 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1615273 Number of test failures exceeded the failure limit.
Created attachment 282645 [details] Archive of layout-test-results from ews113 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 282646 [details] the patch Turns out I had an assertion exactly backwards.
Attachment 282646 [details] did not pass style-queue: ERROR: Source/WTF/wtf/ParkingLot.cpp:622: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/wtf/ParkingLot.cpp:666: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/wtf/ParkingLot.cpp:705: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/wtf/ParkingLot.cpp:745: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 4 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 282661 [details] the patch
Attachment 282661 [details] did not pass style-queue: ERROR: Source/WTF/wtf/ParkingLot.cpp:622: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/wtf/ParkingLot.cpp:666: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/wtf/ParkingLot.cpp:705: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/wtf/ParkingLot.cpp:745: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 4 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 282661 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=282661&action=review This is pretty cool. Randomness wins again! > Source/WTF/wtf/Lock.cpp:121 > + return 1; It's nice that token establishes a separation of concerns between ParkingLot and Lock, but it's also a bummer for readability that we have to be so abstract (0 vs 1) about a concept that has only one use -- to determine if we need to lock or not. One way to solve this is to use an old-school enum inside Lock, which can auto-convert to int: enum LockToken : intptr_t { DoesNotHoldLock, HoldsLock }; Or maybe a class inside Lock: class LockToken { public: LockToken(bool holdsLock) : m_holdsLock(holdsLock) { } operator intptr_t() { return m_holdsLock; } bool holdsLock() { return m_holdsLock; } private: bool m_holdsLock; }; > Source/WTF/wtf/Lock.h:134 > + enum Fairness { Let's use enum class. The type safety and namespacing are nice.
(In reply to comment #13) > Comment on attachment 282661 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=282661&action=review > > This is pretty cool. Randomness wins again! > > > Source/WTF/wtf/Lock.cpp:121 > > + return 1; > > It's nice that token establishes a separation of concerns between ParkingLot > and Lock, but it's also a bummer for readability that we have to be so > abstract (0 vs 1) about a concept that has only one use -- to determine if > we need to lock or not. You're right, this should be an enum! > > One way to solve this is to use an old-school enum inside Lock, which can > auto-convert to int: > > enum LockToken : intptr_t { > DoesNotHoldLock, > HoldsLock > }; Or we could use the Fairness enum. > > Or maybe a class inside Lock: > > class LockToken { > public: > LockToken(bool holdsLock) : m_holdsLock(holdsLock) { } > operator intptr_t() { return m_holdsLock; } > bool holdsLock() { return m_holdsLock; } > > private: > bool m_holdsLock; > }; > > > Source/WTF/wtf/Lock.h:134 > > + enum Fairness { > > Let's use enum class. The type safety and namespacing are nice. OK.
I ended up going with normal enums, not enum classes, because this is not valid: enum class Thing : intptr_t { Foo, Bar } [&] (things) -> intptr_t { return Thing::Foo; // Compiler error, it wants a cast here!! } Also, for the Fairness enum, it felt really awkward to say Fairness::Fair and Fairness::Unfair. I don't know if we have a style rule for this, but I've been using the rule that plain enums are better if prefixing the enum class name would be tautological. It is in this case, since the enums are already encapsulated inside the protected namespace of WTF::LockBase and the names of the members tell you everything you need to know.
Created attachment 282802 [details] patch for landing
Attachment 282802 [details] did not pass style-queue: ERROR: Source/WTF/wtf/ParkingLot.cpp:622: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/wtf/ParkingLot.cpp:666: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/wtf/ParkingLot.cpp:705: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WTF/wtf/ParkingLot.cpp:745: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 4 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Landed in https://trac.webkit.org/changeset/203350