-
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
Improve System Trace Implementation #trivial #368
Changes from all commits
09de853
812341f
99df7d1
3af84ea
2e0d393
4cb6ffb
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 |
---|---|---|
|
@@ -16,18 +16,55 @@ | |
// | ||
|
||
#import <AsyncDisplayKit/ASAvailability.h> | ||
#import <Foundation/Foundation.h> | ||
|
||
#pragma once | ||
/// The signposts we use. Signposts are grouped by color. The SystemTrace.tracetemplate file | ||
/// should be kept up-to-date with these values. | ||
typedef NS_ENUM(uint32_t, ASSignpostName) { | ||
// Collection/Table (Blue) | ||
ASSignpostDataControllerBatch = 300, // Alloc/layout nodes before collection update. | ||
ASSignpostRangeControllerUpdate, // Ranges update pass. | ||
ASSignpostCollectionUpdate, // Entire update process, from -endUpdates to [super perform…] | ||
|
||
// Rendering (Green) | ||
ASSignpostLayerDisplay = 325, // Client display callout. | ||
ASSignpostRunLoopQueueBatch, // One batch of ASRunLoopQueue. | ||
|
||
// Layout (Purple) | ||
ASSignpostCalculateLayout = 350, // Start of calculateLayoutThatFits to end. Max 1 per thread. | ||
|
||
// Misc (Orange) | ||
ASSignpostDeallocQueueDrain = 375, // One chunk of dealloc queue work. arg0 is count. | ||
ASSignpostCATransactionLayout, // The CA transaction commit layout phase. | ||
ASSignpostCATransactionCommit // The CA transaction commit post-layout phase. | ||
}; | ||
|
||
typedef NS_ENUM(uintptr_t, ASSignpostColor) { | ||
ASSignpostColorBlue, | ||
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. By convention, perhaps set Blue = 0 to start off the enum (your choice) |
||
ASSignpostColorGreen, | ||
ASSignpostColorPurple, | ||
ASSignpostColorOrange, | ||
ASSignpostColorRed, | ||
ASSignpostColorDefault | ||
}; | ||
|
||
static inline ASSignpostColor ASSignpostGetColor(ASSignpostName name, ASSignpostColor colorPref) { | ||
if (colorPref == ASSignpostColorDefault) { | ||
return (ASSignpostColor)((name / 25) % 4); | ||
} else { | ||
return colorPref; | ||
} | ||
} | ||
|
||
#define ASMultiplexImageNodeLogDebug(...) | ||
#define ASMultiplexImageNodeCLogDebug(...) | ||
|
||
#define ASMultiplexImageNodeLogError(...) | ||
#define ASMultiplexImageNodeCLogError(...) | ||
|
||
// Note: `<sys/kdebug_signpost.h>` only exists in Xcode 8 and later. | ||
#if defined(PROFILE) && __has_include(<sys/kdebug_signpost.h>) | ||
#define AS_KDEBUG_ENABLE defined(PROFILE) && __has_include(<sys/kdebug_signpost.h>) | ||
|
||
#if AS_KDEBUG_ENABLE | ||
|
||
#import <sys/kdebug_signpost.h> | ||
|
||
|
@@ -45,21 +82,27 @@ | |
#define APPSDBG_CODE(SubClass,code) KDBG_CODE(DBG_APPS, SubClass, code) | ||
#endif | ||
|
||
#define ASProfilingSignpost(x) \ | ||
AS_AT_LEAST_IOS10 ? kdebug_signpost(x, 0, 0, 0, (uint32_t)(x % 4)) \ | ||
: syscall(SYS_kdebug_trace, APPSDBG_CODE(DBG_MACH_CHUD, x) | DBG_FUNC_NONE, 0, 0, 0, (uint32_t)(x % 4)); | ||
// Currently we'll reserve arg3. | ||
#define ASSignpost(name, identifier, arg2, color) \ | ||
AS_AT_LEAST_IOS10 ? kdebug_signpost(name, (uintptr_t)identifier, (uintptr_t)arg2, 0, ASSignpostGetColor(name, color)) \ | ||
: syscall(SYS_kdebug_trace, APPSDBG_CODE(DBG_MACH_CHUD, name) | DBG_FUNC_NONE, (uintptr_t)identifier, (uintptr_t)arg2, 0, ASSignpostGetColor(name, color)); | ||
|
||
#define ASSignpostStartCustom(name, identifier, arg2) \ | ||
AS_AT_LEAST_IOS10 ? kdebug_signpost_start(name, (uintptr_t)identifier, (uintptr_t)arg2, 0, 0) \ | ||
: syscall(SYS_kdebug_trace, APPSDBG_CODE(DBG_MACH_CHUD, name) | DBG_FUNC_START, (uintptr_t)identifier, (uintptr_t)arg2, 0, 0); | ||
#define ASSignpostStart(name) ASSignpostStartCustom(name, self, 0) | ||
|
||
#define ASProfilingSignpostStart(x, y) \ | ||
AS_AT_LEAST_IOS10 ? kdebug_signpost_start((uint32_t)x, (uintptr_t)y, 0, 0, (uint32_t)(x % 4)) \ | ||
: syscall(SYS_kdebug_trace, APPSDBG_CODE(DBG_MACH_CHUD, x) | DBG_FUNC_START, (uintptr_t)y, 0, 0, (uint32_t)(x % 4)); | ||
#define ASSignpostEndCustom(name, identifier, arg2, color) \ | ||
AS_AT_LEAST_IOS10 ? kdebug_signpost_end(name, (uintptr_t)identifier, (uintptr_t)arg2, 0, ASSignpostGetColor(name, color)) \ | ||
: syscall(SYS_kdebug_trace, APPSDBG_CODE(DBG_MACH_CHUD, name) | DBG_FUNC_END, (uintptr_t)identifier, (uintptr_t)arg2, 0, ASSignpostGetColor(name, color)); | ||
#define ASSignpostEnd(name) ASSignpostEndCustom(name, self, 0, ASSignpostColorDefault) | ||
|
||
#define ASProfilingSignpostEnd(x, y) \ | ||
AS_AT_LEAST_IOS10 ? kdebug_signpost_end((uint32_t)x, (uintptr_t)y, 0, 0, (uint32_t)(x % 4)) \ | ||
: syscall(SYS_kdebug_trace, APPSDBG_CODE(DBG_MACH_CHUD, x) | DBG_FUNC_END, (uintptr_t)y, 0, 0, (uint32_t)(x % 4)); | ||
#else | ||
|
||
#define ASProfilingSignpost(x) | ||
#define ASProfilingSignpostStart(x, y) | ||
#define ASProfilingSignpostEnd(x, y) | ||
#define ASSignpost(name, identifier, arg2, color) | ||
#define ASSignpostStartCustom(name, identifier, arg2) | ||
#define ASSignpostStart(name) | ||
#define ASSignpostEndCustom(name, identifier, arg2, color) | ||
#define ASSignpostEnd(name) | ||
|
||
#endif |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -40,8 +40,6 @@ | |
//#define LOG(...) NSLog(__VA_ARGS__) | ||
#define LOG(...) | ||
|
||
#define AS_MEASURE_AVOIDED_DATACONTROLLER_WORK 0 | ||
|
||
#define RETURN_IF_NO_DATASOURCE(val) if (_dataSource == nil) { return val; } | ||
#define ASSERT_ON_EDITING_QUEUE ASDisplayNodeAssertNotNil(dispatch_get_specific(&kASDataControllerEditingQueueKey), @"%@ must be called on the editing transaction queue.", NSStringFromSelector(_cmd)) | ||
|
||
|
@@ -54,13 +52,6 @@ | |
|
||
typedef void (^ASDataControllerCompletionBlock)(NSArray<ASCollectionElement *> *elements, NSArray<ASCellNode *> *nodes); | ||
|
||
#if AS_MEASURE_AVOIDED_DATACONTROLLER_WORK | ||
@interface ASDataController (AvoidedWorkMeasuring) | ||
+ (void)_didLayoutNode; | ||
+ (void)_expectToInsertNodes:(NSUInteger)count; | ||
@end | ||
#endif | ||
|
||
@interface ASDataController () { | ||
id<ASDataControllerLayoutDelegate> _layoutDelegate; | ||
|
||
|
@@ -159,17 +150,14 @@ - (void)setLayoutDelegate:(id<ASDataControllerLayoutDelegate>)layoutDelegate | |
- (void)batchAllocateNodesFromElements:(NSArray<ASCollectionElement *> *)elements andLayout:(BOOL)shouldLayout batchSize:(NSInteger)batchSize batchCompletion:(ASDataControllerCompletionBlock)batchCompletionHandler | ||
{ | ||
ASSERT_ON_EDITING_QUEUE; | ||
#if AS_MEASURE_AVOIDED_DATACONTROLLER_WORK | ||
[ASDataController _expectToInsertNodes:elements.count]; | ||
#endif | ||
|
||
|
||
if (elements.count == 0 || _dataSource == nil) { | ||
batchCompletionHandler(@[], @[]); | ||
return; | ||
} | ||
|
||
ASProfilingSignpostStart(2, _dataSource); | ||
ASSignpostStart(ASSignpostDataControllerBatch); | ||
|
||
if (batchSize == 0) { | ||
batchSize = [[ASDataController class] parallelProcessorCount] * kASDataControllerSizingCountPerProcessor; | ||
} | ||
|
@@ -182,8 +170,8 @@ - (void)batchAllocateNodesFromElements:(NSArray<ASCollectionElement *> *)element | |
NSArray<ASCellNode *> *nodes = [self _allocateNodesFromElements:batchedElements andLayout:shouldLayout]; | ||
batchCompletionHandler(batchedElements, nodes); | ||
} | ||
ASProfilingSignpostEnd(2, _dataSource); | ||
|
||
ASSignpostEndCustom(ASSignpostDataControllerBatch, self, 0, (_dataSource != nil ? ASSignpostColorDefault : ASSignpostColorRed)); | ||
} | ||
|
||
/** | ||
|
@@ -228,10 +216,6 @@ - (void)_layoutNode:(ASCellNode *)node withConstrainedSize:(ASSizeRange)constrai | |
if (ASSizeRangeHasSignificantArea(sizeRange)) { | ||
[self _layoutNode:node withConstrainedSize:sizeRange]; | ||
} | ||
|
||
#if AS_MEASURE_AVOIDED_DATACONTROLLER_WORK | ||
[ASDataController _didLayoutNode]; | ||
#endif | ||
} | ||
|
||
allocatedNodeBuffer[i] = node; | ||
|
@@ -836,27 +820,3 @@ - (void)_scheduleBlockOnMainSerialQueue:(dispatch_block_t)block | |
} | ||
|
||
@end | ||
|
||
#if AS_MEASURE_AVOIDED_DATACONTROLLER_WORK | ||
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. IMO this is a pretty valuable thing, to the point that we should have unit tests at least verifying that the skipping / cancellation of layout work never regresses; this was a very important development. It does appear to add a good bit of complexity, although the code would be very similar with Signposts. With that in mind, and because a key area to extend the signpost system will be in ASDataController introspection, I'd vote to keep this around until it is refactored into something better. That said, you and @nguyenhuy should make the call and proceed as you'd like - I know there are big plans across the stack and you're best equipped to decide how to get this kind of view into ASDC.. 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. I see that this mechanism is valuable to you, but I truly believe this isn't the way to implement this. I spent some time this morning building a unit test to replace this. I failed, but ironically I discovered that the early bail is currently broken due to a subtle retain cycle where the node block retains the ASCollectionView. I've fixed that retain cycle, but I think it's worth noting that nobody noticed the feature was broken, possibly because this measurement feature is so obscure and unused. I care a lot about performance but I don't think this is the way to provide accountability. |
||
|
||
static volatile int64_t _totalExpectedItems = 0; | ||
static volatile int64_t _totalMeasuredNodes = 0; | ||
|
||
@implementation ASDataController (WorkMeasuring) | ||
|
||
+ (void)_didLayoutNode | ||
{ | ||
int64_t measured = OSAtomicIncrement64(&_totalMeasuredNodes); | ||
int64_t expected = _totalExpectedItems; | ||
if (measured % 20 == 0 || measured == expected) { | ||
NSLog(@"Data controller avoided work (underestimated): %lld / %lld", measured, expected); | ||
} | ||
} | ||
|
||
+ (void)_expectToInsertNodes:(NSUInteger)count | ||
{ | ||
OSAtomicAdd64((int64_t)count, &_totalExpectedItems); | ||
} | ||
|
||
@end | ||
#endif |
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.
Interesting, is this a real API? I didn't see it implemented in this patch. Given the compile-time gating, seems like a great thing to use.
Even though debug-only, since this could change in any iOS version (or maybe even doesn't exist on one of the older versions), it's worth adding an early-return to the registerObservers method checking [CALayer respondsToSelector:].