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

New runloop queue to coalesce Interface state update calls. #788

Merged
merged 28 commits into from
Feb 13, 2018
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
929d118
fix SIMULATE_WEB_RESPONSE not imported #449
wsdwsd0829 Jul 16, 2017
dd24d8f
Merge branch 'master' of github.com:TextureGroup/Texture
wsdwsd0829 Sep 6, 2017
b8eaffa
Merge branch 'master' of github.com:TextureGroup/Texture
wsdwsd0829 Oct 4, 2017
2918ea0
Merge branch 'master' of github.com:TextureGroup/Texture
wsdwsd0829 Oct 11, 2017
9c42266
Merge branch 'master' of github.com:TextureGroup/Texture
wsdwsd0829 Oct 12, 2017
3529cc7
Merge branch 'master' of github.com:TextureGroup/Texture
wsdwsd0829 Jan 10, 2018
ee44725
Coalesce interface state updates to ASCATransactionQueue before CATra…
wsdwsd0829 Jan 10, 2018
cdd2e9b
fix tests for new run loop queue
wsdwsd0829 Jan 12, 2018
e99abf7
Support for disabling ASCATransactionQueue
wsdwsd0829 Feb 2, 2018
6f2ba8e
Fix didExitHierarchy to use ASCATransactionQueue.
wsdwsd0829 Feb 2, 2018
f807efa
merge range managed and none range managed for didExitHierarchy
wsdwsd0829 Feb 5, 2018
bd8cf01
Revert "merge range managed and none range managed for didExitHierarchy"
wsdwsd0829 Feb 5, 2018
7306dc5
merge range managed and none range managed for didExitHierarchy
wsdwsd0829 Feb 5, 2018
cfd083a
remove metadata
wsdwsd0829 Feb 5, 2018
1ec63b6
abstract queue to impl class methods
wsdwsd0829 Feb 5, 2018
58effec
Add tests
wsdwsd0829 Feb 5, 2018
cace535
Fix test fail because of shared object.
wsdwsd0829 Feb 5, 2018
0631372
guard _pendingInterfaceState access with lock
wsdwsd0829 Feb 5, 2018
f3c8507
name refactor
wsdwsd0829 Feb 6, 2018
e2c7d06
Refactor from comments https://github.com/TextureGroup/Texture/pull/7…
wsdwsd0829 Feb 8, 2018
7556ed7
Apply InterfaceState immediately after ASCATranactionQueue is process…
wsdwsd0829 Feb 9, 2018
0904422
refactor
wsdwsd0829 Feb 9, 2018
786ef37
merge master from up stream
wsdwsd0829 Feb 9, 2018
1efd5e8
no op to start CI build
wsdwsd0829 Feb 11, 2018
632c69f
remove unused var and kick off tests
wsdwsd0829 Feb 11, 2018
9dad5a5
change lisence
wsdwsd0829 Feb 11, 2018
a6a7e0a
remove code for weak ref
wsdwsd0829 Feb 13, 2018
19bdef2
add change log and adjust license
wsdwsd0829 Feb 13, 2018
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
92 changes: 63 additions & 29 deletions Source/ASDisplayNode.mm
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@
// We have to forward declare the protocol as this place otherwise it will not compile compiling with an Base SDK < iOS 10
@protocol CALayerDelegate;

@interface ASDisplayNode () <UIGestureRecognizerDelegate, CALayerDelegate, _ASDisplayLayerDelegate>
@interface ASDisplayNode () <UIGestureRecognizerDelegate, CALayerDelegate, _ASDisplayLayerDelegate, ASCATransactionQueueObserving>

/**
* See ASDisplayNodeInternal.h for ivars
Expand Down Expand Up @@ -2741,7 +2741,9 @@ - (void)setHierarchyState:(ASHierarchyState)newState
// Entered or exited range managed state.
if ((newState & ASHierarchyStateRangeManaged) != (oldState & ASHierarchyStateRangeManaged)) {
if (newState & ASHierarchyStateRangeManaged) {
[self enterInterfaceState:self.supernode.interfaceState];
if (self.supernode) {
[self enterInterfaceState:self.supernode->_pendingInterfaceState];
Copy link
Member

@appleguy appleguy Feb 5, 2018

Choose a reason for hiding this comment

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

@wsdwsd0829 In order to do this, we would need to grab the instanceLock of the supernode, which shouldn't be done manually.

To correctly fix this case, we'll need to add an accessor method for - (ASInterfaceState)pendingInterfaceState which uses a MutexLocker and returns the _pendingInterfaceState.

This method can be declared in ASDisplayNode+FrameworkPrivate.h, so it can be used by various internal subclasses without being made public right away (we may eventually want to make this public, but not until there is a use case — I'm optimistic that there will never be a need to do this).

}
} else {
// The case of exiting a range-managed state should be fairly rare. Adding or removing the node
// to a view hierarchy will cause its interfaceState to be either fully set or unset (all fields),
Expand Down Expand Up @@ -2784,30 +2786,34 @@ - (void)didExitHierarchy
ASDisplayNodeAssert(_flags.isExitingHierarchy, @"You should never call -didExitHierarchy directly. Appearance is automatically managed by ASDisplayNode");
ASDisplayNodeAssert(!_flags.isEnteringHierarchy, @"ASDisplayNode inconsistency. __enterHierarchy and __exitHierarchy are mutually exclusive");
ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock__);

if (![self supportsRangeManagedInterfaceState]) {
self.interfaceState = ASInterfaceStateNone;
} else {
// This case is important when tearing down hierarchies. We must deliver a visibileStateDidChange:NO callback, as part our API guarantee that this method can be used for
// things like data analytics about user content viewing. We cannot call the method in the dealloc as any incidental retain operations in client code would fail.
// Additionally, it may be that a Standard UIView which is containing us is moving between hierarchies, and we should not send the call if we will be re-added in the
// same runloop. Strategy: strong reference (might be the last!), wait one runloop, and confirm we are still outside the hierarchy (both layer-backed and view-backed).
// TODO: This approach could be optimized by only performing the dispatch for root elements + recursively apply the interface state change. This would require a closer
// integration with _ASDisplayLayer to ensure that the superlayer pointer has been cleared by this stage (to check if we are root or not), or a different delegate call.

if (ASInterfaceStateIncludesVisible(self.interfaceState)) {
dispatch_async(dispatch_get_main_queue(), ^{
// This block intentionally retains self.
__instanceLock__.lock();
unsigned isInHierarchy = _flags.isInHierarchy;
BOOL isVisible = ASInterfaceStateIncludesVisible(_interfaceState);
ASInterfaceState newState = (_interfaceState & ~ASInterfaceStateVisible);
__instanceLock__.unlock();

if (!isInHierarchy && isVisible) {
self.interfaceState = newState;

// This case is important when tearing down hierarchies. We must deliver a visibileStateDidChange:NO callback, as part our API guarantee that this method can be used for
// things like data analytics about user content viewing. We cannot call the method in the dealloc as any incidental retain operations in client code would fail.
// Additionally, it may be that a Standard UIView which is containing us is moving between hierarchies, and we should not send the call if we will be re-added in the
// same runloop. Strategy: strong reference (might be the last!), wait one runloop, and confirm we are still outside the hierarchy (both layer-backed and view-backed).
// TODO: This approach could be optimized by only performing the dispatch for root elements + recursively apply the interface state change. This would require a closer
// integration with _ASDisplayLayer to ensure that the superlayer pointer has been cleared by this stage (to check if we are root or not), or a different delegate call.
if (ASInterfaceStateIncludesVisible(_pendingInterfaceState)) {
void(^exitVisibleInterfaceState)(void) = ^{
// This block intentionally retains self.
__instanceLock__.lock();
unsigned isStillInHierarchy = _flags.isInHierarchy;
BOOL isVisible = ASInterfaceStateIncludesVisible(_pendingInterfaceState);
ASInterfaceState newState = (_pendingInterfaceState & ~ASInterfaceStateVisible);
__instanceLock__.unlock();

if (!isStillInHierarchy && isVisible) {
if (![self supportsRangeManagedInterfaceState]) {
newState = ASInterfaceStateNone;
}
});
self.interfaceState = newState;
}
};

if ([[ASCATransactionQueue sharedQueue] disabled]) {
dispatch_async(dispatch_get_main_queue(), exitVisibleInterfaceState);
Copy link
Member

@appleguy appleguy Feb 5, 2018

Choose a reason for hiding this comment

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

For reviewers: Note that this change intentionally alters the behavior of the -disabled state as well. After the change, the -didExitVisibleState call is held for a dispatch_async to confirm the node is still invisible, for both range-managed and not range-managed nodes (previously there was a check distinguishing these).

This will decrease the accuracy of the call slightly, compared to before the change and also compared to interfaceState coalescing which guarantees the call is done on the same runloop / implicit transaction.

However it will also address a very common issue where visibility calls thrash to invisible and visible twice during the UIViewController transition process, and this feels like a better / safer behavior to prefer rather than the utmost accuracy of the invisibility call.

The visibility call remains un-delayed, and that one is much more important. Gating the invisibility is sufficient to avoid the thrashing since the visibility call won't re-fire if the interfaceState remains visible.

} else {
exitVisibleInterfaceState();
}
}
}
Expand Down Expand Up @@ -2868,25 +2874,48 @@ - (ASInterfaceState)interfaceState
}

- (void)setInterfaceState:(ASInterfaceState)newState
{
ASDN::MutexLocker l(__instanceLock__);
if ([[ASCATransactionQueue sharedQueue] disabled]) {
[self applyPendingInterfaceState:newState];
} else {
ASDN::MutexLocker l(__instanceLock__);
if (_pendingInterfaceState != newState) {
_pendingInterfaceState = newState;
[[ASCATransactionQueue sharedQueue] enqueue:self];
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Remove the lock at the top of this method. It isn't needed and applyPendingInterfaceState asserts that we aren't locked. (_unlocked_applyPendingInterfaceState:?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is a mis-merge, thanks for spotting that.

}

- (void)applyPendingInterfaceState:(ASInterfaceState)newPendingState
{
//This method is currently called on the main thread. The assert has been added here because all of the
//did(Enter|Exit)(Display|Visible|Preload)State methods currently guarantee calling on main.
ASDisplayNodeAssertMainThread();
// It should never be possible for a node to be visible but not be allowed / expected to display.
ASDisplayNodeAssertFalse(ASInterfaceStateIncludesVisible(newState) && !ASInterfaceStateIncludesDisplay(newState));

// This method manages __instanceLock__ itself, to ensure the lock is not held while didEnter/Exit(.*)State methods are called, thus avoid potential deadlocks
ASDisplayNodeAssertLockUnownedByCurrentThread(__instanceLock__);

ASInterfaceState oldState = ASInterfaceStateNone;
ASInterfaceState newState = ASInterfaceStateNone;
{
ASDN::MutexLocker l(__instanceLock__);
if (_interfaceState == newState) {
return;
// newPendingState will not be used when ASCATransactionQueue is enabled
// and use _pendingInterfaceState instead for interfaceState update.
if ([[ASCATransactionQueue sharedQueue] disabled]) {
_pendingInterfaceState = newPendingState;
}
oldState = _interfaceState;
newState = _pendingInterfaceState;
if (newState == oldState) {
return;
}
_interfaceState = newState;
}

// It should never be possible for a node to be visible but not be allowed / expected to display.
ASDisplayNodeAssertFalse(ASInterfaceStateIncludesVisible(newState) && !ASInterfaceStateIncludesDisplay(newState));

// TODO: Trigger asynchronous measurement if it is not already cached or being calculated.
// if ((newState & ASInterfaceStateMeasureLayout) != (oldState & ASInterfaceStateMeasureLayout)) {
// }
Expand Down Expand Up @@ -2983,6 +3012,11 @@ - (void)setInterfaceState:(ASInterfaceState)newState
[self interfaceStateDidChange:newState fromState:oldState];
}

- (void)prepareForCATransactionCommit
{
[self applyPendingInterfaceState:ASInterfaceStateNone];
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Explain that pending state was set before and the ASInterfaceStateNone param here won't be used.

}

- (void)interfaceStateDidChange:(ASInterfaceState)newState fromState:(ASInterfaceState)oldState
{
// Subclass hook
Expand Down
33 changes: 32 additions & 1 deletion Source/ASRunLoopQueue.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,15 @@

NS_ASSUME_NONNULL_BEGIN

@protocol ASCATransactionQueueObserving <NSObject>
- (void)prepareForCATransactionCommit;
@end

@interface ASAbstractRunLoopQueue : NSObject
@end

AS_SUBCLASSING_RESTRICTED
@interface ASRunLoopQueue<ObjectType> : NSObject <NSLocking>
@interface ASRunLoopQueue<ObjectType> : ASAbstractRunLoopQueue <NSLocking>

/**
* Create a new queue with the given run loop and handler.
Expand All @@ -48,6 +55,30 @@ AS_SUBCLASSING_RESTRICTED

@end

AS_SUBCLASSING_RESTRICTED
@interface ASCATransactionQueue : ASAbstractRunLoopQueue

@property (nonatomic, readonly) BOOL isEmpty;
@property (nonatomic, readonly) BOOL disabled;
Copy link
Member

Choose a reason for hiding this comment

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

These properties need to be atomic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess ASRunloopQueue's isEmpty needs to be atomic also.

/**
* The queue to run on main run loop before CATransaction commit.
*
* @discussion this queue will run after ASRunLoopQueue and before CATransaction commit
* to get last chance of updating/coalesce info like interface state.
* Each node will only be called once per transaction commit to reflect interface change.
*/
+ (ASCATransactionQueue *)sharedQueue;
Copy link
Member

Choose a reason for hiding this comment

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

Declare this as an atomic readonly class property.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


- (void)enqueue:(id<ASCATransactionQueueObserving>)object;

/**
* @abstract Apply a node's interfaceState immediately rather than adding to the queue.
*/
- (void)disableInterfaceStateCoalesce;
Copy link
Member

Choose a reason for hiding this comment

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

I would just call this method -disable, since this queue isn't supposed to know anything about interface states per-se.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


@end


AS_SUBCLASSING_RESTRICTED
@interface ASDeallocQueue : NSObject

Expand Down
Loading