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

Jump to bottom

Thumbnail changes update the tabs tray too often #7313

Closed
jonalmeida opened this issue Jun 9, 2020 · 2 comments · Fixed by #7356
Closed

Thumbnail changes update the tabs tray too often #7313

jonalmeida opened this issue Jun 9, 2020 · 2 comments · Fixed by #7356
Assignees
@jonalmeida
Labels
🐞 bug performance <tabs> <thumbnails>
Projects
Tabs Tray A-C: Website thumbnails Performance, front-end roadmap
Milestone
46.0.0 🏄‍♂️

Comments

@jonalmeida
Copy link
Contributor

@jonalmeida jonalmeida commented Jun 9, 2020 •
edited by data-sync-user

When we update the tabs tray, we use the generationId from the bitmap to identify if the thumbnails have changed. It turns out that these are not stable (probably because we're compressing the images so the ID changes).

We need a better way to avoid the whole tray from updating when we have a thumbnail update.

┆Issue is synchronized with this Jira Task
@jonalmeida jonalmeida added 🐞 bug <tabs> <thumbnails> labels Jun 9, 2020
@jonalmeida jonalmeida self-assigned this Jun 9, 2020
@gabrielluong gabrielluong added this to Triage in Tabs Tray via automation Jun 9, 2020
@gabrielluong gabrielluong moved this from Triage to In Progress in Tabs Tray Jun 9, 2020
@jonalmeida
Copy link
Contributor Author

@jonalmeida jonalmeida commented Jun 9, 2020 •
edited

Possible duplicates/fallout:

@jonalmeida jonalmeida mentioned this issue Jun 10, 2020
Open
@jonalmeida
Copy link
Contributor Author

@jonalmeida jonalmeida commented Jun 11, 2020 •
edited

I verified that removing the generationId from Tabs.equals causes the multiple refreshes.

In mozilla-mobile/fenix#11185 (comment), I mentioned we should try to use the loadToView mechanism to show the icons safely. I now think that's probably the best way forward for this problem too. It would solve:

  • The initial case where you want to load the initial image from the disk.
  • We can remove the use case mapping from the TabsTrayPresenter
  • For thumbnail updates, we forward the new thumbnails into the presenter and save them async for the next time we need to load fresh thumbnails from disk.

What changes though is the surface area of this API for the consumer to load the thumbnails where ever the caller needs.

More concretely, the ThumbnailsStorage should add a method like loadIntoView and call that on the viewholder when binding, and update the viewholder as well if new changes come from the store.

@jonalmeida jonalmeida added this to In progress in A-C: Website thumbnails Jun 11, 2020
jonalmeida added a commit to jonalmeida/android-components that referenced this issue Jun 12, 2020
@jonalmeida
Close mozilla-mobile#7313: Use storage/loader to get thumbnails
b16595f 
We were seeing odd bugs and performance issues from trying to map the
disk cache into the TabsTrayPresenter.

A better solution, would be to load the thumbnails straight from the
cache, and incremental updates via the store.
jonalmeida added a commit to jonalmeida/android-components that referenced this issue Jun 12, 2020
@jonalmeida
Issue mozilla-mobile#7313: Update sample browser to use thumbnail loader
ba8bdef
jonalmeida added a commit to jonalmeida/android-components that referenced this issue Jun 12, 2020
@jonalmeida
Close mozilla-mobile#7313: Use storage/loader to get thumbnails
68d5518 
We were seeing odd bugs and performance issues from trying to map the
disk cache into the TabsTrayPresenter.

A better solution, would be to load the thumbnails straight from the
cache, and incremental updates via the store.
jonalmeida added a commit to jonalmeida/android-components that referenced this issue Jun 12, 2020
@jonalmeida
Issue mozilla-mobile#7313: Update sample browser to use thumbnail loader
d5d8745
jonalmeida added a commit to jonalmeida/android-components that referenced this issue Jun 12, 2020
@jonalmeida
Issue mozilla-mobile#7313: Add changelog for tabs tray changes
265242c
@jonalmeida jonalmeida linked a pull request Jun 12, 2020 that will close this issue
Issue #7313: Use ThumbnailLoader for the TabViewHolder #7356
Merged
4 tasks
jonalmeida added a commit to jonalmeida/android-components that referenced this issue Jun 12, 2020
@jonalmeida
Close mozilla-mobile#7313: Use storage/loader to get thumbnails
f3358da 
We were seeing odd bugs and performance issues from trying to map the
disk cache into the TabsTrayPresenter.

A better solution, would be to load the thumbnails straight from the
cache, and incremental updates via the store.
jonalmeida added a commit to jonalmeida/android-components that referenced this issue Jun 12, 2020
@jonalmeida
Issue mozilla-mobile#7313: Update sample browser to use thumbnail loader
8f5daa6
jonalmeida added a commit to jonalmeida/android-components that referenced this issue Jun 12, 2020
@jonalmeida
Issue mozilla-mobile#7313: Add changelog for tabs tray changes
153bbce
jonalmeida added a commit to jonalmeida/android-components that referenced this issue Jun 12, 2020
@jonalmeida
Close mozilla-mobile#7313: Use storage/loader to get thumbnails
d078536 
We were seeing odd bugs and performance issues from trying to map the
disk cache into the TabsTrayPresenter.

A better solution, would be to load the thumbnails straight from the
cache, and incremental updates via the store.
jonalmeida added a commit to jonalmeida/android-components that referenced this issue Jun 12, 2020
@jonalmeida
Issue mozilla-mobile#7313: Update sample browser to use thumbnail loader
2d34e0b
jonalmeida added a commit to jonalmeida/android-components that referenced this issue Jun 12, 2020
@jonalmeida
Issue mozilla-mobile#7313: Add changelog for tabs tray changes
dc99982
jonalmeida added a commit to jonalmeida/android-components that referenced this issue Jun 12, 2020
@jonalmeida
Issue mozilla-mobile#7313: Update sample browser to use thumbnail loader
9c1a921
jonalmeida added a commit to jonalmeida/android-components that referenced this issue Jun 12, 2020
@jonalmeida
Issue mozilla-mobile#7313: Add changelog for tabs tray changes
1f143e1
bors bot pushed a commit that referenced this issue Jun 12, 2020
@MozLando @jonalmeida
Merge #7356
ee34a97 
7356: Issue #7313: Use ThumbnailLoader for the TabViewHolder r=gabrielluong,boek a=jonalmeida

Instead of trying to inline the thumbnail images from disk into the TabsTrayPresenter, we can load them from the `ThumbnailStorage` via the `ThumbnailLoader` and rely on the `TabsTrayPresenter` to consume new thumbnail updates only from the store.

This fixes some perf issues, inconsistencies, and duplicate updates to the tabs tray.



Co-authored-by: Jonathan Almeida <jalmeida@mozilla.com>
@bors bors bot closed this as completed in 0a95c91 Jun 12, 2020
Tabs Tray automation moved this from In Progress to Done Jun 12, 2020
A-C: Website thumbnails automation moved this from In progress to Done Jun 12, 2020
This was referenced Jun 16, 2020
Closed
Closed
Closed
Closed
@jonalmeida jonalmeida added this to the 46.0.0 🏄‍♂️ milestone Jun 16, 2020
@mcomella mcomella mentioned this issue Jun 16, 2020
Closed
@mcomella mcomella added the performance label Jun 17, 2020
@mcomella mcomella added this to Needs prioritization in Performance, front-end roadmap via automation Jun 17, 2020
@mcomella mcomella moved this from Needs prioritization to Done in Performance, front-end roadmap Jun 17, 2020
@gabrielluong gabrielluong mentioned this issue Jun 19, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jonalmeida jonalmeida

Labels
🐞 bug performance <tabs> <thumbnails>
Projects
Performance, front-end roadmap
Done
A-C: Website thumbnails
  
Done
Tabs Tray
  
Done
Milestone
46.0.0 🏄‍♂️
Development

Successfully merging a pull request may close this issue.

Issue #7313: Use ThumbnailLoader for the TabViewHolder jonalmeida/android-components
3 participants
@mcomella @gabrielluong @jonalmeida