Skip to content

RUMM-3356 fix jankstats#1512

Merged
xgouchet merged 3 commits into
developfrom
xgouchet/RUMM-3356/fix_jankstats
Jul 3, 2023
Merged

RUMM-3356 fix jankstats#1512
xgouchet merged 3 commits into
developfrom
xgouchet/RUMM-3356/fix_jankstats

Conversation

@xgouchet

@xgouchet xgouchet commented Jun 29, 2023

Copy link
Copy Markdown
Contributor

What does this PR do?

This PR aims at solving some issues found when using the JankStats tool to track Mobile Vitals

  • Delay the initialization to onActivityStarted(): before that the window is not fully created and the JankStats creation would often fail
  • Add some tracking of the active windows: several activities could theoretically share the same window. This PR tracks this to avoid registering more than one listener to the same window
  • Furthermore, we now listen to onActivityStopped() to prevent keeping a listener on a Window not being used anymore.

Additional Notes

  • Since the JankStats doesn't measure the actual frame rate, but the time needed to draw the frame, scaling by the device's maximum frame rate doesn't make sense anymore.
  • A custom view in the sample app is added to illustrate what a slow framerate can look like; it uses a random image generation (avoids possible optimization by the compiler or JIT), and performs a lot of bitmap operation that makes it very slow.

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • Make sure you discussed the feature or bugfix with the maintaining team in an Issue
  • Make sure each commit and the PR mention the Issue number (cf the CONTRIBUTING doc)

@xgouchet
xgouchet requested a review from a team as a code owner June 29, 2023 15:35
@xgouchet
xgouchet force-pushed the xgouchet/RUMM-3356/fix_jankstats branch 3 times, most recently from 719411f to 627b446 Compare June 30, 2023 06:48
@xgouchet
xgouchet force-pushed the xgouchet/RUMM-3356/fix_jankstats branch from 627b446 to 492cbe8 Compare June 30, 2023 07:06
@codecov-commenter

codecov-commenter commented Jun 30, 2023 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #1512 (d22aeed) into develop (a982b35) will decrease coverage by 0.04%.
The diff coverage is 80.36%.

@@             Coverage Diff             @@
##           develop    #1512      +/-   ##
===========================================
- Coverage    82.00%   81.96%   -0.04%     
===========================================
  Files          363      364       +1     
  Lines        12981    13018      +37     
  Branches      2162     2168       +6     
===========================================
+ Hits         10645    10670      +25     
- Misses        1679     1685       +6     
- Partials       657      663       +6
Impacted Files Coverage Δ
...lin/com/datadog/android/rum/internal/RumFeature.kt 92.48% <ø> (-1.55%) ⬇️
...g/android/rum/internal/vitals/JankStatsProvider.kt 10.00% <10.00%> (ø)
...ernal/vitals/JankStatsActivityLifecycleListener.kt 92.45% <95.65%> (+12.45%) ⬆️

... and 14 files with indirect coverage changes

0xnm
0xnm reviewed Jun 30, 2023
View reviewed changes

@0xnm 0xnm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nice work, thanks for the help! I added few comments/suggestions.

Comment thread ...est/kotlin/com/datadog/android/rum/internal/vitals/JankStatsActivityLifecycleListenerTest.kt
}

@Test
fun `𝕄 foNothing 𝕎 onActivityStopped() {}`() {

@0xnm 0xnm Jun 30, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
fun `𝕄 foNothing 𝕎 onActivityStopped() {}`() {
fun `𝕄 do nothing 𝕎 onActivityStopped() {}`() {

Comment thread detekt_custom.yml Outdated
- "kotlin.collections.List.toSet()"
- "kotlin.collections.List.withIndex()"
- "kotlin.collections.Map.containsKey(kotlin.String)"
- "kotlin.collections.MutableMap.containsKey(android.view.Window)"

@0xnm 0xnm Jun 30, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

probably should be placed below, around line 673.

Comment thread ...rc/main/kotlin/com/datadog/android/rum/internal/vitals/JankStatsActivityLifecycleListener.kt
InternalLogger.Target.MAINTAINER,
"Disabling jankStats for window $window"
)
activeWindowsListener[window]?.isTrackingEnabled = false

@0xnm 0xnm Jun 30, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm wondering if we should use WeakHashMap for activeActivities and activeWindowsListener, in case if the window was temporary and will be destroyed once a particular activity is destroyed.

I can see that PolicyManager.makeNewWindow can be called from different places, Dialog constructor included. So if there is a dialog shown and then destroyed, we will be keeping Window instance in the memory forever, preventing it from be GCed.

@xgouchet
xgouchet requested a review from 0xnm July 3, 2023 07:34
0xnm
0xnm approved these changes Jul 3, 2023
View reviewed changes

@0xnm 0xnm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm!

@xgouchet
xgouchet merged commit bb5c9ed into develop Jul 3, 2023
@xgouchet
xgouchet deleted the xgouchet/RUMM-3356/fix_jankstats branch July 3, 2023 09:14
@xgouchet xgouchet added this to the 2.0.0 milestone Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@0xnm 0xnm 0xnm approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@xgouchet @codecov-commenter @0xnm