Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ASThread: Remove Locker, Unlocker, and SharedMutex #1213

Merged
merged 5 commits into from
Nov 5, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 36 additions & 37 deletions Source/ASDisplayNode.mm
Original file line number Diff line number Diff line change
Expand Up @@ -416,12 +416,13 @@ - (ASDisplayNodeMethodOverrides)methodOverrides

- (void)onDidLoad:(ASDisplayNodeDidLoadBlock)body
{
ASDN::MutexLocker l(__instanceLock__);
ASDN::UniqueLock l(__instanceLock__);

if ([self _locked_isNodeLoaded]) {
ASDisplayNodeAssertThreadAffinity(self);
ASDN::MutexUnlocker l(__instanceLock__);
l.unlock();
body(self);
return;
} else if (_onDidLoadBlocks == nil) {
_onDidLoadBlocks = [NSMutableArray arrayWithObject:body];
} else {
Expand Down Expand Up @@ -601,7 +602,7 @@ - (BOOL)_locked_isNodeLoaded

- (UIView *)view
{
ASDN::MutexLocker l(__instanceLock__);
ASDN::UniqueLock l(__instanceLock__);

ASDisplayNodeAssert(!_flags.layerBacked, @"Call to -view undefined on layer-backed nodes");
BOOL isLayerBacked = _flags.layerBacked;
Expand All @@ -626,30 +627,28 @@ - (UIView *)view
// in the background on a loaded node, which isn't currently supported.
if (_pendingViewState.hasSetNeedsLayout) {
// Need to unlock before calling setNeedsLayout to avoid deadlocks.
// MutexUnlocker will re-lock at the end of scope.
ASDN::MutexUnlocker u(__instanceLock__);
l.unlock();
[self __setNeedsLayout];
l.lock();
}

[self _locked_applyPendingStateToViewOrLayer];

{
// The following methods should not be called with a lock
ASDN::MutexUnlocker u(__instanceLock__);
// The following methods should not be called with a lock
l.unlock();

// No need for the lock as accessing the subviews or layers are always happening on main
[self _addSubnodeViewsAndLayers];

// A subclass hook should never be called with a lock
[self _didLoad];
}
// No need for the lock as accessing the subviews or layers are always happening on main
[self _addSubnodeViewsAndLayers];

// A subclass hook should never be called with a lock
[self _didLoad];

return _view;
}

- (CALayer *)layer
{
ASDN::MutexLocker l(__instanceLock__);
ASDN::UniqueLock l(__instanceLock__);
if (_layer != nil) {
return _layer;
}
Expand All @@ -661,31 +660,30 @@ - (CALayer *)layer
// Loading a layer needs to happen on the main thread
ASDisplayNodeAssertMainThread();
[self _locked_loadViewOrLayer];
CALayer *layer = _layer;

// FIXME: Ideally we'd call this as soon as the node receives -setNeedsLayout
// but automatic subnode management would require us to modify the node tree
// in the background on a loaded node, which isn't currently supported.
if (_pendingViewState.hasSetNeedsLayout) {
// Need to unlock before calling setNeedsLayout to avoid deadlocks.
// MutexUnlocker will re-lock at the end of scope.
ASDN::MutexUnlocker u(__instanceLock__);
l.unlock();
[self __setNeedsLayout];
l.lock();
}

[self _locked_applyPendingStateToViewOrLayer];

{
// The following methods should not be called with a lock
ASDN::MutexUnlocker u(__instanceLock__);
// The following methods should not be called with a lock
l.unlock();

// No need for the lock as accessing the subviews or layers are always happening on main
[self _addSubnodeViewsAndLayers];

// A subclass hook should never be called with a lock
[self _didLoad];
}
// No need for the lock as accessing the subviews or layers are always happening on main
[self _addSubnodeViewsAndLayers];

// A subclass hook should never be called with a lock
[self _didLoad];

return _layer;
return layer;
}

// Returns nil if the layer is not an _ASDisplayLayer; will not create the layer if nil.
Expand Down Expand Up @@ -1033,7 +1031,7 @@ - (void)__layout

BOOL loaded = NO;
{
ASDN::MutexLocker l(__instanceLock__);
ASDN::UniqueLock l(__instanceLock__);
loaded = [self _locked_isNodeLoaded];
CGRect bounds = _threadSafeBounds;

Expand All @@ -1055,10 +1053,9 @@ - (void)__layout

// This method will confirm that the layout is up to date (and update if needed).
// Importantly, it will also APPLY the layout to all of our subnodes if (unless parent is transitioning).
{
ASDN::MutexUnlocker u(__instanceLock__);
[self _u_measureNodeWithBoundsIfNecessary:bounds];
}
l.unlock();
[self _u_measureNodeWithBoundsIfNecessary:bounds];
l.lock();

[self _locked_layoutPlaceholderIfNecessary];
}
Expand Down Expand Up @@ -1121,7 +1118,7 @@ - (ASLayout *)calculateLayoutThatFits:(ASSizeRange)constrainedSize
{
__ASDisplayNodeCheckForLayoutMethodOverrides;

ASDN::MutexLocker l(__instanceLock__);
ASDN::UniqueLock l(__instanceLock__);

#if YOGA
// There are several cases where Yoga could arrive here:
Expand All @@ -1138,11 +1135,13 @@ - (ASLayout *)calculateLayoutThatFits:(ASSizeRange)constrainedSize
if ([self shouldHaveYogaMeasureFunc] == NO) {
// If we're a yoga root, tree node, or leaf with no measure func (e.g. spacer), then
// initiate a new Yoga calculation pass from root.
ASDN::MutexUnlocker ul(__instanceLock__);

as_activity_create_for_scope("Yoga layout calculation");
if (self.yogaLayoutInProgress == NO) {
ASYogaLog("Calculating yoga layout from root %@, %@", self, NSStringFromASSizeRange(constrainedSize));
l.unlock();
[self calculateLayoutFromYogaRoot:constrainedSize];
l.lock();
} else {
ASYogaLog("Reusing existing yoga layout %@", _yogaCalculatedLayout);
}
Expand Down Expand Up @@ -3546,15 +3545,15 @@ - (void)applyPendingViewState
ASDisplayNodeAssertMainThread();
ASAssertUnlocked(__instanceLock__);

ASDN::MutexLocker l(__instanceLock__);
ASDN::UniqueLock l(__instanceLock__);
// FIXME: Ideally we'd call this as soon as the node receives -setNeedsLayout
// but automatic subnode management would require us to modify the node tree
// in the background on a loaded node, which isn't currently supported.
if (_pendingViewState.hasSetNeedsLayout) {
// Need to unlock before calling setNeedsLayout to avoid deadlocks.
// MutexUnlocker will re-lock at the end of scope.
ASDN::MutexUnlocker u(__instanceLock__);
l.unlock();
[self __setNeedsLayout];
l.lock();
}

[self _locked_applyPendingViewState];
Expand Down
6 changes: 3 additions & 3 deletions Source/ASImageNode.mm
Original file line number Diff line number Diff line change
Expand Up @@ -434,12 +434,12 @@ + (UIImage *)displayWithParameters:(id<NSObject>)parameter isCancelled:(NS_NOESC

static ASWeakMap<ASImageNodeContentsKey *, UIImage *> *cache = nil;
// Allocate cacheLock on the heap to prevent destruction at app exit (https://github.com/TextureGroup/Texture/issues/136)
static ASDN::StaticMutex& cacheLock = *new ASDN::StaticMutex;
static auto *cacheLock = new ASDN::Mutex;

+ (ASWeakMapEntry *)contentsForkey:(ASImageNodeContentsKey *)key drawParameters:(id)drawParameters isCancelled:(asdisplaynode_iscancelled_block_t)isCancelled
{
{
ASDN::StaticMutexLocker l(cacheLock);
ASDN::MutexLocker l(*cacheLock);
if (!cache) {
cache = [[ASWeakMap alloc] init];
}
Expand All @@ -456,7 +456,7 @@ + (ASWeakMapEntry *)contentsForkey:(ASImageNodeContentsKey *)key drawParameters:
}

{
ASDN::StaticMutexLocker l(cacheLock);
ASDN::MutexLocker l(*cacheLock);
return [cache setObject:contents forKey:key];
}
}
Expand Down
6 changes: 3 additions & 3 deletions Source/ASTextNode2.mm
Original file line number Diff line number Diff line change
Expand Up @@ -55,14 +55,14 @@ @implementation ASTextCacheValue
*/
static NS_RETURNS_RETAINED ASTextLayout *ASTextNodeCompatibleLayoutWithContainerAndText(ASTextContainer *container, NSAttributedString *text) {
// Allocate layoutCacheLock on the heap to prevent destruction at app exit (https://github.com/TextureGroup/Texture/issues/136)
static ASDN::StaticMutex& layoutCacheLock = *new ASDN::StaticMutex;
static auto *layoutCacheLock = new ASDN::Mutex;
static NSCache<NSAttributedString *, ASTextCacheValue *> *textLayoutCache;
static dispatch_once_t onceToken;
dispatch_once(&onceToken, ^{
textLayoutCache = [[NSCache alloc] init];
});

layoutCacheLock.lock();
layoutCacheLock->lock();

ASTextCacheValue *cacheValue = [textLayoutCache objectForKey:text];
if (cacheValue == nil) {
Expand All @@ -72,7 +72,7 @@ @implementation ASTextCacheValue

// Lock the cache item for the rest of the method. Only after acquiring can we release the NSCache.
ASDN::MutexLocker lock(cacheValue->_m);
layoutCacheLock.unlock();
layoutCacheLock->unlock();

CGRect containerBounds = (CGRect){ .size = container.size };
{
Expand Down
12 changes: 9 additions & 3 deletions Source/ASVideoNode.mm
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#import <AVFoundation/AVFoundation.h>
#import <AsyncDisplayKit/ASDisplayNode+FrameworkPrivate.h>
#import <AsyncDisplayKit/ASDisplayNode+Subclasses.h>
#import <AsyncDisplayKit/ASDisplayNodeInternal.h>
#import <AsyncDisplayKit/ASVideoNode.h>
#import <AsyncDisplayKit/ASEqualityHelpers.h>
#import <AsyncDisplayKit/ASInternalHelpers.h>
Expand Down Expand Up @@ -322,16 +323,17 @@ - (void)setVideoPlaceholderImage:(UIImage *)image

- (void)observeValueForKeyPath:(NSString *)keyPath ofObject:(id)object change:(NSDictionary *)change context:(void *)context
{
ASLockScopeSelf();
ASDN::UniqueLock l(__instanceLock__);

if (object == _currentPlayerItem) {
if ([keyPath isEqualToString:kStatus]) {
if ([change[NSKeyValueChangeNewKey] integerValue] == AVPlayerItemStatusReadyToPlay) {
if (self.playerState != ASVideoNodePlayerStatePlaying) {
self.playerState = ASVideoNodePlayerStateReadyToPlay;
if (_shouldBePlaying && ASInterfaceStateIncludesVisible(self.interfaceState)) {
ASUnlockScope(self);
l.unlock();
[self play];
l.lock();
}
}
// If we don't yet have a placeholder image update it now that we should have data available for it
Expand All @@ -354,8 +356,10 @@ - (void)observeValueForKeyPath:(NSString *)keyPath ofObject:(id)object change:(N
[self.delegate videoNodeDidRecoverFromStall:self];
}

ASUnlockScope(self);
l.unlock();
[self play]; // autoresume after buffer catches up
// NOTE: Early return without re-locking.
return;
}
} else if ([keyPath isEqualToString:kplaybackBufferEmpty]) {
if (_shouldBePlaying && [change[NSKeyValueChangeNewKey] boolValue] == YES && ASInterfaceStateIncludesVisible(self.interfaceState)) {
Expand All @@ -373,6 +377,8 @@ - (void)observeValueForKeyPath:(NSString *)keyPath ofObject:(id)object change:(N
}
}
}

// NOTE: Early return above.
}

- (void)tapped
Expand Down
6 changes: 3 additions & 3 deletions Source/Details/ASBasicImageDownloader.mm
Original file line number Diff line number Diff line change
Expand Up @@ -39,11 +39,11 @@ @implementation ASBasicImageDownloaderContext

static NSMutableDictionary *currentRequests = nil;
// Allocate currentRequestsLock on the heap to prevent destruction at app exit (https://github.com/TextureGroup/Texture/issues/136)
static ASDN::StaticMutex& currentRequestsLock = *new ASDN::StaticMutex;
static auto *currentRequestsLock = new ASDN::Mutex;

+ (ASBasicImageDownloaderContext *)contextForURL:(NSURL *)URL
{
ASDN::StaticMutexLocker l(currentRequestsLock);
ASDN::MutexLocker l(*currentRequestsLock);
if (!currentRequests) {
currentRequests = [[NSMutableDictionary alloc] init];
}
Expand All @@ -57,7 +57,7 @@ + (ASBasicImageDownloaderContext *)contextForURL:(NSURL *)URL

+ (void)cancelContextWithURL:(NSURL *)URL
{
ASDN::StaticMutexLocker l(currentRequestsLock);
ASDN::MutexLocker l(*currentRequestsLock);
if (currentRequests) {
[currentRequests removeObjectForKey:URL];
}
Expand Down
11 changes: 7 additions & 4 deletions Source/Details/ASMainSerialQueue.mm
Original file line number Diff line number Diff line change
Expand Up @@ -40,19 +40,21 @@ - (NSUInteger)numberOfScheduledBlocks

- (void)performBlockOnMainThread:(dispatch_block_t)block
{
ASDN::MutexLocker l(_serialQueueLock);

ASDN::UniqueLock l(_serialQueueLock);
[_blocks addObject:block];
{
ASDN::MutexUnlocker u(_serialQueueLock);
l.unlock();
[self runBlocks];
l.lock();
}
}

- (void)runBlocks
{
dispatch_block_t mainThread = ^{
ASDN::UniqueLock l(self->_serialQueueLock);
do {
ASDN::MutexLocker l(self->_serialQueueLock);
dispatch_block_t block;
if (self->_blocks.count > 0) {
block = _blocks[0];
Expand All @@ -61,8 +63,9 @@ - (void)runBlocks
break;
}
{
ASDN::MutexUnlocker u(self->_serialQueueLock);
l.unlock();
block();
l.lock();
}
} while (true);
};
Expand Down
Loading