Bug 159384 – WTF::Lock should be fair eventually

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Filip Pizlo]

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.

[Filip Pizlo]

Created attachment 282637 [details]
maybe

I haven't even compiled this yet.

[Filip Pizlo]

Created attachment 282642 [details]
OMG it totally works!

[Filip Pizlo]

Created attachment 282643 [details]
the patch

[WebKit Commit Bot]

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.

[Filip Pizlo]

Created attachment 282644 [details]
the patch

[WebKit Commit Bot]

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.

[Build Bot]

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.

[Build Bot]

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

[Filip Pizlo]

Created attachment 282646 [details]
the patch

Turns out I had an assertion exactly backwards.

[WebKit Commit Bot]

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.

[Filip Pizlo]

Created attachment 282661 [details]
the patch

[WebKit Commit Bot]

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.

[Geoffrey Garen]

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.

[Filip Pizlo]

(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.

[Filip Pizlo]

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.

[Filip Pizlo]

Created attachment 282802 [details]
patch for landing

[WebKit Commit Bot]

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.

[Filip Pizlo]

Landed in https://trac.webkit.org/changeset/203350