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.

Sign up for GitHub

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

Jump to bottom

Clean Up ASAsyncTransaction #trivial #316

Merged
merged 1 commit into from
May 29, 2017
Merged

Clean Up ASAsyncTransaction #trivial #316

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
14 changes: 3 additions & 11 deletions Source/Details/Transactions/_ASAsyncTransaction.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ NS_ASSUME_NONNULL_BEGIN

typedef void(^asyncdisplaykit_async_transaction_completion_block_t)(_ASAsyncTransaction *completedTransaction, BOOL canceled);
typedef id<NSObject> _Nullable(^asyncdisplaykit_async_transaction_operation_block_t)(void);
typedef void(^asyncdisplaykit_async_transaction_operation_completion_block_t)(id<NSObject> _Nullable value, BOOL canceled);
typedef void(^asyncdisplaykit_async_transaction_complete_async_operation_block_t)(id<NSObject> _Nullable value);
typedef void(^asyncdisplaykit_async_transaction_operation_completion_block_t)(id _Nullable value, BOOL canceled);
typedef void(^asyncdisplaykit_async_transaction_complete_async_operation_block_t)(id _Nullable value);
typedef void(^asyncdisplaykit_async_transaction_async_operation_block_t)(asyncdisplaykit_async_transaction_complete_async_operation_block_t completeOperationBlock);

/**
Expand Down Expand Up @@ -62,12 +62,9 @@ extern NSInteger const ASDefaultTransactionPriority;
/**
@summary Initialize a transaction that can start collecting async operations.

@see initWithCallbackQueue:commitBlock:completionBlock:executeConcurrently:
@param callbackQueue The dispatch queue that the completion blocks will be called on. Default is the main queue.
@param completionBlock A block that is called when the transaction is completed.
*/
- (instancetype)initWithCallbackQueue:(nullable dispatch_queue_t)callbackQueue
completionBlock:(nullable asyncdisplaykit_async_transaction_completion_block_t)completionBlock;
- (instancetype)initWithCompletionBlock:(nullable asyncdisplaykit_async_transaction_completion_block_t)completionBlock;

/**
@summary Block the main thread until the transaction is complete, including callbacks.
Expand All @@ -76,11 +73,6 @@ extern NSInteger const ASDefaultTransactionPriority;
*/
- (void)waitUntilComplete;

/**
The dispatch queue that the completion blocks will be called on.
*/
@property (nonatomic, readonly, strong) dispatch_queue_t callbackQueue;

/**
A block that is called when the transaction is completed.
*/
Expand Down
49 changes: 17 additions & 32 deletions Source/Details/Transactions/_ASAsyncTransaction.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
@interface ASAsyncTransactionOperation : NSObject
- (instancetype)initWithOperationCompletionBlock:(asyncdisplaykit_async_transaction_operation_completion_block_t)operationCompletionBlock;
@property (nonatomic, copy) asyncdisplaykit_async_transaction_operation_completion_block_t operationCompletionBlock;
@property (nonatomic, strong) id<NSObject> value; // set on bg queue by the operation block
@property (atomic, strong) id value; // set on bg queue by the operation block
@end

@implementation ASAsyncTransactionOperation
Expand All @@ -55,16 +55,17 @@ - (void)dealloc

- (void)callAndReleaseCompletionBlock:(BOOL)canceled;
{
ASDisplayNodeAssertMainThread();
if (_operationCompletionBlock) {
_operationCompletionBlock(self.value, canceled);
// Guarantee that _operationCompletionBlock is released on _callbackQueue:
self.operationCompletionBlock = nil;
// Guarantee that _operationCompletionBlock is released on main thread
_operationCompletionBlock = nil;
}
}

- (NSString *)description
{
return [NSString stringWithFormat:@"<ASAsyncTransactionOperation: %p - value = %@", self, self.value];
return [NSString stringWithFormat:@"<ASAsyncTransactionOperation: %p - value = %@>", self, self.value];
}

@end
Expand Down Expand Up @@ -328,27 +329,25 @@ - (NSString *)description
return *instance;
}

@interface _ASAsyncTransaction ()
@property (atomic) ASAsyncTransactionState state;
@end


@implementation _ASAsyncTransaction
{
ASAsyncTransactionQueue::Group *_group;
NSMutableArray<ASAsyncTransactionOperation *> *_operations;
_Atomic(ASAsyncTransactionState) _state;
}

#pragma mark -
#pragma mark Lifecycle

- (instancetype)initWithCallbackQueue:(dispatch_queue_t)callbackQueue
completionBlock:(void(^)(_ASAsyncTransaction *, BOOL))completionBlock
- (instancetype)initWithCompletionBlock:(void(^)(_ASAsyncTransaction *, BOOL))completionBlock
{
if ((self = [self init])) {
if (callbackQueue == NULL) {
callbackQueue = dispatch_get_main_queue();
}
_callbackQueue = callbackQueue;
_completionBlock = completionBlock;

_state = ATOMIC_VAR_INIT(ASAsyncTransactionStateOpen);
self.state = ASAsyncTransactionStateOpen;
}
return self;
}
Expand All @@ -362,18 +361,6 @@ - (void)dealloc
}
}

#pragma mark - Properties

- (ASAsyncTransactionState)state
{
return atomic_load(&_state);
}

- (void)setState:(ASAsyncTransactionState)state
{
atomic_store(&_state, state);
}

#pragma mark - Transaction Management

- (void)addAsyncOperationWithBlock:(asyncdisplaykit_async_transaction_async_operation_block_t)block
Expand Down Expand Up @@ -402,7 +389,7 @@ - (void)addAsyncOperationWithBlock:(asyncdisplaykit_async_transaction_async_oper
@autoreleasepool {
if (self.state != ASAsyncTransactionStateCanceled) {
_group->enter();
block(^(id<NSObject> value){
block(^(id value){
operation.value = value;
_group->leave();
});
Expand Down Expand Up @@ -445,7 +432,8 @@ - (void)addOperationWithBlock:(asyncdisplaykit_async_transaction_operation_block
- (void)addCompletionBlock:(asyncdisplaykit_async_transaction_completion_block_t)completion
{
__weak __typeof__(self) weakSelf = self;
[self addOperationWithBlock:^(){return (id<NSObject>)nil;} queue:_callbackQueue completion:^(id<NSObject> value, BOOL canceled) {
dispatch_queue_t bsQueue = dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wtf does bsQueue mean? lol

Either way, shouldn't this operation be called on the main queue, if _callbackQueue used to be main? This seems like it might be a behavior change.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bsQueue means "bullshit queue." The completion will be called on main still, but the bs "work" that we submitted should be done on another queue. If we try to do the work on the main queue, the transaction won't be safe to wait on (the group will get stuck.)

This method isn't currently used and I'd be glad to remove it. Thoughts?

[self addOperationWithBlock:^(){return (id)nil;} queue:bsQueue completion:^(id value, BOOL canceled) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I bet the (id)nil cast isn't needed anymore.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Surprisingly it is! The compiler inferred nullptr instead of (id)nil when I just put nil. Bizarre I know =/

__typeof__(self) strongSelf = weakSelf;
completion(strongSelf, canceled);
}];
Expand All @@ -472,17 +460,15 @@ - (void)commit
} else {
NSAssert(_group != NULL, @"If there are operations, dispatch group should have been created");

_group->notify(_callbackQueue, ^{
// _callbackQueue is the main queue in current practice (also asserted in -waitUntilComplete).
// This code should be reviewed before taking on significantly different use cases.
ASAsyncTransactionAssertMainThread();
_group->notify(dispatch_get_main_queue(), ^{
[self completeTransaction];
});
}
}

- (void)completeTransaction
{
ASAsyncTransactionAssertMainThread();
ASAsyncTransactionState state = self.state;
if (state != ASAsyncTransactionStateComplete) {
BOOL isCanceled = (state == ASAsyncTransactionStateCanceled);
Expand All @@ -506,7 +492,6 @@ - (void)waitUntilComplete
ASAsyncTransactionAssertMainThread();
if (self.state != ASAsyncTransactionStateComplete) {
if (_group) {
NSAssert(_callbackQueue == dispatch_get_main_queue(), nil);
_group->wait();

// At this point, the asynchronous operation may have completed, but the runloop
Expand Down
4 changes: 2 additions & 2 deletions Source/Details/Transactions/_ASAsyncTransactionContainer.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,11 +93,11 @@ - (_ASAsyncTransaction *)asyncdisplaykit_asyncTransaction
if (transaction == nil) {
NSHashTable *transactions = self.asyncdisplaykit_asyncLayerTransactions;
if (transactions == nil) {
transactions = [NSHashTable hashTableWithOptions:NSPointerFunctionsObjectPointerPersonality];
transactions = [NSHashTable hashTableWithOptions:NSHashTableObjectPointerPersonality];
self.asyncdisplaykit_asyncLayerTransactions = transactions;
}
__weak CALayer *weakSelf = self;
transaction = [[_ASAsyncTransaction alloc] initWithCallbackQueue:dispatch_get_main_queue() completionBlock:^(_ASAsyncTransaction *completedTransaction, BOOL cancelled) {
transaction = [[_ASAsyncTransaction alloc] initWithCompletionBlock:^(_ASAsyncTransaction *completedTransaction, BOOL cancelled) {
__strong CALayer *self = weakSelf;
if (self == nil) {
return;
Expand Down
30 changes: 8 additions & 22 deletions Source/Details/Transactions/_ASAsyncTransactionGroup.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,6 @@
#import <AsyncDisplayKit/_ASAsyncTransactionContainer.h>
#import <AsyncDisplayKit/_ASAsyncTransactionContainer+Private.h>

static void _transactionGroupRunLoopObserverCallback(CFRunLoopObserverRef observer, CFRunLoopActivity activity, void *info);

@interface _ASAsyncTransactionGroup ()
+ (void)registerTransactionGroupAsMainRunloopObserver:(_ASAsyncTransactionGroup *)transactionGroup;
- (void)commit;
Expand Down Expand Up @@ -54,20 +52,15 @@ + (void)registerTransactionGroupAsMainRunloopObserver:(_ASAsyncTransactionGroup
CFRunLoopRef runLoop = CFRunLoopGetCurrent();
CFOptionFlags activities = (kCFRunLoopBeforeWaiting | // before the run loop starts sleeping
kCFRunLoopExit); // before exiting a runloop run
CFRunLoopObserverContext context = {
0, // version
(__bridge void *)transactionGroup, // info
&CFRetain, // retain
&CFRelease, // release
NULL // copyDescription
};

observer = CFRunLoopObserverCreate(NULL, // allocator
activities, // activities
YES, // repeats
INT_MAX, // order after CA transaction commits
&_transactionGroupRunLoopObserverCallback, // callback
&context); // context
observer = CFRunLoopObserverCreateWithHandler(NULL, // allocator
activities, // activities
YES, // repeats
INT_MAX, // order after CA transaction commits
^(CFRunLoopObserverRef observer, CFRunLoopActivity activity) {
ASDisplayNodeCAssertMainThread();
[transactionGroup commit];
});
CFRunLoopAddObserver(runLoop, observer, kCFRunLoopCommonModes);
CFRelease(observer);
}
Expand Down Expand Up @@ -111,10 +104,3 @@ + (void)commit
}

@end

static void _transactionGroupRunLoopObserverCallback(CFRunLoopObserverRef observer, CFRunLoopActivity activity, void *info)
{
ASDisplayNodeCAssertMainThread();
_ASAsyncTransactionGroup *group = (__bridge _ASAsyncTransactionGroup *)info;
[group commit];
}