New runloop observer #5632
New runloop observer #5632
Conversation
noahsmartin
force-pushed
the
newRunloopObserver
branch
from
7e0b439 to
de85790
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5632 +/- ##
=============================================
- Coverage 86.550% 8.157% -78.394%
=============================================
Files 417 386 -31
Lines 35429 27595 -7834
Branches 15353 57 -15296
=============================================
- Hits 30664 2251 -28413
- Misses 4725 25344 +20619
+ Partials 40 0 -40
... and 407 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
noahsmartin
force-pushed
the
newRunloopObserver
branch
from
de85790 to
0590db9
Compare
| var maxFPS = 60.0 | ||
| if #available(iOS 13.0, tvOS 13.0, *) { | ||
| let window = UIApplication.shared.connectedScenes.flatMap { ($0 as? UIWindowScene)?.windows ?? [] }.first { $0.isKeyWindow } | ||
| maxFPS = Double(window?.screen.maximumFramesPerSecond ?? 60) |
There was a problem hiding this comment.
I believe the display rate can change dynamically in a few cases like when displaying video. Not seeing any way to subscribe to that change though.
There was a problem hiding this comment.
yeah I didn't see it either. I figured using maximum at least would mean we would detect any time slow code is preventing the app from running at fastest possible. I think other things can cause the OS to decide to slow it down like low power mode. But you probably want to still know when frames are going to slow even if the OS chooses not to render them all
There was a problem hiding this comment.
The CADisplayLink docs actually mention this a bit https://developer.apple.com/documentation/quartzcore/cadisplaylink#Preferred-and-Actual-Frame-Rates.
You run into a risk of falsely reporting slow frames if the frame rate changes. That's why we calculate the actual frame rate here:
sentry-cocoa/Sources/Sentry/SentryFramesTracker.m
Lines 186 to 197 in 9a17767
| private func waitForHang(semaphore: DispatchSemaphore, started: TimeInterval, isStarting: Bool) { | ||
| dispatchPrecondition(condition: .onQueue(queue)) | ||
|
|
||
| let timeout = DispatchTime.now() + DispatchTimeInterval.milliseconds(Int((expectedFrameDuration + thresholdForFrameStacktrace) * 1_000)) |
There was a problem hiding this comment.
nit: for easier readability I'd expect thresholdForFrameStacktraceDurationMs = expectedFrameDuration *1.5 * 1000 on assignment instead of calculating down here.
| threads?.forEach { $0.current = false } | ||
| threads?[0].current = true | ||
| } | ||
| let duration = dateProvider.systemUptime() - started |
There was a problem hiding this comment.
Assuming this works fine for the first frame edge case?
| } | ||
| } | ||
|
|
||
| private func continueHang(started: TimeInterval, isStarting: Bool) { |
There was a problem hiding this comment.
How does this work if the app is backgrounded?
There was a problem hiding this comment.
I've never seen a case where the runloop doesn't get to finish an iteration. Even if the app is backgrounded your code isn't suspended instantly (for example NSNotifications get called on the main thread). If the main thread was suspended and never got a chance to resume then we would use the event stored to disk and report a fatal exception
There was a problem hiding this comment.
Gotcha, I was more thinking about whether the lastFrameTime value could get out of sync at all and report a false positive since that seems like it might have happened with the current implementation, but I'm guessing you're right that the runloop is allowed to finish its iteration.
| private var maxHangTime: TimeInterval? | ||
|
|
||
| func start() { | ||
| let observer = CFRunLoopObserverCreateWithHandler(nil, CFRunLoopActivity.beforeWaiting.rawValue | CFRunLoopActivity.afterWaiting.rawValue | CFRunLoopActivity.beforeSources.rawValue, true, CFIndex(INT_MAX)) { [weak self] _, activity in |
There was a problem hiding this comment.
One weird edge-case that ChatGPT claims is possible is re-entry into the current runloop via CFRunLoopPerformBlock that could result in false negatives, I guess as part of keyboard/modal interactions? I'm not very familiar with this API though so not sure if that's a real or hallucination.
There was a problem hiding this comment.
I'll test out keyboards specifically
WIP because this has no tests yet, just opening it to let CI run
#skip-changelog