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

Overhaul our logging, add activity tracing support. #399

Merged
merged 3 commits into from
Jul 4, 2017

Conversation

Adlai-Holler
Copy link
Member

@Adlai-Holler Adlai-Holler commented Jun 29, 2017

In iOS 10, the kernel logging and activity tracing frameworks were unified together. Check out this WWDC talk for a full run-through of what that means, but it's pretty powerful https://developer.apple.com/videos/play/wwdc2016/721/ .

Short video demo of the verbose level on a cold launch of the Pinterest iOS app.
https://youtu.be/e392CekeH4c EDIT: another demo https://youtu.be/H_P5nCFlPYM

This diff creates several different logs, and it adds a new level verbose in addition to the system levels info, debug, fault, error. We should feel free to add new logs in the future because these things are fast and very easy to disable.

/// Log for general node-specific events e.g. interfaceState (for root nodes).
os_log_t ASNodeLog();
/// Log for layout-specific events e.g. calculateLayout (for root nodes).
os_log_t ASLayoutLog();
/// Log for display-specific events e.g. display queue batches.
os_log_t ASRenderLog();
/// Log for collection node events e.g. performBatchUpdates.
os_log_t ASCollectionLog();
/// Log for ASNetworkImageNode and ASMultiplexImageNode
os_log_t ASImageLoadingLog();
/// Specialized log for our main thread deallocation system.
os_log_t ASMainThreadDeallocationLog();

The default log level is intended to be quiet enough that users don't turn it off. Currently includes:

  • Activities and logs for all calls to transitionLayoutWithSizeRange. @nguyenhuy it could be beneficial to add more logs in here, probably at the verbose level.
  • Activities and logs for all collection initial load/reloadData/performBatchUpdates.
  • Activities and logs for all collection node batch fetching.
  • Activities and logs for ASNavigationController and ASTabBarController operations.
  • Activities and logs for trait collection propagation.
  • Activities and logs for transitionLayoutWithAnimation:.

The verbose log level is a free-for-all. We should feel free to dump more and more content in there to our heart's content. Currently includes:

  • Logs and activities for calls to [ASDisplayNode calculateLayoutThatFits:], along with ASCII art for the layout spec.
  • Activities for all range controller updates.
  • Activities for all data controller cell allocation batches.
  • Activities for all calls to -recursivelySetInterfaceState:
  • Logs for all node interface state changes.
  • Logs for all node didLoad events.
  • Logs for all ASNetworkImageNode/ASMultiplexImageNode events.
  • Logs for addSubnode/removeFromSupernode.

Requirements

  • For activity tracing, Texture's deployment target must be raised to 10.0.
    • Easiest way is to go to the Pods project of your Xcode workspace, select the Texture target and under Build settings raise iOS Deployment Target to 10.0.
    • The tracing API in 9.0 is very different, and it's incompatible with the unified system so we don't use it.
  • To use Console.app, you have to use a real device not simulator. The simulator and Console.app are supposed to work together, but on my computers they interfere and the simulator dies.
  • In Console.app, check "Include debug messages" and "Include info messages"

Recommendations

  • Set debugName and/or accessibilityIdentifiers! These help tremendously with identifying nodes. You can use the ASSetDebugName macro to conditionally set the debug name if you're in DEBUG and skip it in production.
  • Filter by library:UIKit and library:MyApp to hide log messages from Security, CoreFoundation, and other system frameworks you probably don't care about.
  • Filter by process e.g. process:Pinterest. You can also filter by PID for each run of your process if you want.
  • Save your common search terms (see screenshot below) so you can quickly jump to them.
  • Show the PID: ThreadID filed in the results view. This field is pretty worthwhile.
  • Get used to the difference between the Activities view and the default view.
    • For instance, if you're looking for info about <ASDisplayNode: 0xFFFFFFFF>, you can leave the Activities view, then search for 0xFFFFFFFF, then read the Activity ID 12345 of the log message you're interested in, then use that as a search term.

Pretty pictures:

screen shot 2017-06-29 at 6 42 42 pm
screen shot 2017-06-29 at 6 42 33 pm
screen shot 2017-06-29 at 6 46 31 pm

@Adlai-Holler Adlai-Holler force-pushed the AHBetterLogging branch 11 times, most recently from 41a4baf to aa503ca Compare June 30, 2017 01:42
@TextureGroup TextureGroup deleted a comment Jun 30, 2017
@TextureGroup TextureGroup deleted a comment Jun 30, 2017
@TextureGroup TextureGroup deleted a comment Jun 30, 2017
@TextureGroup TextureGroup deleted a comment Jun 30, 2017
@TextureGroup TextureGroup deleted a comment Jun 30, 2017
@TextureGroup TextureGroup deleted a comment Jun 30, 2017
@TextureGroup TextureGroup deleted a comment Jun 30, 2017
@appleguy
Copy link
Member

appleguy commented Jul 2, 2017

@Adlai-Holler fixed up the two merge conflicts. I'll try to get this reviewed in the next hour.

Copy link
Member

@appleguy appleguy left a comment

Choose a reason for hiding this comment

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

This is like the god-diff of logging! Holy wow.

All of my comments are questions or suggestions, so I think it would be a good idea to land this and address them separately.

I will say that this adds a nontrivial amount of complexity to some of our denser areas of the code. The value proposition is unquestionable - it's worth that cost - but it would also be worth imagining if there are ways (through denser macros or other wrappers) that can make the logging hints lighter weight visually.

One way to do that would be for the invocation to be as short as possible and really lead with the log message, to have them serve as comments, and put everything else in arguments later in the call.

@@ -315,13 +315,13 @@ - (void)handleVisibilityChange:(BOOL)isVisible
if (ip != nil) {
[result addObject:@{ @"indexPath" : ip }];
}
[result addObject:@{ @"collectionNode" : ASObjectDescriptionMakeTiny(owningNode) }];
Copy link
Member

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.

@@ -828,8 +831,12 @@ - (void)performBatchAnimated:(BOOL)animated updates:(void (^)())updates completi
{
ASDisplayNodeAssertMainThread();
[self beginUpdates];
if (updates) {
updates();
as_activity_scope(_changeSet.rootActivity);
Copy link
Member

Choose a reason for hiding this comment

The 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?)

@@ -1575,10 +1582,12 @@ - (void)_beginBatchFetchingIfNeededWithContentOffset:(CGPoint)contentOffset velo

- (void)_beginBatchFetching
{
as_activity_scope(as_activity_create("Batch fetch for collection node", AS_ACTIVITY_CURRENT, OS_ACTIVITY_FLAG_DEFAULT));
Copy link
Member

Choose a reason for hiding this comment

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

Should we offer as_activity_scope_create() for one level of parentheses?

Copy link
Member

Choose a reason for hiding this comment

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

Also worth to use AS_ACTIVITY_CURRENT and OS_ACTIVITY_FLAG_DEFAULT as default values as well.

- (NSString *)asciiArtString
{
return [ASLayoutSpec asciiArtStringForChildren:@[] parentName:[self asciiArtName]];
}

- (NSString *)asciiArtName
{
NSString *string = NSStringFromClass([self class]);
NSMutableString *result = [NSMutableString stringWithCString:object_getClassName(self) encoding:NSASCIIStringEncoding];
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

@Adlai-Holler Adlai-Holler Jul 2, 2017

Choose a reason for hiding this comment

The 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 NSStringFromClass shows that it always creates a new instance of NSString.

@@ -1304,6 +1310,7 @@ + (void)scheduleNodeForRecursiveDisplay:(ASDisplayNode *)node
}];
});

as_log_verbose(ASRenderLog(), "%s %@", sel_getName(_cmd), node);
Copy link
Member

Choose a reason for hiding this comment

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

I think ASDisplayLog might be a better name for this, as we've standardized on Display Range, recursivelyEnsureDisplay, setNeedsDisplay, etc. There are some stragglers like renderQueue here though, and I realize ASDisplay looks a bit like ASDisplayNode, so you can decide.

@@ -2331,6 +2339,11 @@ - (void)_removeFromSupernodeIfEqualTo:(ASDisplayNode *)supernode

- (void)_removeFromSupernode:(ASDisplayNode *)supernode view:(UIView *)view layer:(CALayer *)layer
{
// TODO: Should we simply return early if supernode is nil? This currently gets called for every insertSubnode.
Copy link
Member

Choose a reason for hiding this comment

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

The answer to this is probably no, because it is possible to have the _view / _layer be in a parent without having a supernode. I've fixed some bugs related to that before: specifically addSubnode: category exists on UIView, but when you perform that and there's no associated node, then removeFromSupernode did not remove it from the view (so it had assymetric behavior).

In that sense I think this is correct, so you can remove the TODO and optionally comment as to why we do this.

ASDisplayNodeLogEvent(self, @"interfaceStateDidChange: %@, old: %@", NSStringFromASInterfaceState(newState), NSStringFromASInterfaceState(oldState));
// Log this change, unless it's just the node going from {} -> {Measure} because that change happens
// for all cell nodes and it isn't currently meaningful.
BOOL isJustEnteringMeasure = (oldState == ASInterfaceStateNone && newState == ASInterfaceStateMeasureLayout);
Copy link
Member

Choose a reason for hiding this comment

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

Is the better way to check this, old | new == MeasureLayout ?

@@ -3179,7 +3211,10 @@ - (ASEventLog *)eventLog
if (self.debugName.length > 0) {
[result addObject:@{ @"debugName" : ASStringWithQuotesIfMultiword(self.debugName)}];
}

if (self.accessibilityIdentifier.length > 0) {
[result addObject:@{ @"axId": ASStringWithQuotesIfMultiword(self.accessibilityIdentifier) }];
Copy link
Member

Choose a reason for hiding this comment

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

Why does this use axId but above, the axId is set to kCFNull? Worth a comment in the kCFNull case as to why that is the key for the dictionary.

Copy link
Member Author

Choose a reason for hiding this comment

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

Since this is the -debugDescription we can afford to be more verbose. The difference in output string is:

<MYNode: 0x00000001; axId = "Like button"> vs. <MYNode: 0x00000001; "Like button">

I'll find some way to expose this behavior more clearly in headers/comments.

} \
})

/// e.g. { +Visible, -Preload } (although that should never actually happen.)
Copy link
Member

Choose a reason for hiding this comment

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

This is a great way to communicate interface state changes!

os_log_t ASRenderLog() {
static os_log_t log;
#if ASEnableLogs && ASRenderLogEnabled
static dispatch_once_t onceToken;
Copy link
Member

Choose a reason for hiding this comment

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

This is totally ripe for ASCreateOnce() plsy pls

Copy link
Member

@nguyenhuy nguyenhuy left a comment

Choose a reason for hiding this comment

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

This is really cool indeed! Nits aside, LGTM!

@@ -1575,10 +1582,12 @@ - (void)_beginBatchFetchingIfNeededWithContentOffset:(CGPoint)contentOffset velo

- (void)_beginBatchFetching
{
as_activity_scope(as_activity_create("Batch fetch for collection node", AS_ACTIVITY_CURRENT, OS_ACTIVITY_FLAG_DEFAULT));
Copy link
Member

Choose a reason for hiding this comment

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

Also worth to use AS_ACTIVITY_CURRENT and OS_ACTIVITY_FLAG_DEFAULT as default values as well.

@@ -3166,9 +3188,19 @@ - (ASEventLog *)eventLog
- (NSMutableArray<NSDictionary *> *)propertiesForDescription
{
NSMutableArray<NSDictionary *> *result = [NSMutableArray array];
if (self.debugName.length > 0) {
[result addObject:@{ @"debugName" : ASStringWithQuotesIfMultiword(self.debugName) }];
ASPushMainThreadAssertionsDisabled();
Copy link
Member

Choose a reason for hiding this comment

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

❤️

// 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.
Copy link
Member

Choose a reason for hiding this comment

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

What's up with the indentations above? Tabs?

@@ -547,27 +562,36 @@ - (void)updateWithChangeSet:(_ASHierarchyChangeSet *)changeSet
}

// Mutable copy of current data.
Copy link
Member

Choose a reason for hiding this comment

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

This comment is in a wrong place now. Please move it down or remove it.

downloadIdentifier = [_downloader downloadImageWithURL:url
callbackQueue:dispatch_get_main_queue()
downloadProgress:NULL
completion:^(id <ASImageContainerProtocol> _Nullable imageContainer, NSError * _Nullable error, id _Nullable downloadIdentifier) {
as_log_verbose(ASImageLoadingLog(), "Downloaded image for %@ img: %@ url: %@", self, [imageContainer asdk_image], url);
Copy link
Member

Choose a reason for hiding this comment

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

Should we weakify/strongify here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call, I moved the log into the completion block since it's always static.

#import <AsyncDisplayKit/ASLog.h>

os_log_t ASNodeLog() {
static os_log_t log;
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this need to be initialized to some blank / NULL value in the case that the compile time gating is turned off?

@Adlai-Holler
Copy link
Member Author

Thanks bros for the feedback, it's in much better shape now and I'll land post-CI.

@ghost
Copy link

ghost commented Jul 4, 2017

1 Warning
⚠️ This is a big PR, please consider splitting it up to ease code review.

Generated by 🚫 Danger

@Adlai-Holler Adlai-Holler merged commit 8ec4b31 into master Jul 4, 2017
@Adlai-Holler Adlai-Holler deleted the AHBetterLogging branch July 4, 2017 02:03
bernieperez pushed a commit to AtomTickets/Texture that referenced this pull request Apr 25, 2018
* Improve the os_log and os_activity integration

* Address feedback from Scott and Huy
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants