REPLAY-1727: Base64 Caching Mechanism by jonathanmos · Pull Request #1534 · DataDog/dd-sdk-android

jonathanmos · 2023-07-10T08:13:36Z

What does this PR do?

Adds an LRUCache for base64 strings - cache base64 strings against drawable hashcodes.
Evict all items in the LRUCache every time we change views.

Motivation

Without a caching mechanism we create and garbage collect an excessive number of bitmaps.

Additional Notes

This PR contains a revert commit of the revert of imagebuttonmapper (iow it adds base64 functionality back)

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)

codecov-commenter · 2023-07-10T09:06:32Z

Codecov Report

Attention: Patch coverage is 78.31325% with 18 lines in your changes missing coverage. Please review.

Project coverage is 83.32%. Comparing base (ee5007f) to head (9d3d7a8).

❗ Current head 9d3d7a8 differs from pull request most recent head 257722b

Please upload reports for the commit 257722b to get more accurate results.

Files with missing lines	Patch %	Lines
...nreplay/internal/recorder/base64/Base64LRUCache.kt	71.43%	12 Missing ⚠️
...eplay/internal/recorder/base64/Base64Serializer.kt	82.76%	4 Missing and 1 partial ⚠️
...play/internal/recorder/mapper/ImageButtonMapper.kt	75.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@                      Coverage Diff                      @@
##           feature/base64-jmoskovich    #1534      +/-   ##
=============================================================
- Coverage                      83.66%   83.32%   -0.34%     
=============================================================
  Files                            438      439       +1     
  Lines                          14854    14927      +73     
  Branches                        2237     2250      +13     
=============================================================
+ Hits                           12427    12437      +10     
- Misses                          1868     1897      +29     
- Partials                         559      593      +34

Files with missing lines	Coverage Δ
...adog/android/sessionreplay/SessionReplayPrivacy.kt	`100.00% <100.00%> (ø)`
...nreplay/internal/async/RecordedDataQueueHandler.kt	`89.39% <100.00%> (+0.87%)`	⬆️
...ay/internal/recorder/mapper/BaseWireframeMapper.kt	`78.57% <100.00%> (+0.79%)`	⬆️
...play/internal/recorder/mapper/ImageButtonMapper.kt	`91.43% <75.00%> (+7.05%)`	⬆️
...eplay/internal/recorder/base64/Base64Serializer.kt	`88.41% <82.76%> (-1.39%)`	⬇️
...nreplay/internal/recorder/base64/Base64LRUCache.kt	`71.43% <71.43%> (ø)`

... and 16 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

mariusc83

We need to test and register that callback

0xnm

nice idea! I've added some comments to discuss.

0xnm · 2023-07-10T14:55:18Z

+    }
+
+    @VisibleForTesting
+    internal fun setBackingCache(cache: LruCache<Int, String>) {


minor: maybe just a provider (Int) -> LruCache in the constructor with default value for prod and mock value for tests?

This reverts commit ef005ea.

0xnm · 2023-07-17T12:59:58Z

+    private const val ON_MODERATE_MEMORY_SIZE_BYTES = (MAX_CACHE_MEMORY_SIZE_BYTES / 4) * 3 // 75% size
+    private var componentCallbacksRegistered = false
+
+    private var cache: LruCache<String, ByteArray> = object :


minor: for testing we can remove this mutability and inject a provider, but I'm not pushing for it (just it is better to have less mutability).

mariusc83

LGTM now. Nicely done.

0xnm

I've added some questions and suggestions, but overall I think it is good to go.

0xnm · 2023-07-18T13:57:42Z

+    }
+
+    private fun getPrefixForDrawableContainer(drawable: DrawableContainer): String {
+        if (drawable !is AnimationDrawable) {


it means that the key for the AnimationDrawable will always be the same independent of the state? meaning that even state of AnimationDrawable has changed then the cache entry for the first known state will be served? Is it is an intention we want to have?

Yes, its just a fix to avoid having AnimationDrawable take up too much space in the lrucache. We'll need to have a discussion/brainstorming if we decide we want to send animations with base64, and this will in any case cease to be an issue once we have the resources endpoint.

0xnm

I added a comment about internalLogger storage, it is a potential memory leak.

mariusc83 reviewed Jul 10, 2023

View reviewed changes

jonathanmos force-pushed the jmoskovich/replay-1727/base64-caching branch from 8418b54 to 7754a16 Compare July 10, 2023 11:33

mariusc83 reviewed Jul 10, 2023

View reviewed changes

Comment thread ...src/main/kotlin/com/datadog/android/sessionreplay/internal/async/RecordedDataQueueHandler.kt

jonathanmos force-pushed the jmoskovich/replay-1727/base64-caching branch from 7754a16 to 5e57c0b Compare July 10, 2023 11:36

mariusc83 reviewed Jul 10, 2023

View reviewed changes

Comment thread ...src/main/kotlin/com/datadog/android/sessionreplay/internal/recorder/base64/Base64LRUCache.kt Outdated

jonathanmos force-pushed the jmoskovich/replay-1727/base64-caching branch from 5e57c0b to 3c431ae Compare July 10, 2023 11:54

jonathanmos commented Jul 10, 2023

View reviewed changes

Comment thread ...src/main/kotlin/com/datadog/android/sessionreplay/internal/recorder/base64/Base64LRUCache.kt Outdated

jonathanmos force-pushed the jmoskovich/replay-1727/base64-caching branch from 3c431ae to 92ba7a7 Compare July 10, 2023 12:42

jonathanmos marked this pull request as ready for review July 10, 2023 13:00

jonathanmos requested a review from a team as a code owner July 10, 2023 13:00

mariusc83 requested changes Jul 10, 2023

View reviewed changes

Comment thread ...c/main/kotlin/com/datadog/android/sessionreplay/internal/recorder/base64/Base64Serializer.kt Outdated

Comment thread ...src/main/kotlin/com/datadog/android/sessionreplay/internal/recorder/base64/Base64LRUCache.kt Outdated

mariusc83 requested changes Jul 10, 2023

View reviewed changes

0xnm reviewed Jul 10, 2023

View reviewed changes

xgouchet reviewed Jul 11, 2023

View reviewed changes

Comment thread ...src/main/kotlin/com/datadog/android/sessionreplay/internal/recorder/base64/Base64LRUCache.kt Outdated

Revert "Temporarily remove imagebutton mapper"

b25e9a4

This reverts commit ef005ea.

jonathanmos force-pushed the jmoskovich/replay-1727/base64-caching branch from 57d6d54 to af3259a Compare July 11, 2023 14:11

jonathanmos changed the base branch from develop to feature/base64-jmoskovich July 11, 2023 14:12

xgouchet approved these changes Jul 17, 2023

View reviewed changes

jonathanmos force-pushed the jmoskovich/replay-1727/base64-caching branch from a9c7810 to 69e1ffe Compare July 17, 2023 11:15

xgouchet self-requested a review July 17, 2023 11:25

jonathanmos force-pushed the jmoskovich/replay-1727/base64-caching branch from 69e1ffe to 3e9b47a Compare July 17, 2023 11:33

jonathanmos requested review from 0xnm and mariusc83 July 17, 2023 11:54

0xnm reviewed Jul 17, 2023

View reviewed changes

mariusc83 reviewed Jul 18, 2023

View reviewed changes

Comment thread ...src/main/kotlin/com/datadog/android/sessionreplay/internal/recorder/base64/Base64LRUCache.kt Outdated

Comment thread ...n-replay/src/main/kotlin/com/datadog/android/sessionreplay/internal/recorder/base64/Cache.kt Outdated

mariusc83 approved these changes Jul 18, 2023

View reviewed changes

jonathanmos requested a review from 0xnm July 18, 2023 13:31

0xnm reviewed Jul 18, 2023

View reviewed changes

0xnm reviewed Jul 19, 2023

View reviewed changes

jonathanmos force-pushed the jmoskovich/replay-1727/base64-caching branch 4 times, most recently from 1f11ad4 to 9d3d7a8 Compare July 19, 2023 10:21

jonathanmos requested a review from 0xnm July 19, 2023 11:26

0xnm approved these changes Jul 19, 2023

View reviewed changes

Implement caching mechanism for base64

257722b

jonathanmos force-pushed the jmoskovich/replay-1727/base64-caching branch from 9d3d7a8 to 257722b Compare July 19, 2023 11:45

jonathanmos merged commit f8cc3e6 into feature/base64-jmoskovich Jul 19, 2023

jonathanmos deleted the jmoskovich/replay-1727/base64-caching branch July 19, 2023 12:04

xgouchet added this to the 2.2.0 milestone Dec 13, 2023

Uh oh!

Conversation

jonathanmos commented Jul 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Jul 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mariusc83 left a comment

Choose a reason for hiding this comment

Uh oh!

0xnm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

0xnm Jul 10, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

0xnm Jul 17, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mariusc83 left a comment

Choose a reason for hiding this comment

Uh oh!

0xnm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

0xnm Jul 18, 2023

Choose a reason for hiding this comment

Uh oh!

jonathanmos Jul 19, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

0xnm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

jonathanmos commented Jul 10, 2023 •

edited

Loading

codecov-commenter commented Jul 10, 2023 •

edited

Loading