-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Overhaul our logging, add activity tracing support. #399
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -800,6 +800,8 @@ - (void)beginUpdates | |
|
||
if (_batchUpdateCount == 0) { | ||
_changeSet = [[_ASHierarchyChangeSet alloc] initWithOldData:[_dataController itemCountsFromDataSource]]; | ||
_changeSet.rootActivity = as_activity_create("Perform async collection update", AS_ACTIVITY_CURRENT, OS_ACTIVITY_FLAG_DEFAULT); | ||
_changeSet.submitActivity = as_activity_create("Submit changes for collection update", _changeSet.rootActivity, OS_ACTIVITY_FLAG_DEFAULT); | ||
} | ||
_batchUpdateCount++; | ||
} | ||
|
@@ -817,6 +819,7 @@ - (void)endUpdatesAnimated:(BOOL)animated completion:(nullable void (^)(BOOL))co | |
|
||
if (_batchUpdateCount == 0) { | ||
_ASHierarchyChangeSet *changeSet = _changeSet; | ||
|
||
// Nil out _changeSet before forwarding to _dataController to allow the change set to cause subsequent batch updates on the same run loop | ||
_changeSet = nil; | ||
changeSet.animated = animated; | ||
|
@@ -828,8 +831,13 @@ - (void)performBatchAnimated:(BOOL)animated updates:(void (^)())updates completi | |
{ | ||
ASDisplayNodeAssertMainThread(); | ||
[self beginUpdates]; | ||
if (updates) { | ||
updates(); | ||
as_activity_scope(_changeSet.rootActivity); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe worth a comment - I don't really understand what this anonymous scope is doing (I think it is only containing the submitActivity, but why is the rootActivity started right above the anonymous scope rather than at the top of the method?) |
||
{ | ||
// Only include client code in the submit activity, the rest just lives in the root activity. | ||
as_activity_scope(_changeSet.submitActivity); | ||
if (updates) { | ||
updates(); | ||
} | ||
} | ||
[self endUpdatesAnimated:animated completion:completion]; | ||
} | ||
|
@@ -1575,10 +1583,12 @@ - (void)_beginBatchFetchingIfNeededWithContentOffset:(CGPoint)contentOffset velo | |
|
||
- (void)_beginBatchFetching | ||
{ | ||
as_activity_create_for_scope("Batch fetch for collection node"); | ||
[_batchContext beginBatchFetching]; | ||
if (_asyncDelegateFlags.collectionNodeWillBeginBatchFetch) { | ||
dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ | ||
GET_COLLECTIONNODE_OR_RETURN(collectionNode, (void)0); | ||
as_log_debug(ASCollectionLog(), "Beginning batch fetch for %@ with context %@", collectionNode, _batchContext); | ||
[_asyncDelegate collectionNode:collectionNode willBeginBatchFetchWithContext:_batchContext]; | ||
}); | ||
} else if (_asyncDelegateFlags.collectionViewWillBeginBatchFetch) { | ||
|
@@ -1894,9 +1904,11 @@ - (void)rangeController:(ASRangeController *)rangeController didUpdateWithChange | |
} | ||
|
||
ASPerformBlockWithoutAnimation(!changeSet.animated, ^{ | ||
if(changeSet.includesReloadData) { | ||
as_activity_scope(as_activity_create("Commit collection update", changeSet.rootActivity, OS_ACTIVITY_FLAG_DEFAULT)); | ||
if (changeSet.includesReloadData) { | ||
_superIsPendingDataLoad = YES; | ||
[super reloadData]; | ||
as_log_debug(ASCollectionLog(), "Did reloadData %@", self.collectionNode); | ||
[changeSet executeCompletionHandlerWithFinished:YES]; | ||
} else { | ||
[_layoutFacilitator collectionViewWillPerformBatchUpdates]; | ||
|
@@ -1933,13 +1945,17 @@ - (void)rangeController:(ASRangeController *)rangeController didUpdateWithChange | |
numberOfUpdates++; | ||
} | ||
} completion:^(BOOL finished){ | ||
as_activity_scope(as_activity_create("Handle collection update completion", changeSet.rootActivity, OS_ACTIVITY_FLAG_DEFAULT)); | ||
as_log_verbose(ASCollectionLog(), "Update animation finished %{public}@", self.collectionNode); | ||
// Flush any range changes that happened as part of the update animations ending. | ||
[_rangeController updateIfNeeded]; | ||
[self _scheduleCheckForBatchFetchingForNumberOfChanges:numberOfUpdates]; | ||
[changeSet executeCompletionHandlerWithFinished:finished]; | ||
}]; | ||
as_log_debug(ASCollectionLog(), "Completed batch update %{public}@", self.collectionNode); | ||
|
||
// Flush any range changes that happened as part of submitting the update. | ||
as_activity_scope(changeSet.rootActivity); | ||
[_rangeController updateIfNeeded]; | ||
} | ||
}); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,6 +15,7 @@ | |
#import <AsyncDisplayKit/ASInternalHelpers.h> | ||
#import <AsyncDisplayKit/ASLayout.h> | ||
#import <AsyncDisplayKit/ASLayoutElementStylePrivate.h> | ||
#import <AsyncDisplayKit/ASLog.h> | ||
|
||
#import <AsyncDisplayKit/ASDisplayNode+FrameworkSubclasses.h> | ||
|
||
|
@@ -127,25 +128,20 @@ - (ASTraitCollection *)asyncTraitCollection | |
|
||
ASPrimitiveTraitCollectionDeprecatedImplementation | ||
|
||
@end | ||
|
||
#pragma mark - | ||
#pragma mark - ASLayoutElementAsciiArtProtocol | ||
|
||
@implementation ASDisplayNode (ASLayoutElementAsciiArtProtocol) | ||
|
||
- (NSString *)asciiArtString | ||
{ | ||
return [ASLayoutSpec asciiArtStringForChildren:@[] parentName:[self asciiArtName]]; | ||
} | ||
|
||
- (NSString *)asciiArtName | ||
{ | ||
NSString *string = NSStringFromClass([self class]); | ||
NSMutableString *result = [NSMutableString stringWithCString:object_getClassName(self) encoding:NSASCIIStringEncoding]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is interesting - it seems plausible to be faster, but is that indeed the reason you changed this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes I wanted to reduce the number of strings created from 3 to 1. Digging into the current implementation of |
||
if (_debugName) { | ||
string = [string stringByAppendingString:[NSString stringWithFormat:@"\"%@\"",_debugName]]; | ||
[result appendFormat:@" (%@)", _debugName]; | ||
} | ||
return string; | ||
return result; | ||
} | ||
|
||
@end | ||
|
@@ -222,6 +218,7 @@ @implementation ASDisplayNode (ASLayoutInternal) | |
*/ | ||
- (void)_setNeedsLayoutFromAbove | ||
{ | ||
as_activity_create_for_scope("Set needs layout from above"); | ||
ASDisplayNodeAssertThreadAffinity(self); | ||
|
||
// Mark the node for layout in the next layout pass | ||
|
@@ -315,6 +312,8 @@ - (void)_locked_measureNodeWithBoundsIfNecessary:(CGRect)bounds | |
} | ||
} | ||
|
||
as_activity_create_for_scope("Update node layout for current bounds"); | ||
as_log_verbose(ASLayoutLog(), "Node %@, bounds size %@, calculatedSize %@, calculatedIsDirty %d", self, NSStringFromCGSize(boundsSizeForLayout), NSStringFromCGSize(_calculatedDisplayNodeLayout->layout.size), _calculatedDisplayNodeLayout->isDirty()); | ||
// _calculatedDisplayNodeLayout is not reusable we need to transition to a new one | ||
[self cancelLayoutTransition]; | ||
|
||
|
@@ -332,7 +331,20 @@ - (void)_locked_measureNodeWithBoundsIfNecessary:(CGRect)bounds | |
|
||
// nextLayout was likely created by a call to layoutThatFits:, check if it is valid and can be applied. | ||
// If our bounds size is different than it, or invalid, recalculate. Use #define to avoid nullptr-> | ||
if (nextLayout == nullptr || nextLayout->isDirty() == YES || layoutSizeDifferentFromBounds) { | ||
BOOL pendingLayoutApplicable = NO; | ||
if (nextLayout == nullptr) { | ||
as_log_verbose(ASLayoutLog(), "No pending layout."); | ||
} else if (nextLayout->isDirty()) { | ||
as_log_verbose(ASLayoutLog(), "Pending layout is invalid."); | ||
} else if (layoutSizeDifferentFromBounds) { | ||
as_log_verbose(ASLayoutLog(), "Pending layout size %@ doesn't match bounds size.", NSStringFromCGSize(nextLayout->layout.size)); | ||
} else { | ||
as_log_verbose(ASLayoutLog(), "Using pending layout %@.", nextLayout->layout); | ||
pendingLayoutApplicable = YES; | ||
} | ||
|
||
if (!pendingLayoutApplicable) { | ||
as_log_verbose(ASLayoutLog(), "Measuring with previous constrained size."); | ||
// Use the last known constrainedSize passed from a parent during layout (if never, use bounds). | ||
ASSizeRange constrainedSize = [self _locked_constrainedSizeForLayoutPass]; | ||
ASLayout *layout = [self calculateLayoutThatFits:constrainedSize | ||
|
@@ -350,6 +362,7 @@ - (void)_locked_measureNodeWithBoundsIfNecessary:(CGRect)bounds | |
// This can occur for either pre-calculated or newly-calculated layouts. | ||
if (nextLayout->requestedLayoutFromAbove == NO | ||
&& CGSizeEqualToSize(boundsSizeForLayout, nextLayout->layout.size) == NO) { | ||
as_log_verbose(ASLayoutLog(), "Layout size doesn't match bounds size. Requesting layout from above."); | ||
// The layout that we have specifies that this node (self) would like to be a different size | ||
// than it currently is. Because that size has been computed within the constrainedSize, we | ||
// expect that calling setNeedsLayoutFromAbove will result in our parent resizing us to this. | ||
|
@@ -506,10 +519,13 @@ - (void)transitionLayoutWithSizeRange:(ASSizeRange)constrainedSize | |
measurementCompletion:(void(^)())completion | ||
{ | ||
ASDisplayNodeAssertMainThread(); | ||
as_activity_create_for_scope("Transition node layout"); | ||
as_log_debug(ASLayoutLog(), "Transition layout for %@ sizeRange %@ anim %d asyncMeasure %d", self, NSStringFromASSizeRange(constrainedSize), animated, shouldMeasureAsync); | ||
|
||
if (constrainedSize.max.width <= 0.0 || constrainedSize.max.height <= 0.0) { | ||
// Using CGSizeZero for the sizeRange can cause negative values in client layout code. | ||
// Most likely called transitionLayout: without providing a size, before first layout pass. | ||
as_log_verbose(ASLayoutLog(), "Ignoring transition due to bad size range."); | ||
return; | ||
} | ||
|
||
|
@@ -526,9 +542,14 @@ - (void)transitionLayoutWithSizeRange:(ASSizeRange)constrainedSize | |
|
||
// Every new layout transition has a transition id associated to check in subsequent transitions for cancelling | ||
int32_t transitionID = [self _startNewTransition]; | ||
as_log_verbose(ASLayoutLog(), "Transition ID is %d", transitionID); | ||
// NOTE: This block captures self. It's cheaper than hitting the weak table. | ||
asdisplaynode_iscancelled_block_t isCancelled = ^{ | ||
return (BOOL)(_transitionID != transitionID); | ||
BOOL result = (_transitionID != transitionID); | ||
if (result) { | ||
as_log_verbose(ASLayoutLog(), "Transition %d canceled, superseded by %d", transitionID, _transitionID.load()); | ||
} | ||
return result; | ||
}; | ||
|
||
// Move all subnodes in layout pending state for this transition | ||
|
@@ -573,6 +594,7 @@ - (void)transitionLayoutWithSizeRange:(ASSizeRange)constrainedSize | |
if (isCancelled()) { | ||
return; | ||
} | ||
as_activity_create_for_scope("Commit layout transition"); | ||
ASLayoutTransition *pendingLayoutTransition; | ||
_ASTransitionContext *pendingLayoutTransitionContext; | ||
{ | ||
|
@@ -598,10 +620,13 @@ - (void)transitionLayoutWithSizeRange:(ASSizeRange)constrainedSize | |
} | ||
|
||
// Apply complete layout transitions for all subnodes | ||
ASDisplayNodePerformBlockOnEverySubnode(self, NO, ^(ASDisplayNode * _Nonnull node) { | ||
[node _completePendingLayoutTransition]; | ||
node.hierarchyState &= (~ASHierarchyStateLayoutPending); | ||
}); | ||
{ | ||
as_activity_create_for_scope("Complete pending layout transitions for subtree"); | ||
ASDisplayNodePerformBlockOnEverySubnode(self, NO, ^(ASDisplayNode * _Nonnull node) { | ||
[node _completePendingLayoutTransition]; | ||
node.hierarchyState &= (~ASHierarchyStateLayoutPending); | ||
}); | ||
} | ||
|
||
// Measurement pass completion | ||
// Give the subclass a change to hook into before calling the completion block | ||
|
@@ -614,7 +639,10 @@ - (void)transitionLayoutWithSizeRange:(ASSizeRange)constrainedSize | |
[pendingLayoutTransition applySubnodeInsertions]; | ||
|
||
// Kick off animating the layout transition | ||
[self animateLayoutTransition:pendingLayoutTransitionContext]; | ||
{ | ||
as_activity_create_for_scope("Animate layout transition"); | ||
[self animateLayoutTransition:pendingLayoutTransitionContext]; | ||
} | ||
|
||
// Mark transaction as finished | ||
[self _finishOrCancelTransition]; | ||
|
@@ -805,7 +833,7 @@ - (void)_completePendingLayoutTransition | |
*/ | ||
- (void)_completeLayoutTransition:(ASLayoutTransition *)layoutTransition | ||
{ | ||
// Layout transition is not supported for nodes that are not have automatic subnode management enabled | ||
// Layout transition is not supported for nodes that do not have automatic subnode management enabled | ||
if (layoutTransition == nil || self.automaticallyManagesSubnodes == NO) { | ||
return; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Longer term, I wonder if a separate file like ASLog+DebugDescriptions.m would be a good place to locate category implementations for all the node classes. One-stop shop for logging contents with helper macros and such. Locating it in the class like this might be better for other reasons, though.