Improve System Trace Implementation #trivial #368

Adlai-Holler · 2017-06-17T20:40:02Z

Added Instruments template file that contains the code names and other options .
Add new events.
Add new structure for controlling signposts.
Remove spurious inclusion of OSAtomic.
Remove AS_MEASURE_AVOIDED_DATACONTROLLER_WORK since it's messy and now we can use system trace for this kind of thing.

I think more events would be great!

Collection edit operations.
Collection data loads (initial/reloadData).
Image downloads
Layout transitions

One example from iPhone simulator ASDKgram:

appleguy

This is some valuable development; thanks Adlai! I have some requested changes, but you could land this and explore them separately (or have an accept ready for when you choose to land).

appleguy · 2017-06-18T06:11:02Z

Source/ASDisplayNode.mm

+  });
+  BOOL isNested = pthread_getspecific(calculateLayoutKey) != NULL;
+  if (!isNested) {
+    pthread_setspecific(calculateLayoutKey, kCFBooleanTrue);


@Adlai-Holler This is quite a bit of code to add to a core method like this; what do you think about simplifying the operation with a #define? There are at least two good options:

Create a generic macro for manufacturing dispatch_once variables; I use the one below, for example.

Better, especially if we need to check for nesting elsewhere: add to the macro and perform the "checkAndSet" together. Macro would return the previous value while setting it if unset, and then you could write if (...CheckAndSet) { ...SignpostStart... }

#define ASCreateOnce(expr) ({ \ static dispatch_once_t onceToken; \ static __typeof__(expr) staticVar; \ dispatch_once(&onceToken, ^{ \ staticVar = expr; \ }); \ staticVar; \ })

e.g. in usage,

static pthread_key_t calculateLayoutKey = ASCreateOnce(pthread_key_create(&calculateLayoutKey, NULL));

Good point, thanks. To split the difference, I created a ASPthreadStaticKey(dtor) macro that lets you easily create a static pthread_key_t with a given destructor, and used it here. The general CreateOnce macro is great but I wanted something more specialized for this.

appleguy · 2017-06-18T06:14:15Z

Source/ASDisplayNode.mm

+
+  if (!isNested) {
+    pthread_setspecific(calculateLayoutKey, NULL);
+    ASProfilingSignpostEnd(ASSignpostCalculateLayout, self, 0, ASSignpostColorDefault);


I wonder if a higher-level version of this with fewer arguments would be warranted; ColorDefault is often used and the colors could be applied elsewhere as a mapping to the signpost type. A higher-level macro could also automatically capture the self pointer, and the signpost "routing layer" could choose to use it or not.

Good call. I've renamed ASProfilingSignpost* -> ASSignpost*, and replaced ASSignpostStart[4] with ASSignpostStartCustom[4] and added ASSignpostStart1. Same for end.

appleguy · 2017-06-18T06:16:22Z

Source/ASRunLoopQueue.mm

+  static dispatch_block_t postCommitHandler;
+  static dispatch_once_t onceToken;
+  dispatch_once(&onceToken, ^{
+    preLayoutHandler = ^{


A possible style here would be:

static dispatch_block_t preLayoutHandler = ASCreateOnce(^{ ASProfilingSignpostStart(ASSignpostCATransactionLayout, 0, [CATransaction currentState]); });

I like the look of this but you can't put breakpoints inside macros unfortunately, so I kept the ugly code.

appleguy · 2017-06-18T06:18:39Z

Source/Base/ASLog.h

+};
+
+typedef NS_ENUM(uintptr_t, ASSignpostColor) {
+  ASSignpostColorBlue,


By convention, perhaps set Blue = 0 to start off the enum (your choice)

appleguy · 2017-06-18T06:23:07Z

Source/Details/ASDataController.mm

@@ -836,27 +820,3 @@ - (void)_scheduleBlockOnMainSerialQueue:(dispatch_block_t)block
 }

 @end
-
-#if AS_MEASURE_AVOIDED_DATACONTROLLER_WORK


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..

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.

appleguy · 2017-06-18T06:26:16Z

Source/ASRunLoopQueue.mm

+} CATransactionPhase;
+
+@interface CATransaction (Private)
+ (void)addCommitHandler:(void(^)(void))block forPhase:(CATransactionPhase)phase;


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:].

ghost · 2017-06-18T19:48:39Z

1 Warning

⚠️

Please ensure license is correct for ASTextDebugOption.m:

//
//  ASTextDebugOption.m
//  Texture
//
//  Copyright (c) 2014-present, Facebook, Inc.  All rights reserved.
//  This source code is licensed under the BSD-style license found in the
//  LICENSE file in the /ASDK-Licenses directory of this source tree. An additional
//  grant of patent rights can be found in the PATENTS file in the same directory.
//
//  Modifications to this file made after 4/13/2017 are: Copyright (c) 2017-present,
//  Pinterest, Inc.  Licensed under the Apache License, Version 2.0 (the "License");
//  you may not use this file except in compliance with the License.
//  You may obtain a copy of the License at
//
//      http://www.apache.org/licenses/LICENSE-2.0
//

Generated by 🚫 Danger

Adlai-Holler · 2017-06-19T17:14:35Z

The remaining license header issue is erroneous – the YYText-based files need a different header.

* Improve the kdebug, system trace integration * Remove superseded subsystem * Address review comments * Please the license header gods * Address harder * Fix node block retaining collection view

Adlai Holler and others added 2 commits June 17, 2017 13:32

Improve the kdebug, system trace integration

09de853

Remove superseded subsystem

812341f

Adlai-Holler requested review from maicki, appleguy and nguyenhuy June 17, 2017 20:40

appleguy approved these changes Jun 18, 2017

View reviewed changes

Adlai-Holler added 4 commits June 18, 2017 11:03

Address review comments

99df7d1

Please the license header gods

3af84ea

Address harder

2e0d393

Fix node block retaining collection view

4cb6ffb

Adlai-Holler merged commit 4829a86 into master Jun 19, 2017

Adlai-Holler deleted the AHBetterTracing branch June 19, 2017 17:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve System Trace Implementation #trivial #368

Improve System Trace Implementation #trivial #368

Adlai-Holler commented Jun 17, 2017

appleguy left a comment

appleguy Jun 18, 2017

Adlai-Holler Jun 18, 2017

appleguy Jun 18, 2017

Adlai-Holler Jun 18, 2017

appleguy Jun 18, 2017

Adlai-Holler Jun 18, 2017

appleguy Jun 18, 2017

appleguy Jun 18, 2017

Adlai-Holler Jun 18, 2017 •

edited

Loading

appleguy Jun 18, 2017

ghost commented Jun 18, 2017

Adlai-Holler commented Jun 19, 2017

Improve System Trace Implementation #trivial #368

Improve System Trace Implementation #trivial #368

Conversation

Adlai-Holler commented Jun 17, 2017

appleguy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Adlai-Holler Jun 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jun 18, 2017

Adlai-Holler commented Jun 19, 2017

Adlai-Holler Jun 18, 2017 •

edited

Loading