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

RUMM-1026 Use sysctl to gather a more accurate App Launch Time #381

Merged
merged 5 commits into from
Jan 28, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions Sources/Datadog/Core/System/LaunchTimeProvider.swift
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,9 @@ internal protocol LaunchTimeProviderType {

internal class LaunchTimeProvider: LaunchTimeProviderType {
var launchTime: TimeInterval? {
// Following dynamic dispatch synchronisation mutes TSAN issues when running tests from CLI.
// Even if AppLaunchTime() is using a lock behind the scenes, TSAN will report a data race if there are no synchronizations at this level.
objc_sync_enter(self)
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 couldn't reproduce the TSAN issues locally, but I believe that if we don't involve any Objc objects (and call a C function) in that call paths there can't be any dynamic dispatches involved to begin with.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds reasonable 👍. My suspicion behind this TSAN issue was indeed around the dynamic dispatch with this call.

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 actually had to come back to using objc_sync_enter()/objc_sync_exit(). I could not reproduce locally the test failure, but based on Bitrise logs, TSAN reported a potential data race on the call to AppLaunchTime() where a write happened according to TSAN. When reporting, TSAN crashes on a SIGABRT.

The only write in that C function might come from pthread_rwlock_rdlock(&rwLock); which in itself must be thread safe (otherwise nothing can be). So anyway, this is back here, it might be extraneous but let's trust TSAN for now.

let time = ObjcAppLaunchHandler.launchTime()
let time = AppLaunchTime()
objc_sync_exit(self)
return time > 0 ? time : nil
}
Expand Down
95 changes: 58 additions & 37 deletions Sources/_Datadog_Private/ObjcAppLaunchHandler.m
Original file line number Diff line number Diff line change
Expand Up @@ -4,60 +4,81 @@
* Copyright 2019-2020 Datadog, Inc.
*/

#import <UIKit/UIKit.h>
#import "ObjcAppLaunchHandler.h"
#import <sys/sysctl.h>
#import <UIKit/UIKit.h>
#import <pthread.h>

@interface AppLaunchTimer : NSObject
@property NSDate *frameworkLoadTime;
// Knowing how this value is collected and that it is written only once, the use of
// `atomic` property is enough for thread safety. It guarantees that neither partial
// nor garbage value will be returned.
@property (atomic) NSTimeInterval timeToApplicationBecomeActive;
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 believe atomic is already the default for a @property. That being said, I don't think this was necessary due the inherent sequencing of setting/getting this property throughout the App lifecycle.

Copy link
Contributor Author

@acgh acgh Jan 25, 2021

Choose a reason for hiding this comment

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

So it looks like LaunchTimeProviderTests.testThreadSafety() disagrees with my assumptions 🤔
see: https://app.bitrise.io/build/87853c6c2bfa5825#?tab=log

@end
// `AppLaunchHandler` aims to track some times as part of the sequence described in Apple's "About the App Launch Sequence"
// https://developer.apple.com/documentation/uikit/app_and_environment/responding_to_the_launch_of_your_app/about_the_app_launch_sequence

// A Read-Write lock to allow concurrent reads of TimeToApplicationDidBecomeActive, unless the initial (and only) write is locking it.
static pthread_rwlock_t rwLock;
Copy link
Contributor

Choose a reason for hiding this comment

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

doesn't it work with objc_sync_enter/exit in LaunchTimeProvider? now we have a global lock staying around, not great :/

static NSTimeInterval TimeToApplicationDidBecomeActive = 0.0;

NS_INLINE NSTimeInterval QueryProcessStartTimeWithFallback(NSTimeInterval fallbackTime) {
NSTimeInterval processStartTime;
// Query the current process' start time:
// https://www.freebsd.org/cgi/man.cgi?sysctl(3)
// https://github.com/darwin-on-arm/xnu/blob/707bfdc4e9a46e3612e53994fffc64542d3f7e72/bsd/sys/sysctl.h#L681
// https://github.com/darwin-on-arm/xnu/blob/707bfdc4e9a46e3612e53994fffc64542d3f7e72/bsd/sys/proc.h#L97

struct kinfo_proc kip;
size_t kipSize = sizeof(kip);
int mib[4] = {CTL_KERN, KERN_PROC, KERN_PROC_PID, getpid()};
int res = sysctl(mib, 4, &kip, &kipSize, NULL, 0);

@implementation AppLaunchTimer

- (instancetype)init
{
self = [super init];
if (self) {
NSDate *time = [NSDate new];
self.frameworkLoadTime = time;
self.timeToApplicationBecomeActive = -1;
[[NSNotificationCenter defaultCenter] addObserver:self
selector:@selector(handleDidBecomeActiveNotification)
name:UIApplicationDidBecomeActiveNotification
object:nil
];
if (res == 0) {
// The process' start time is provided relative to 1 Jan 1970
struct timeval startTime = kip.kp_proc.p_starttime;
processStartTime = startTime.tv_sec + startTime.tv_usec / USEC_PER_SEC;
// Convert to time since 1 Jan 2001 to align with CFAbsoluteTimeGetCurrent()
processStartTime -= kCFAbsoluteTimeIntervalSince1970;
} else {
// Fallback to less accurate delta with DD's framework load time
processStartTime = fallbackTime;
}
return self;
return processStartTime;
}

- (void)handleDidBecomeActiveNotification {
if (self.timeToApplicationBecomeActive > -1) { // sanity check & extra safety
NS_INLINE void ComputeTimeToApplicationDidBecomeActiveWithFallback(NSTimeInterval fallbackTime) {
if (TimeToApplicationDidBecomeActive > 0) {
return;
}

NSDate *time = [NSDate new];
NSTimeInterval duration = [time timeIntervalSinceDate:self.frameworkLoadTime];
self.timeToApplicationBecomeActive = duration;
[[NSNotificationCenter defaultCenter] removeObserver:self];
NSTimeInterval now = CFAbsoluteTimeGetCurrent();
NSTimeInterval processStartTime = QueryProcessStartTimeWithFallback(fallbackTime);
TimeToApplicationDidBecomeActive = now - processStartTime;
}

@interface AppLaunchHandler : NSObject
@end


@implementation ObjcAppLaunchHandler: NSObject

static AppLaunchTimer *_timer;
@implementation AppLaunchHandler

+ (void)load {
// This is called at the `_Datadog_Private` load time, keep the work minimal
_timer = [AppLaunchTimer new];
}
NSTimeInterval frameworkLoadTime = CFAbsoluteTimeGetCurrent();
id __block token = [NSNotificationCenter.defaultCenter
addObserverForName:UIApplicationDidBecomeActiveNotification
object:nil
queue:NSOperationQueue.mainQueue
usingBlock:^(NSNotification *_){

+ (NSTimeInterval)launchTime {
return _timer.timeToApplicationBecomeActive;
pthread_rwlock_init(&rwLock, NULL);
pthread_rwlock_wrlock(&rwLock);
ComputeTimeToApplicationDidBecomeActiveWithFallback(frameworkLoadTime);
pthread_rwlock_unlock(&rwLock);

[NSNotificationCenter.defaultCenter removeObserver:token];
}];
}

@end

CFTimeInterval AppLaunchTime() {
pthread_rwlock_rdlock(&rwLock);
CFTimeInterval time = TimeToApplicationDidBecomeActive;
pthread_rwlock_unlock(&rwLock);
return time;
}
8 changes: 2 additions & 6 deletions Sources/_Datadog_Private/include/ObjcAppLaunchHandler.h
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,6 @@
* Copyright 2019-2020 Datadog, Inc.
*/

#import <Foundation/Foundation.h>
#import <CoreFoundation/CFDate.h>

@interface ObjcAppLaunchHandler : NSObject

+ (NSTimeInterval)launchTime;

@end
CFTimeInterval AppLaunchTime(void);