Introduce ASRecursiveUnfairLock and tests #858

Adlai-Holler · 2018-03-27T23:28:37Z

Adds a new C lock struct that adds recursiveness to unfair_lock but isn't much slower.
Adds an experiment exp_unfair_lock that makes all ASDN::Mutex instances use either os_unfair_lock or ASRecursiveUnfairLock under the hood, if we're on iOS/tvOS >= 10.
The name Mutex isn't totally appropriate now but it's fine. If this works well we can move it.

Here's the perf on my 2016 MBP in release mode. Take synthetic tests with a grain of salt. "Uncontested" (less contested) = 2 threads, contested = 16 threads. See ASRecursiveUnfairLockTests.m.

Takeaways:

Close to plain unfair lock performance in uncontested case.
25x faster in uncontested case, 50x faster in contested case.

Data:

testNoLockUncontested: average: 1ms, relative standard deviation: 15.788%
testPlainUnfairLockUncontested: average: 4ms, relative standard deviation: 26.805%
testRecursiveUnfairLockUncontested: average: 5ms, relative standard deviation: 10.843%
testRecursiveMutexUncontested: average: 129ms, relative standard deviation: 3.632%

testNoLockContested: average: 4ms, relative standard deviation: 7.002%
testPlainUnfairLockContested: average: 18ms, relative standard deviation: 3.598%
testRecursiveUnfairLockContested: average: 25ms, relative standard deviation: 12.423%
testRecursiveMutexContested: average: 1248ms, relative standard deviation: 3.859%

Adlai-Holler · 2018-03-27T23:37:00Z

Source/Details/ASThread.h

+      if (gMutex_unfair) {
+        // nop
+      } else {
+        AS_POSIX_ASSERT_NOERR(pthread_mutex_destroy (&_m));


I renamed this assert macro just because it was so dang long.

Adlai-Holler · 2018-03-27T23:37:20Z

Source/Details/ASThread.h

-    // non-copyable.
-    ReadWriteLockWriteLocker(const ReadWriteLockWriteLocker&) = delete;
-    ReadWriteLockWriteLocker &operator=(const ReadWriteLockWriteLocker&) = delete;
-  };


Removed unused C++ classes from this header.

appleguy · 2018-03-28T04:47:20Z

This is a seminal contribution. Texture (and AsyncDisplayKit) developers have always dreamed of a faster locking primitive than the unnecessarily-fair pthread mutex, but it has felt just out of reach with so much else to do. Thank you @Adlai-Holler for jumping on this opportunity!

Some background reading for folks unfamiliar with os_unfair_lock:
https://blog.mozilla.org/nfroyd/2017/03/29/on-mutex-performance-part-1/ (discusses that pthread mutex happens to be fair on OS X / iOS platforms)
https://webkit.org/blog/6161/locking-in-webkit/ (the path that Safari / WebKit developers took to address this problem)
https://twitter.com/catfish_man/status/743252500728315905?lang=en (excitement about the new-in-iOS 10 API)
https://gist.github.com/steipete/36350a8a60693d440954b95ea6cbbafc (some hard-to-read, but more broad benchmarking comparisons with other iOS locking primitives)

jparise

Looks good to me. I don't see any issues or obvious optimization opportunities.

jparise · 2018-03-28T15:32:50Z

Source/ASConfigurationInternal.h

@@ -10,6 +10,8 @@
 //      http://www.apache.org/licenses/LICENSE-2.0
 //

+/// Note this has to be public because it's imported by public header ASThread.h =/


But only temporarily for the lifetime of the exp_unfair_lock experiment?

jparise · 2018-03-28T15:42:31Z

Source/Details/ASRecursiveUnfairLock.m

+  // Just a cache for pthread_self so that we never call it twice.
+  pthread_t s = NULL;
+
+  // Try to lock without blocking. If we fail, check what thread owns it.


grammar nit: s/what/which/

jparise · 2018-03-28T15:54:06Z

Source/Details/ASThread.h

+          if (!recursive) {
+            AS_POSIX_ASSERT_NOERR(pthread_mutex_init (&_m, NULL));
+          } else {
+          // Fall back to recursive mutex.


Is the indentation here off one level?

ghost · 2018-03-28T17:45:04Z

	1 Warning
⚠️	This is a big PR, please consider splitting it up to ease code review.

Generated by 🚫 Danger

Adlai-Holler · 2018-03-28T18:29:46Z

Thanks Jon for reviewing. Merging!

garrettmoon · 2018-03-30T03:29:55Z

We should be on the lookout for deadlocks due to priority inversion when this lands. Of course if all clients and Texture itself are built correctly, no priority inversion cases will come up 🤞. I'm definitely excited for the perf gains, reducing locking contention will be huge!

Adlai-Holler · 2018-03-30T03:39:36Z

We good, unfair locks protect against priority inversion! All that they lack is a guarantee that you will get the lock first if you tried to lock first:

From https://opensource.apple.com/source/libplatform/libplatform-125/include/os/lock_private.h.auto.html

 * @abstract
 * Replacement for the deprecated OSSpinLock. Does not spin on contention but
 * waits in the kernel to be woken up by an unlock. The opaque lock value
 * contains thread ownership information that the system may use to attempt to
 * resolve priority inversions.

garrettmoon · 2018-03-30T14:47:59Z

Amazing :)

* Introduce ASRecursiveUnfairLock and tests * Document it and put underscores to scare people away * Increment changelog * Integrate it with ASDN::Mutex behind experiment * Rename the experiment * Love these license headers oh so much * Move internal header because we have to * Address Jon's feedback

- Followup to #1742 - At Pinterest this shipped with D516974 in late 02/2020 - As discussed in #858 this is iOS10 or later, so the runtime `gMutex_unfair` check is still necessary for Texture.

- Followup to TextureGroup#1742 - At Pinterest this shipped with D516974 in late 02/2020 - As discussed in TextureGroup#858 this is iOS10 or later, so the runtime `gMutex_unfair` check is still necessary for Texture.

Adlai Holler added 5 commits March 27, 2018 14:25

Introduce ASRecursiveUnfairLock and tests

e1c6ec8

Document it and put underscores to scare people away

b56c792

Increment changelog

0474e17

Integrate it with ASDN::Mutex behind experiment

e3248aa

Rename the experiment

80f9dc4

Adlai-Holler requested review from jparise, maicki, garrettmoon, appleguy and nguyenhuy March 27, 2018 23:28

Adlai Holler added 2 commits March 27, 2018 16:30

Love these license headers oh so much

9ff1223

Move internal header because we have to

2b4e824

TextureGroup deleted a comment Mar 27, 2018

Adlai-Holler commented Mar 27, 2018

View reviewed changes

jparise approved these changes Mar 28, 2018

View reviewed changes

Address Jon's feedback

4b4e1a9

Adlai-Holler merged commit 4bbbd72 into master Mar 28, 2018

Adlai-Holler deleted the AHRecursiveUnfairLock branch March 28, 2018 18:29

bolsinga mentioned this pull request May 26, 2020

Ship ASExperimentalUnfairLock experiment #1841

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce ASRecursiveUnfairLock and tests #858

Introduce ASRecursiveUnfairLock and tests #858

Adlai-Holler commented Mar 27, 2018

Adlai-Holler Mar 27, 2018

Adlai-Holler Mar 27, 2018

appleguy commented Mar 28, 2018

jparise left a comment

jparise Mar 28, 2018

jparise Mar 28, 2018

jparise Mar 28, 2018

ghost commented Mar 28, 2018

Adlai-Holler commented Mar 28, 2018

garrettmoon commented Mar 30, 2018 •

edited

Loading

Adlai-Holler commented Mar 30, 2018

garrettmoon commented Mar 30, 2018

Introduce ASRecursiveUnfairLock and tests #858

Introduce ASRecursiveUnfairLock and tests #858

Conversation

Adlai-Holler commented Mar 27, 2018

Adlai-Holler Mar 27, 2018

Choose a reason for hiding this comment

Adlai-Holler Mar 27, 2018

Choose a reason for hiding this comment

appleguy commented Mar 28, 2018

jparise left a comment

Choose a reason for hiding this comment

jparise Mar 28, 2018

Choose a reason for hiding this comment

jparise Mar 28, 2018

Choose a reason for hiding this comment

jparise Mar 28, 2018

Choose a reason for hiding this comment

ghost commented Mar 28, 2018

Adlai-Holler commented Mar 28, 2018

garrettmoon commented Mar 30, 2018 • edited Loading

Adlai-Holler commented Mar 30, 2018

garrettmoon commented Mar 30, 2018

garrettmoon commented Mar 30, 2018 •

edited

Loading