Use soft references to hold sprites #297
Conversation
There was a problem hiding this comment.
This looks good.
I can fix the style changes.
@rubengees can you also have a look at the changes here, please?
| sheet = BitmapFactory.decodeResource(context.getResources(), R.drawable.emoji_google_sheet); | ||
| strip = (Bitmap) STRIP_REFS[x].get(); | ||
| if (strip == null) { | ||
| Log.i(getClass().getSimpleName(), "Loading strip " + x); |
There was a problem hiding this comment.
do we need this logging here?
There was a problem hiding this comment.
This can be removed, it's just to get a picture of what's happening while testing the branch.
|
Sure, I'll have a look this weekend.
|
|
Added two commits to fix style errors and remove logging. |
|
Testes this PR on production, and it fixes the OOM issue. Hats off, @akwizgran! 🎩 |
generator/template/Emoji.java
Outdated
| final Bitmap strip = loadStrip(context); | ||
| final Bitmap cut = Bitmap.createBitmap(strip, 1, | ||
| y * SPRITE_SIZE_INC_BORDER + 1, SPRITE_SIZE, SPRITE_SIZE); | ||
| BITMAP_CACHE.put(key, cut); |
There was a problem hiding this comment.
Have you tested the performance without this second cache? I could imagine that cutting the image whenever requested would make no big difference (since the stripes are somewhat small) and that would also help with memory pressure.
There was a problem hiding this comment.
I added the LRU cache because otherwise it was possible to trigger a lot of loading and unloading of strips when the app was under memory pressure. For example, I'd be looking at a conversation with a few emoji visible, the GC would run and unload the strips backing the emoji, then the next time the conversation was redrawn, the strips would be reloaded. I didn't do any rigorous performance testing, but my guess was that this repeated loading was probably worse for performance overall than keeping a few recently used sprites in memory.
If you'd like to do a proper performance comparison, the version without the LRU cache is here: https://github.com/akwizgran/Emoji/tree/sprite-strips-soft-refs
There was a problem hiding this comment.
I see! I'll compare both solutions.
generator/template/Emoji.java
Outdated
| if (sheet != null) { | ||
| private Bitmap loadStrip(final Context context) { | ||
| Bitmap strip = (Bitmap) STRIP_REFS[x].get(); | ||
| if (strip == null) { | ||
| synchronized (LOCK) { |
There was a problem hiding this comment.
If you synchronize getDrawable, the synchronization here can be removed.
|
@akwizgran will you address the feedback? |
|
@vanniktech sorry for the delay, I'll work on this tomorrow. |
|
I've addressed the review comments but haven't had time to re-test the branch. I'll do that on Monday. |
This is a false positive due to the resource ID being generated dynamically.
Codecov Report
@@ Coverage Diff @@
## master #297 +/- ##
==========================================
- Coverage 26.01% 25.84% -0.18%
==========================================
Files 24 25 +1
Lines 911 917 +6
Branches 99 99
==========================================
Hits 237 237
- Misses 655 661 +6
Partials 19 19
Continue to review full report at Codecov.
|
|
What's unused? I'll take a closer look at this later / tomorrow. |
|
I've tested the branch and fixed a lint failure caused by dynamically generating the resource IDs. |
Lint considers the sprite strips unused because their resource IDs don't appear in the code. This is a false positive because the resource ID is generated dynamically based on the x coordinate of the sprite. |
There was a problem hiding this comment.
Looks good from my side. Tested it out quickly and it worked nicely.
I'll ask for another review from @rubengees
|
Thank you so much for this. I'll leave this in master for a while and hopefully we'll get some feedback. |
|
I've been using this since a few days ago and it's working very well so far. Thank you all! |
This branch modifies the generator to cut the sprite sheets into strips that can be loaded separately. Each strip is held by a soft reference so it's released when the app is under memory pressure. To avoid reloading a whole strip each time an emoji needs to be displayed, the most recently used bitmaps are kept in an LRU cache.
Please test the memory usage and loading speed under common scenarios, such as scrolling through the emoji chooser or reading a conversation while the app is under memory pressure. I've chosen an arbitrary size for the LRU cache, but a smaller or larger cache may offer a better time/space tradeoff.