From 63ec2560cecc1fb8284388e97b6ac7431b88e0b4 Mon Sep 17 00:00:00 2001 From: Adlai Holler Date: Tue, 31 Oct 2017 15:59:13 -0700 Subject: [PATCH 1/5] Make our async deallocation functions take a double pointer, so we can be sure we've released before the queue drains --- CHANGELOG.md | 1 + Source/ASCollectionView.mm | 4 ++-- Source/ASDisplayNode.mm | 2 +- Source/ASDisplayNodeExtras.h | 2 +- Source/ASDisplayNodeExtras.mm | 13 +++++++++---- Source/ASImageNode.mm | 2 +- Source/ASMultiplexImageNode.mm | 4 ++-- Source/ASRunLoopQueue.h | 6 +++--- Source/ASRunLoopQueue.mm | 23 +++++++++++++++++++---- Source/ASTableView.mm | 4 ++-- Source/ASViewController.mm | 2 +- Source/Private/ASInternalHelpers.h | 2 +- Source/Private/ASInternalHelpers.m | 2 +- 13 files changed, 44 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6f0a0b1ff..784e6fb3a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ - [ASCollectionView] Check if batch fetching is needed if batch fetching parameter has been changed. [#624](https://github.com/TextureGroup/Texture/pull/624) [Garrett Moon](https://github.com/garrettmoon) - [ASNetworkImageNode] New delegate callback to tell the consumer whether the image was loaded from cache or download. [Adlai Holler](https://github.com/Adlai-Holler) - [Layout] Fixes a deadlock in layout. [#638](https://github.com/TextureGroup/Texture/pull/638) [Garrett Moon](https://github.com/garrettmoon) +- Fixed cases where main-thread-deallocation doesn't work as expected [Adlai Holler](https://github.com/Adlai-Holler) ## 2.6 - [Xcode 9] Updated to require Xcode 9 (to fix warnings) [Garrett Moon](https://github.com/garrettmoon) diff --git a/Source/ASCollectionView.mm b/Source/ASCollectionView.mm index 339728d83..d3857ae45 100644 --- a/Source/ASCollectionView.mm +++ b/Source/ASCollectionView.mm @@ -332,8 +332,8 @@ - (void)dealloc [self setAsyncDataSource:nil]; // Data controller & range controller may own a ton of nodes, let's deallocate those off-main. - ASPerformBackgroundDeallocation(_dataController); - ASPerformBackgroundDeallocation(_rangeController); + ASPerformBackgroundDeallocation(&_dataController); + ASPerformBackgroundDeallocation(&_rangeController); } #pragma mark - diff --git a/Source/ASDisplayNode.mm b/Source/ASDisplayNode.mm index 3ef5f3629..c5bdd8832 100644 --- a/Source/ASDisplayNode.mm +++ b/Source/ASDisplayNode.mm @@ -440,7 +440,7 @@ - (void)_scheduleIvarsForMainDeallocation // is still deallocated on a background thread object_setIvar(self, ivar, nil); - ASPerformMainThreadDeallocation(value); + ASPerformMainThreadDeallocation(&value); } else { as_log_debug(ASMainThreadDeallocationLog(), "%@: Not trampolining ivar '%s' value %@.", self, ivar_getName(ivar), value); } diff --git a/Source/ASDisplayNodeExtras.h b/Source/ASDisplayNodeExtras.h index 923f8b72f..ba0a38172 100644 --- a/Source/ASDisplayNodeExtras.h +++ b/Source/ASDisplayNodeExtras.h @@ -35,7 +35,7 @@ #endif /// For deallocation of objects on the main thread across multiple run loops. -extern void ASPerformMainThreadDeallocation(_Nullable id object); +extern void ASPerformMainThreadDeallocation(id _Nullable __strong * _Nonnull objectPtr); // Because inline methods can't be extern'd and need to be part of the translation unit of code // that compiles with them to actually inline, we both declare and define these in the header. diff --git a/Source/ASDisplayNodeExtras.mm b/Source/ASDisplayNodeExtras.mm index 279687662..2839e1a6c 100644 --- a/Source/ASDisplayNodeExtras.mm +++ b/Source/ASDisplayNodeExtras.mm @@ -23,8 +23,7 @@ #import #import -extern void ASPerformMainThreadDeallocation(_Nullable id object) -{ +extern void ASPerformMainThreadDeallocation(id _Nullable __strong * _Nonnull objectPtr) { /** * UIKit components must be deallocated on the main thread. We use this shared * run loop queue to gradually deallocate them across many turns of the main run loop. @@ -35,8 +34,14 @@ extern void ASPerformMainThreadDeallocation(_Nullable id object) queue = [[ASRunLoopQueue alloc] initWithRunLoop:CFRunLoopGetMain() retainObjects:YES handler:nil]; queue.batchSize = 10; }); - if (object != nil) { - [queue enqueue:object]; + + if (objectPtr != NULL && *objectPtr != nil) { + // Lock queue while enqueuing and releasing, so that there's no risk + // that the queue will release before we get a chance to release. + [queue lock]; + [queue enqueue:*objectPtr]; // Retain, +1 + *objectPtr = nil; // Release, +0 + [queue unlock]; // (After queue drains), release, -1 } } diff --git a/Source/ASImageNode.mm b/Source/ASImageNode.mm index df39741b0..5e2383081 100644 --- a/Source/ASImageNode.mm +++ b/Source/ASImageNode.mm @@ -278,7 +278,7 @@ - (void)_locked_setImage:(UIImage *)image BOOL shouldReleaseImageOnBackgroundThread = oldImageSize.width > kMinReleaseImageOnBackgroundSize.width || oldImageSize.height > kMinReleaseImageOnBackgroundSize.height; if (shouldReleaseImageOnBackgroundThread) { - ASPerformBackgroundDeallocation(oldImage); + ASPerformBackgroundDeallocation(&oldImage); } } diff --git a/Source/ASMultiplexImageNode.mm b/Source/ASMultiplexImageNode.mm index 81f3f43c1..dc689d829 100644 --- a/Source/ASMultiplexImageNode.mm +++ b/Source/ASMultiplexImageNode.mm @@ -532,10 +532,10 @@ - (void)_clearImage CGSize imageSize = image.size; BOOL shouldReleaseImageOnBackgroundThread = imageSize.width > kMinReleaseImageOnBackgroundSize.width || imageSize.height > kMinReleaseImageOnBackgroundSize.height; + [self _setImage:nil]; if (shouldReleaseImageOnBackgroundThread) { - ASPerformBackgroundDeallocation(image); + ASPerformBackgroundDeallocation(&image); } - [self _setImage:nil]; } #pragma mark - diff --git a/Source/ASRunLoopQueue.h b/Source/ASRunLoopQueue.h index 6b08540cc..0d5e2173b 100644 --- a/Source/ASRunLoopQueue.h +++ b/Source/ASRunLoopQueue.h @@ -21,7 +21,7 @@ NS_ASSUME_NONNULL_BEGIN AS_SUBCLASSING_RESTRICTED -@interface ASRunLoopQueue : NSObject +@interface ASRunLoopQueue : NSObject /** * Create a new queue with the given run loop and handler. @@ -51,11 +51,11 @@ AS_SUBCLASSING_RESTRICTED AS_SUBCLASSING_RESTRICTED @interface ASDeallocQueue : NSObject -+ (instancetype)sharedDeallocationQueue; ++ (ASDeallocQueue *)sharedDeallocationQueue; - (void)test_drain; -- (void)releaseObjectInBackground:(id)object; +- (void)releaseObjectInBackground:(id __strong _Nullable * _Nonnull)objectPtr; @end diff --git a/Source/ASRunLoopQueue.mm b/Source/ASRunLoopQueue.mm index 1902ad23d..69c3dc520 100644 --- a/Source/ASRunLoopQueue.mm +++ b/Source/ASRunLoopQueue.mm @@ -55,16 +55,19 @@ + (instancetype)sharedDeallocationQueue return deallocQueue; } -- (void)releaseObjectInBackground:(id)object +- (void)releaseObjectInBackground:(id _Nullable __strong *)objectPtr { // Disable background deallocation on iOS 8 and below to avoid crashes related to UIAXDelegateClearer (#2767). if (!AS_AT_LEAST_IOS9) { return; } - _queueLock.lock(); - _queue.push_back(object); - _queueLock.unlock(); + if (objectPtr != NULL && *objectPtr != nil) { + _queueLock.lock(); + _queue.push_back(*objectPtr); + *objectPtr = nil; + _queueLock.unlock(); + } } - (void)threadMain @@ -454,4 +457,16 @@ - (BOOL)isEmpty return _internalQueue.count == 0; } +#pragma mark - NSLocking + +- (void)lock +{ + _internalQueueLock.lock(); +} + +- (void)unlock +{ + _internalQueueLock.unlock(); +} + @end diff --git a/Source/ASTableView.mm b/Source/ASTableView.mm index 32e497977..594d6a996 100644 --- a/Source/ASTableView.mm +++ b/Source/ASTableView.mm @@ -372,8 +372,8 @@ - (void)dealloc [self setAsyncDataSource:nil]; // Data controller & range controller may own a ton of nodes, let's deallocate those off-main - ASPerformBackgroundDeallocation(_dataController); - ASPerformBackgroundDeallocation(_rangeController); + ASPerformBackgroundDeallocation(&_dataController); + ASPerformBackgroundDeallocation(&_rangeController); } #pragma mark - diff --git a/Source/ASViewController.mm b/Source/ASViewController.mm index 2826fae9d..4e1e57c3a 100644 --- a/Source/ASViewController.mm +++ b/Source/ASViewController.mm @@ -96,7 +96,7 @@ - (void)_initializeInstance - (void)dealloc { - ASPerformBackgroundDeallocation(_node); + ASPerformBackgroundDeallocation(&_node); } - (void)loadView diff --git a/Source/Private/ASInternalHelpers.h b/Source/Private/ASInternalHelpers.h index 90a525ef8..dc67700e1 100644 --- a/Source/Private/ASInternalHelpers.h +++ b/Source/Private/ASInternalHelpers.h @@ -38,7 +38,7 @@ void ASPerformBlockOnMainThread(void (^block)(void)); void ASPerformBlockOnBackgroundThread(void (^block)(void)); // DISPATCH_QUEUE_PRIORITY_DEFAULT /// For deallocation of objects on a background thread without GCD overhead / thread explosion -void ASPerformBackgroundDeallocation(id object); +void ASPerformBackgroundDeallocation(id __strong _Nullable * _Nonnull object); CGFloat ASScreenScale(void); diff --git a/Source/Private/ASInternalHelpers.m b/Source/Private/ASInternalHelpers.m index 8bc865eb3..ad55f295a 100644 --- a/Source/Private/ASInternalHelpers.m +++ b/Source/Private/ASInternalHelpers.m @@ -84,7 +84,7 @@ void ASPerformBlockOnBackgroundThread(void (^block)(void)) } } -void ASPerformBackgroundDeallocation(id object) +void ASPerformBackgroundDeallocation(id __strong _Nullable * _Nonnull object) { [[ASDeallocQueue sharedDeallocationQueue] releaseObjectInBackground:object]; } From 1d28667d7f1e977919803c911b08d7456293a1fd Mon Sep 17 00:00:00 2001 From: Adlai Holler Date: Tue, 31 Oct 2017 16:06:36 -0700 Subject: [PATCH 2/5] Make it a class property --- Source/ASRunLoopQueue.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Source/ASRunLoopQueue.h b/Source/ASRunLoopQueue.h index 0d5e2173b..50a0eeb24 100644 --- a/Source/ASRunLoopQueue.h +++ b/Source/ASRunLoopQueue.h @@ -51,7 +51,7 @@ AS_SUBCLASSING_RESTRICTED AS_SUBCLASSING_RESTRICTED @interface ASDeallocQueue : NSObject -+ (ASDeallocQueue *)sharedDeallocationQueue; +@property (class, atomic, readonly) ASDeallocQueue *sharedDeallocationQueue; - (void)test_drain; From 71b7556422c94e36359fbab52c6635ef7dd80b0c Mon Sep 17 00:00:00 2001 From: Adlai Holler Date: Tue, 31 Oct 2017 16:06:54 -0700 Subject: [PATCH 3/5] Fix the return type --- Source/ASRunLoopQueue.mm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Source/ASRunLoopQueue.mm b/Source/ASRunLoopQueue.mm index 69c3dc520..8747d7ee4 100644 --- a/Source/ASRunLoopQueue.mm +++ b/Source/ASRunLoopQueue.mm @@ -45,7 +45,7 @@ @implementation ASDeallocQueue { ASDN::RecursiveMutex _queueLock; } -+ (instancetype)sharedDeallocationQueue ++ (ASDeallocQueue *)sharedDeallocationQueue { static ASDeallocQueue *deallocQueue = nil; static dispatch_once_t onceToken; From cd6ebcb4844b9875241dcc7c22ef040f7684e643 Mon Sep 17 00:00:00 2001 From: Adlai Holler Date: Tue, 31 Oct 2017 16:07:55 -0700 Subject: [PATCH 4/5] Use a locker --- Source/ASRunLoopQueue.mm | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Source/ASRunLoopQueue.mm b/Source/ASRunLoopQueue.mm index 8747d7ee4..ced798252 100644 --- a/Source/ASRunLoopQueue.mm +++ b/Source/ASRunLoopQueue.mm @@ -63,10 +63,9 @@ - (void)releaseObjectInBackground:(id _Nullable __strong *)objectPtr } if (objectPtr != NULL && *objectPtr != nil) { - _queueLock.lock(); + ASDN::MutexLocker l(_queueLock); _queue.push_back(*objectPtr); *objectPtr = nil; - _queueLock.unlock(); } } From a1c61ad16e5528ebcf1f7a34a24c9a0e8878deef Mon Sep 17 00:00:00 2001 From: Adlai Holler Date: Thu, 2 Nov 2017 10:44:26 -0700 Subject: [PATCH 5/5] Improve release notes --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index facf3b696..e3e86c4e1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,7 +7,7 @@ - [ASNetworkImageNode] New delegate callback to tell the consumer whether the image was loaded from cache or download. [Adlai Holler](https://github.com/Adlai-Holler) - [Layout] Fixes a deadlock in layout. [#638](https://github.com/TextureGroup/Texture/pull/638) [Garrett Moon](https://github.com/garrettmoon) - Updated to be backwards compatible with Xcode 8. [Adlai Holler](https://github.com/Adlai-Holler) -- Fixed cases where main-thread-deallocation doesn't work as expected [Adlai Holler](https://github.com/Adlai-Holler) +- [API CHANGES] `ASPerformMainThreadDeallocation` and `ASPerformBackgroundDeallocation` functions take `id *` instead of `id` and they're now more reliable. Also, in Swift, `ASDeallocQueue.sharedDeallocationQueue() -> ASDeallocQueue.sharedDeallocationQueue`. [Adlai Holler](https://github.com/Adlai-Holler) [#651](https://github.com/TextureGroup/Texture/pull/651) ## 2.6 - [Xcode 9] Updated to require Xcode 9 (to fix warnings) [Garrett Moon](https://github.com/garrettmoon)