Reset frames tracker on activity started #2269
Conversation
| @@ -54,6 +54,7 @@ public synchronized void addActivity(final @NotNull Activity activity) { | |||
| if (!isFrameMetricsAggregatorAvailable()) { | |||
| return; | |||
| } | |||
| frameMetricsAggregator.reset(); | |||
There was a problem hiding this comment.
Is it possible we have one activity alive with a transaction happening at the time a new one kicks off? Where the reset breaks reporting the values for the first activity?
There was a problem hiding this comment.
As far as I know you can only have one activity at a time. So for transactions that use ActivityFramesTracker there should not be more than one. But maybe @romtsn or @marandaneto know better.
There was a problem hiding this comment.
Actually I think this could be a problem if users disable auto-finish of the activity transactions and finish them manually in onStop. Because when you launch a second Activity, the lifecycle will be like this:
ActivityA->onPause
ActivityB->onCreate
ActivityB->onStart
ActivityB->onResume
ActivityA->onStop
so in this case when the ActivityA transaction gets finished in onStop it will have no metrics (or maybe even the wrong metrics from the ActivityB)
Needs to be tested though, just my assumption based on https://developer.android.com/guide/components/activities/activity-lifecycle#coordinating-activities
There was a problem hiding this comment.
Hmm need to think some more here. Our other solution was to do the reset after retrieving the counts.
Then this would also cause issues:
ActivityA->onPause
ActivityB->onCreate
ActivityB->onStart [frames should start counting here]
ActivityB->onResume
ActivityA->onStop [reset happens; losing counts from ActivityB as well]
So counts wouldn't be accurate.
Yet another approach was to take snapshots
ActivityA->onPause
ActivityB->onCreate [start snapshot for ActivityB happens here]
ActivityB->onStart
ActivityB->onResume
ActivityA->onStop [stop snapshot for ActivityA happens here]
When can we call reset here and not lose any counts?
There was a problem hiding this comment.
Maybe this needs to be a multi-step collection process so we get to call reset somewhere?
There was a problem hiding this comment.
After some quick testing:
It stores counts in a SparseIntArray thus only adding an entry for each ms duration value that any frame took (including a count of many frames fell in that time range). As time goes by this means we'll probably have every possible ms length value in the array. For very long transactions (e.g. if a user stayed on a screen for a very long time) and long usage periods on the app this might actually cause problems as we'll have more and more values in the array. So this alone doesn't feel like the right approach to me. Ideally I'd like to get a reset in there somewhere. That's why I suggested we break the tracking apart. Question is if this is worth the effort to invest here if we should rather go with a different appraoch based on Choreographer which we could then also use to track UI events etc.
There was a problem hiding this comment.
yeah feels like custom framecallback approach is the most viable here. We could also persist intermediate counts for ActivityB, so they don't get lost after calling reset, and then sum them up with the new counts after the reset, but if the Choreographer approach is a silver bullet for them all, I'd go for it probably
There was a problem hiding this comment.
After having a call about this we decided to move forward with calculating the delta value and keep using a single frameMetricsAggregator. If that causes problems we can either use one frameMetricsAggregator per transaction or Activity or investigate e.g. jank stats.
There was a problem hiding this comment.
I agree that calculating the delta with a single frameMetricsAggregator should be fine. The distribution of frames should be around 16 ms. I don't expect the SparseIntArray to get too big. Even if we have 1000 different frame durations, which is highly unlikely, we only need 1000 * 32 * 2 bits ~ 8 KiB. We never called reset until now, and it seemed not to cause a memory issue.
There was a problem hiding this comment.
@philipphofmann yeah the case I was worried about was more the one where it tracks until onActivityStopped which might me significantly longer.
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 2f079a1 | 296.91 ms | 337.43 ms | 40.51 ms |
| 3d89dea | 345.59 ms | 364.06 ms | 18.47 ms |
| 3d89dea | 322.38 ms | 350.82 ms | 28.45 ms |
| 1e4690d | 354.69 ms | 387.88 ms | 33.19 ms |
| c5ccd8a | 329.98 ms | 365.52 ms | 35.54 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 2f079a1 | 1.74 MiB | 2.33 MiB | 605.56 KiB |
| 3d89dea | 1.74 MiB | 2.33 MiB | 604.92 KiB |
| 3d89dea | 1.74 MiB | 2.33 MiB | 604.92 KiB |
| 1e4690d | 1.74 MiB | 2.33 MiB | 604.92 KiB |
| c5ccd8a | 1.74 MiB | 2.33 MiB | 607.44 KiB |
Codecov ReportBase: 80.07% // Head: 80.07% // No change to project coverage 👍
Additional details and impacted files@@ Coverage Diff @@
## main #2269 +/- ##
=========================================
Coverage 80.07% 80.07%
Complexity 3415 3415
=========================================
Files 241 241
Lines 12636 12636
Branches 1697 1697
=========================================
Hits 10118 10118
Misses 1876 1876
Partials 642 642 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
|
|
Closed in favor of #2271 |
📜 Description
Fixes the wrong reporting of frames by resetting the count via
onActivityStarted.💡 Motivation and Context
Fixes #2264
💚 How did you test it?
Played around with our sample app. Looks like it's working. Only saw some occurrences where the reset was logged before extracting the number of frames but that was on a total frame count of 0 so instantly done. Might have just been the logger printing the messages in wrong order. On total frame counts > 0 i never saw a reversed order.
📝 Checklist
🔮 Next steps