Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

Investigate performance impact of thumbnails with the tab gesture switcher on toolbar #13763

Closed
jonalmeida opened this issue Aug 12, 2020 · 11 comments
Assignees
Labels
performance Possible performance wins pin Issues, features, improvements that are still valid

Comments

@jonalmeida
Copy link
Contributor

jonalmeida commented Aug 12, 2020

We want to measure what is the memory impact of the thumbnail images loaded into memory for the tab switcher that uses them.

Questions:

  • Are we loading them all into memory? (Probably not; still worth verifying)
  • Are we unloading a thumbnail when the view is lost?
  • What is the memory size of each thumbnail (relative to the device)?
  • Can we reduce the quality to reduce the memory pressure? We have the API to do this today, so Iif yes, by how much?

cc: @person808

┆Issue is synchronized with this Jira Task

@jonalmeida jonalmeida added the performance Possible performance wins label Aug 12, 2020
@github-actions github-actions bot added the needs:triage Issue needs triage label Aug 12, 2020
@person808
Copy link
Contributor

Related: #13411

Are we loading them all into memory? (Probably not; still worth verifying)

We reuse the same TabPreview so we shouldn't be, unless loadIntoView is doing something I'm not aware of.

Are we unloading a thumbnail when the view is lost?

Don't think so. We set the visibility of the TabPreview to GONE when the gesture is done, but I think that still leaves the image loaded in memory.

Can we reduce the quality to reduce the memory pressure? We have the API to do this today, so Iif yes, by how much?

Chrome uses noticeably worse quality for their implementation of the gesture but I didn't notice until it was pointed out to me, so we can probably get away with a lower quality here. Its easy to see the quality difference on Hacker News since its all text.

@jonalmeida
Copy link
Contributor Author

jonalmeida commented Aug 12, 2020

Thanks!

For higher density screens, a thumbnail can be multiple MB in size so if we can unload them when unneeded, we can reduce our probability of the OS trying to kill us.

Chrome's thumbnail quality is what we were comparing against as well, and I had the same sentiment as you! If we can't visually notice the quality difference we can try to reduce it then and see how much we really save.

@liuche liuche removed the needs:triage Issue needs triage label Aug 17, 2020
@mcomella
Copy link
Contributor

Triage: suggested short term action items from the performance team:

  • Check size of thumbnails on high resolution device and low resolution device
  • Identify the number of simultaneous thumbnails in memory
  • If it's concerning (e.g. extremely large thumbnails or extremely large number of thumbnails such as one per tab), address the issue

If it's not concerning, we can probably deprioritize this until later.

@jonalmeida
Copy link
Contributor Author

jonalmeida commented Aug 28, 2020

When investigating mozilla-mobile/android-components#7304, I loaded 150 tabs with thumbnails and only one of them dropped the thumbnail by going over the LRU disk cache which is a fixed size of 150MB on all devices.

Pixel 4 with Android 10.
Nightly 200828 06:01 (Build #2015760563)
AC: 57.0.20200826190111, c93fc20af
GV: 81.0a1-20200820093209


  • Identify the number of simultaneous thumbnails in memory

The calculated amount without the tabs tray open, should be 1-3 high resolutions of the most recent thumbnails depending on memory pressure from the system - when it notifies us, we drop the thumbnails. This is fine because we store them immediately upon receiving new thumbnails and load it back up when the tabs tray needs it. We always use the in-memory one first before deciding to use one from disk.

The calculated amount with the tabs tray, should be much smaller since we request a thumbnail for the max dimensions of what is visible.

@mcomella
Copy link
Contributor

mcomella commented Sep 1, 2020

I loaded 150 tabs with thumbnails and only one of them dropped the thumbnail by going over the LRU disk cache which is a fixed size of 150MB on all devices.

@jonalmeida What's in the LRU disk cache? Is it just thumbnails? I wonder what all the caches we have are and if we're coordinating to ensure they don't grow uncontrollably. Note: we have telemetry to check the app/cache size but don't have an owner to analyze it. See https://jira.mozilla.com/browse/FXP-1237

The calculated amount without the tabs tray open,

It sounds like these are loaded from disk when they're needed rather than being in memory all the time. If so, this sounds fine from a RAM perspective. Let's continue the discussion about the disk.

@jonalmeida
Copy link
Contributor Author

jonalmeida commented Sep 1, 2020

@jonalmeida What's in the LRU disk cache? Is it just thumbnails? I wonder what all the caches we have are and if we're coordinating to ensure they don't grow uncontrollably. Note: we have telemetry to check the app/cache size but don't have an owner to analyze it. See https://jira.mozilla.com/browse/FXP-1237

Yes, this is a cache specifically for thumbnails. It does not fall under the app data size. It cannot grow larger the max value, so there shouldn't be cause for concern in that situation, but we can change it's size if we have reasonable data to suggest we should make it smaller and/or consolidate it with others. So far, with my 150 tabs, my cache size is only 55MB; app size unchanged.

@jonalmeida
Copy link
Contributor Author

The most actionable option we can/should look at is:

Can we reduce the quality to reduce the memory pressure? We have the API to do this today, so Iif yes, by how much?

We can see how much memory we can reduce even further, if needed.

@liuche
Copy link
Contributor

liuche commented Sep 18, 2020

possibly related: #13907 ? (This will go into the sourpatch sprint next week, unless you have more info/think you should take it)

@jonalmeida
Copy link
Contributor Author

possibly related: #13907 ? (This will go into the sourpatch sprint next week, unless you have more info/think you should take it)

I'm not sure what this issue is referring to. There is a good discussion in this ticket that can move forward for engineering. Feel free to ping me if anything is unclear.

@person808
Copy link
Contributor

@jonalmeida IIRC that's referring to using ViewStub to inflate the TabPreview only when the gesture is actually used. It's not really related to the memory usage of the thumbnails in this issue.

@jonalmeida jonalmeida self-assigned this Sep 28, 2020
@gabrielluong gabrielluong added the pin Issues, features, improvements that are still valid label Jan 12, 2021
@MarcLeclair
Copy link
Contributor

Close: we can reopen if we want to investigate the performance of thumbnails with gesture again.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
performance Possible performance wins pin Issues, features, improvements that are still valid
Projects
None yet
Development

No branches or pull requests

6 participants