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

RUM-4561 Delegate Drawable copy to background thread #2048

Merged
merged 2 commits into from
May 23, 2024

Conversation

xgouchet
Copy link
Member

What does this PR do?

Our previous mechanism of copying a drawable's content was performed on the main thread.
When starting an app, this would create a lot of main thread task to run. And any new image would also take some of the main thread time, creating a lot of jank frames in any non hello world app.

This will fix the performance issue some of our customers started to notice when enabling SR.

@xgouchet xgouchet requested review from a team as code owners May 22, 2024 17:56
@codecov-commenter
Copy link

codecov-commenter commented May 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.02%. Comparing base (daec9a0) to head (efb6200).

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2048      +/-   ##
===========================================
- Coverage    83.10%   83.02%   -0.09%     
===========================================
  Files          494      494              
  Lines        17720    17688      -32     
  Branches      2686     2684       -2     
===========================================
- Hits         14726    14684      -42     
- Misses        2253     2259       +6     
- Partials       741      745       +4     
Files Coverage Δ
...nreplay/internal/recorder/SessionReplayRecorder.kt 96.61% <100.00%> (+0.09%) ⬆️
...ssionreplay/internal/recorder/TreeViewTraversal.kt 95.92% <100.00%> (+0.80%) ⬆️
...roid/sessionreplay/internal/utils/DrawableUtils.kt 93.65% <100.00%> (ø)

... and 30 files with indirect coverage changes

@@ -76,6 +81,14 @@ internal class TreeViewTraversal(
)
}

val resolvedWireframes = internalLogger.measureMethodCallPerf(
javaClass,
Copy link
Member

Choose a reason for hiding this comment

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

Similar to what was done with WindowsOnDrawListener, do we need to also add this class to proguard rules to prevent classname obfuscation?

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense indeeed

@xgouchet xgouchet changed the title Delegate Drawable copy to background thread RUM-4561 Delegate Drawable copy to background thread May 23, 2024
@xgouchet xgouchet mentioned this pull request May 23, 2024
3 tasks
@@ -59,8 +58,8 @@ internal class DrawableUtils(
resizeBitmapCallback = object :
ResizeBitmapCallback {
override fun onSuccess(bitmap: Bitmap) {
mainThreadHandler.post {
@Suppress("ThreadSafety") // this runs on the main thread
executorService.submitSafe("drawOnCanvas", internalLogger) {
Copy link
Member

Choose a reason for hiding this comment

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

is it safe to operate with bitmap/canvas from the worker thread on all Android versions?

Copy link
Member Author

Choose a reason for hiding this comment

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

From the tests I've done, it seems so. I'll do more test to ensure it works on all levels.

Copy link
Member Author

Choose a reason for hiding this comment

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

It does work on all versions 👍

@xgouchet xgouchet merged commit de8989f into develop May 23, 2024
20 checks passed
@xgouchet xgouchet deleted the xgouchet/sr_performance_mutable_list branch May 23, 2024 08:53
@xgouchet xgouchet added this to the 2.11.x milestone Jul 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants