From 35d59acd83bdd6f2d3e177587641a8c5a37aa0f2 Mon Sep 17 00:00:00 2001 From: Adlai Holler Date: Tue, 5 Jun 2018 12:31:30 -0700 Subject: [PATCH] Add support for acquiring multiple locks at once (#958) * Add ASLocking which supports -tryLock and taking multiple locks safely * Better multi locking * Assert about lock set capacity --- AsyncDisplayKit.xcodeproj/project.pbxproj | 4 + CHANGELOG.md | 1 + Source/ASDisplayNode.h | 3 +- Source/ASDisplayNode.mm | 10 +- Source/ASLocking.h | 162 ++++++++++++++++++++++ Source/ASRunLoopQueue.h | 3 +- Source/ASRunLoopQueue.mm | 12 +- Source/AsyncDisplayKit.h | 1 + Source/Details/ASThread.h | 29 ++++ Source/Layout/ASLayoutElement.h | 3 +- Source/Layout/ASLayoutElement.mm | 12 +- Source/Layout/ASLayoutSpec.h | 3 +- Source/Layout/ASLayoutSpec.mm | 12 +- 13 files changed, 209 insertions(+), 46 deletions(-) create mode 100644 Source/ASLocking.h diff --git a/AsyncDisplayKit.xcodeproj/project.pbxproj b/AsyncDisplayKit.xcodeproj/project.pbxproj index e6efd3309b..24a5d473f4 100644 --- a/AsyncDisplayKit.xcodeproj/project.pbxproj +++ b/AsyncDisplayKit.xcodeproj/project.pbxproj @@ -428,6 +428,7 @@ CCEDDDD1200C488000FFCD0A /* ASConfiguration.m in Sources */ = {isa = PBXBuildFile; fileRef = CCEDDDD0200C488000FFCD0A /* ASConfiguration.m */; }; CCEDDDD9200C518800FFCD0A /* ASConfigurationTests.m in Sources */ = {isa = PBXBuildFile; fileRef = CCEDDDD8200C518800FFCD0A /* ASConfigurationTests.m */; }; CCF18FF41D2575E300DF5895 /* NSIndexSet+ASHelpers.h in Headers */ = {isa = PBXBuildFile; fileRef = CC4981BA1D1C7F65004E13CC /* NSIndexSet+ASHelpers.h */; settings = {ATTRIBUTES = (Private, ); }; }; + CCF1FF5E20C4785000AAD8FC /* ASLocking.h in Headers */ = {isa = PBXBuildFile; fileRef = CCF1FF5D20C4785000AAD8FC /* ASLocking.h */; settings = {ATTRIBUTES = (Public, ); }; }; DB55C2671C641AE4004EDCF5 /* ASContextTransitioning.h in Headers */ = {isa = PBXBuildFile; fileRef = DB55C2651C641AE4004EDCF5 /* ASContextTransitioning.h */; settings = {ATTRIBUTES = (Public, ); }; }; DB7121BCD50849C498C886FB /* libPods-AsyncDisplayKitTests.a in Frameworks */ = {isa = PBXBuildFile; fileRef = EFA731F0396842FF8AB635EE /* libPods-AsyncDisplayKitTests.a */; }; DB78412E1C6BCE1600A9E2B4 /* _ASTransitionContext.m in Sources */ = {isa = PBXBuildFile; fileRef = DB55C2601C6408D6004EDCF5 /* _ASTransitionContext.m */; }; @@ -947,6 +948,7 @@ CCEDDDCE200C42A200FFCD0A /* ASConfigurationDelegate.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = ASConfigurationDelegate.h; sourceTree = ""; }; CCEDDDD0200C488000FFCD0A /* ASConfiguration.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = ASConfiguration.m; sourceTree = ""; }; CCEDDDD8200C518800FFCD0A /* ASConfigurationTests.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = ASConfigurationTests.m; sourceTree = ""; }; + CCF1FF5D20C4785000AAD8FC /* ASLocking.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = ASLocking.h; sourceTree = ""; }; D3779BCFF841AD3EB56537ED /* Pods-AsyncDisplayKitTests.release.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-AsyncDisplayKitTests.release.xcconfig"; path = "Pods/Target Support Files/Pods-AsyncDisplayKitTests/Pods-AsyncDisplayKitTests.release.xcconfig"; sourceTree = ""; }; D785F6601A74327E00291744 /* ASScrollNode.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = ASScrollNode.h; sourceTree = ""; }; D785F6611A74327E00291744 /* ASScrollNode.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = ASScrollNode.mm; sourceTree = ""; }; @@ -1162,6 +1164,7 @@ 058D09DD195D050800B7D73C /* ASImageNode.h */, 058D09DE195D050800B7D73C /* ASImageNode.mm */, 68355B2E1CB5799E001D4E68 /* ASImageNode+AnimatedImage.mm */, + CCF1FF5D20C4785000AAD8FC /* ASLocking.h */, 92DD2FE11BF4B97E0074C9DD /* ASMapNode.h */, 92DD2FE21BF4B97E0074C9DD /* ASMapNode.mm */, 0516FA3E1A1563D200B4EBED /* ASMultiplexImageNode.h */, @@ -1871,6 +1874,7 @@ CC56013B1F06E9A700DC4FBE /* ASIntegerMap.h in Headers */, B35061FA1B010EFD0018CF92 /* ASControlNode+Subclasses.h in Headers */, E54E81FC1EB357BD00FFE8E1 /* ASPageTable.h in Headers */, + CCF1FF5E20C4785000AAD8FC /* ASLocking.h in Headers */, B35061F81B010EFD0018CF92 /* ASControlNode.h in Headers */, B35062171B010EFD0018CF92 /* ASDataController.h in Headers */, 34EFC75B1B701BAF00AD841F /* ASDimension.h in Headers */, diff --git a/CHANGELOG.md b/CHANGELOG.md index 8af2841ee9..17a6e76845 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ - Make `ASPerformMainThreadDeallocation` visible in C. [Adlai Holler](https://github.com/Adlai-Holler) - Add snapshot test for astextnode2. [Max Wang](https://github.com/wsdwsd0829) [#935](https://github.com/TextureGroup/Texture/pull/935) - Internal housekeeping on the async transaction (rendering) system. [Adlai Holler](https://github.com/Adlai-Holler) +- Add new protocol `ASLocking` that extends `NSLocking` with `tryLock`, and allows taking multiple locks safely. [Adlai Holler](https://github.com/Adlai-Holler) ## 2.7 - Fix pager node for interface coalescing. [Max Wang](https://github.com/wsdwsd0829) [#877](https://github.com/TextureGroup/Texture/pull/877) diff --git a/Source/ASDisplayNode.h b/Source/ASDisplayNode.h index b389e8a66d..131069a95e 100644 --- a/Source/ASDisplayNode.h +++ b/Source/ASDisplayNode.h @@ -25,6 +25,7 @@ #import #import #import +#import NS_ASSUME_NONNULL_BEGIN @@ -127,7 +128,7 @@ extern NSInteger const ASDefaultDrawingPriority; * */ -@interface ASDisplayNode : NSObject +@interface ASDisplayNode : NSObject /** @name Initializing a node object */ diff --git a/Source/ASDisplayNode.mm b/Source/ASDisplayNode.mm index 102be6d4e2..0b3dc3709e 100644 --- a/Source/ASDisplayNode.mm +++ b/Source/ASDisplayNode.mm @@ -373,15 +373,7 @@ static ASDisplayNodeMethodOverrides GetASDisplayNodeMethodOverrides(Class c) return self; } -- (void)lock -{ - __instanceLock__.lock(); -} - -- (void)unlock -{ - __instanceLock__.unlock(); -} +ASSynthesizeLockingMethodsWithMutex(__instanceLock__); - (void)setViewBlock:(ASDisplayNodeViewBlock)viewBlock { diff --git a/Source/ASLocking.h b/Source/ASLocking.h new file mode 100644 index 0000000000..1e53a150ea --- /dev/null +++ b/Source/ASLocking.h @@ -0,0 +1,162 @@ +// +// ASLocking.h +// Texture +// +// Copyright (c) 2018-present, Pinterest, Inc. All rights reserved. +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// + +#import +#import + +#import + +NS_ASSUME_NONNULL_BEGIN + +#define kLockSetCapacity 32 + +/** + * An extension of NSLocking that supports -tryLock. + */ +@protocol ASLocking + +/// Try to take lock without blocking. Returns whether the lock was taken. +- (BOOL)tryLock; + +@end + +/** + * A set of locks acquired during ASLockSequence. + */ +typedef struct { + unsigned count; + CFTypeRef _Nullable locks[kLockSetCapacity]; +} ASLockSet; + +/** + * Declare a lock set that is automatically unlocked at the end of scope. + * + * We use this instead of a scope-locking macro because we want to be able + * to step through the lock sequence block in the debugger. + */ +#define ASScopedLockSet __unused ASLockSet __attribute__((cleanup(ASUnlockSet))) + +/** + * A block that attempts to add a lock to a lock sequence. + * Such a block is provided to the caller of ASLockSequence. + * + * Returns whether the lock was added. You should return + * NO from your lock sequence body if it returns NO. + * + * For instance, you might write `return addLock(l1) && addLock(l2)`. + * + * @param lock The lock to attempt to add. + * @return YES if the lock was added, NO otherwise. + */ +typedef BOOL(^ASAddLockBlock)(id lock); + +/** + * A block that attempts to lock multiple locks in sequence. + * Such a block is provided by the caller of ASLockSequence. + * + * The block may be run multiple times, if not all locks are immediately + * available. Therefore the block should be idempotent. + * + * The block should attempt to invoke addLock multiple times with + * different locks. It should return NO as soon as any addLock + * operation fails. + * + * For instance, you might write `return addLock(l1) && addLock(l2)`. + * + * @param addLock A block you can call to attempt to add a lock. + * @return YES if all locks were added, NO otherwise. + */ +typedef BOOL(^ASLockSequenceBlock)(NS_NOESCAPE ASAddLockBlock addLock); + +/** + * Unlock and release all of the locks in this lock set. + */ +NS_INLINE void ASUnlockSet(ASLockSet *lockSet) { + for (unsigned i = 0; i < lockSet->count; i++) { + CFTypeRef lock = lockSet->locks[i]; + [(__bridge id)lock unlock]; + CFRelease(lock); + } +} + +/** + * Take multiple locks "simultaneously," avoiding deadlocks + * caused by lock ordering. + * + * The block you provide should attempt to take a series of locks, + * using the provided `addLock` block. As soon as any addLock fails, + * you should return NO. + * + * For example: + * ASLockSequence(^(ASAddLockBlock addLock) ^{ + * return addLock(l0) && addLock(l1); + * }); + * + * Note: This function doesn't protect from lock ordering deadlocks if + * one of the locks is already locked (recursive.) Only locks taken + * inside this function are guaranteed not to cause a deadlock. + */ +NS_INLINE ASLockSet ASLockSequence(NS_NOESCAPE ASLockSequenceBlock body) +{ + __block ASLockSet locks = (ASLockSet){0}; + BOOL (^addLock)(id) = ^(id obj) { + + // nil lock = ignore. + if (!obj) { + return YES; + } + + // If they go over capacity, assert and return YES. + // If we return NO, they will enter an infinite loop. + if (locks.count == kLockSetCapacity) { + ASDisplayNodeCFailAssert(@"Locking more than %d locks at once is not supported.", kLockSetCapacity); + return YES; + } + + if ([obj tryLock]) { + locks.locks[locks.count++] = (__bridge_retained CFTypeRef)obj; + return YES; + } + return NO; + }; + + /** + * Repeatedly try running their block, passing in our `addLock` + * until it succeeds. If it fails, unlock all and yield the thread + * to reduce spinning. + */ + while (true) { + if (body(addLock)) { + // Success + return locks; + } else { + ASUnlockSet(&locks); + locks.count = 0; + sched_yield(); + } + } +} + +/** + * These Foundation classes already implement -tryLock. + */ + +@interface NSLock (ASLocking) +@end + +@interface NSRecursiveLock (ASLocking) +@end + +@interface NSConditionLock (ASLocking) +@end + +NS_ASSUME_NONNULL_END diff --git a/Source/ASRunLoopQueue.h b/Source/ASRunLoopQueue.h index a0ce9f57d8..84f557438f 100644 --- a/Source/ASRunLoopQueue.h +++ b/Source/ASRunLoopQueue.h @@ -17,6 +17,7 @@ #import #import +#import NS_ASSUME_NONNULL_BEGIN @@ -28,7 +29,7 @@ NS_ASSUME_NONNULL_BEGIN @end AS_SUBCLASSING_RESTRICTED -@interface ASRunLoopQueue : ASAbstractRunLoopQueue +@interface ASRunLoopQueue : ASAbstractRunLoopQueue /** * Create a new queue with the given run loop and handler. diff --git a/Source/ASRunLoopQueue.mm b/Source/ASRunLoopQueue.mm index 9670402b5c..fd043d5d02 100644 --- a/Source/ASRunLoopQueue.mm +++ b/Source/ASRunLoopQueue.mm @@ -541,17 +541,7 @@ typedef enum { return _internalQueue.count == 0; } -#pragma mark - NSLocking - -- (void)lock -{ - _internalQueueLock.lock(); -} - -- (void)unlock -{ - _internalQueueLock.unlock(); -} +ASSynthesizeLockingMethodsWithMutex(_internalQueueLock) @end diff --git a/Source/AsyncDisplayKit.h b/Source/AsyncDisplayKit.h index 19d49afe8c..f96084001b 100644 --- a/Source/AsyncDisplayKit.h +++ b/Source/AsyncDisplayKit.h @@ -110,6 +110,7 @@ #import #import #import +#import #import #import #import diff --git a/Source/Details/ASThread.h b/Source/Details/ASThread.h index feed904666..b650fec139 100644 --- a/Source/Details/ASThread.h +++ b/Source/Details/ASThread.h @@ -87,6 +87,16 @@ ASDISPLAYNODE_INLINE void _ASLockScopeUnownedCleanup(id __unsafe_unre id __lockToken __attribute__((cleanup(_ASUnlockScopeCleanup))) NS_VALID_UNTIL_END_OF_SCOPE = nsLocking; \ [__lockToken unlock]; +#define ASSynthesizeLockingMethodsWithMutex(mutex) \ +- (void)lock { mutex.lock(); } \ +- (void)unlock { mutex.unlock(); } \ +- (BOOL)tryLock { return mutex.tryLock(); } + +#define ASSynthesizeLockingMethodsWithObject(object) \ +- (void)lock { [object lock]; } \ +- (void)unlock { [object unlock]; } \ +- (BOOL)tryLock { return [object tryLock]; } + ASDISPLAYNODE_INLINE void _ASUnlockScopeCleanup(id __strong *lockPtr) { [*lockPtr lock]; } @@ -265,6 +275,25 @@ namespace ASDN { Mutex (const Mutex&) = delete; Mutex &operator=(const Mutex&) = delete; + bool tryLock() { + if (gMutex_unfair) { + if (_recursive) { + return ASRecursiveUnfairLockTryLock(&_runfair); + } else { + return os_unfair_lock_trylock(&_unfair); + } + } else { + auto result = pthread_mutex_trylock(&_m); + if (result == 0) { + return true; + } else if (result == EBUSY) { + return false; + } else { + ASDisplayNodeCFailAssert(@"Locking error: %s", strerror(result)); + return true; // if we return false we may enter an infinite loop. + } + } + } void lock() { if (gMutex_unfair) { if (_recursive) { diff --git a/Source/Layout/ASLayoutElement.h b/Source/Layout/ASLayoutElement.h index 2e0b920e59..0d35fe6782 100644 --- a/Source/Layout/ASLayoutElement.h +++ b/Source/Layout/ASLayoutElement.h @@ -22,6 +22,7 @@ #import #import #import +#import @class ASLayout; @class ASLayoutSpec; @@ -174,7 +175,7 @@ extern NSString * const ASLayoutElementStyleLayoutPositionProperty; - (void)style:(__kindof ASLayoutElementStyle *)style propertyDidChange:(NSString *)propertyName; @end -@interface ASLayoutElementStyle : NSObject +@interface ASLayoutElementStyle : NSObject /** * @abstract Initializes the layoutElement style with a specified delegate diff --git a/Source/Layout/ASLayoutElement.mm b/Source/Layout/ASLayoutElement.mm index 8baba53269..62dd62e66e 100644 --- a/Source/Layout/ASLayoutElement.mm +++ b/Source/Layout/ASLayoutElement.mm @@ -176,17 +176,7 @@ do {\ return self; } -#pragma mark - NSLocking - -- (void)lock -{ - __instanceLock__.lock(); -} - -- (void)unlock -{ - __instanceLock__.unlock(); -} +ASSynthesizeLockingMethodsWithMutex(__instanceLock__) #pragma mark - ASLayoutElementStyleSize diff --git a/Source/Layout/ASLayoutSpec.h b/Source/Layout/ASLayoutSpec.h index 437f1e84dc..7bc7f3ef4c 100644 --- a/Source/Layout/ASLayoutSpec.h +++ b/Source/Layout/ASLayoutSpec.h @@ -17,6 +17,7 @@ #import #import +#import #import NS_ASSUME_NONNULL_BEGIN @@ -24,7 +25,7 @@ NS_ASSUME_NONNULL_BEGIN /** * A layout spec is an immutable object that describes a layout, loosely inspired by React. */ -@interface ASLayoutSpec : NSObject +@interface ASLayoutSpec : NSObject /** * Creation of a layout spec should only happen by a user in layoutSpecThatFits:. During that method, a diff --git a/Source/Layout/ASLayoutSpec.mm b/Source/Layout/ASLayoutSpec.mm index ff45add448..7b70df4e3d 100644 --- a/Source/Layout/ASLayoutSpec.mm +++ b/Source/Layout/ASLayoutSpec.mm @@ -259,17 +259,7 @@ ASLayoutElementStyleExtensibilityForwarding return result; } -#pragma mark - NSLocking - -- (void)lock -{ - __instanceLock__.lock(); -} - -- (void)unlock -{ - __instanceLock__.unlock(); -} +ASSynthesizeLockingMethodsWithMutex(__instanceLock__) @end