[Vertical] Icons fetched by shared/gaia_grid/js/icon_retriever.js are cached in idb forever

RESOLVED FIXED

Status

Firefox OS
Gaia::Homescreen
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: lightsofapollo, Assigned: lightsofapollo)

Tracking

unspecified
x86
Mac OS X
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.0+, b2g-v2.0 fixed, b2g-v2.1 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
Any consumer of gaia_grid will grow its async_storage database indefinitely as we add icons to the cache whenever it is displayed but we never remove them from the cache.

This effects not only the system app but any other apps which use the gaia_grid webcomponent.
(Assignee)

Comment 1

3 years ago
The logic in the icon_retriever (the one used on the vertical home) could use some reworking... We always hit http first before checking the cache. This is the correct behavior for app:// protocol fetches (fetching the blob via xhr takes 1-2s vs 5-6s in the idb async storage cache) but is incorrect for hosted apps which take an extra 1s prior to hitting the cache.

We can improve this logic by:

  - just hitting packaged apps directly via xhr rather then going through idb (never cache)
  - try http and the cache in parallel for hosted apps
(Assignee)

Comment 2

3 years ago
The sensible thing here is to probably store the blob of the icon (when needed for caching) along with .detail in the grid_item classes. Unless I am mistaken it's safe to hold a reference to the blob (without keeping its memory alive).
(Assignee)

Comment 3

3 years ago
Meaning we can ditch async_storage which has it's own secondary idb database with its own overhead... loading the first icon out of the cache should then be faster since we initialize the "item" store anyway to figure out where stuff is on the homescreen.
(Assignee)

Comment 4

3 years ago
^ the above makes perfect sense for the homescreen but not for anywhere else (since we don't have these centralized store things across all the apps) which makes a LRU cache (based on when the item was last accessed) make more sense. Maybe there is something in the platform which can/does cache this for us so we don't need to ourselves.
(Assignee)

Updated

3 years ago
User Story: (updated)
(Assignee)

Comment 5

3 years ago
Created attachment 8437517 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/20199

Detailed notes in github about what this does... This also should fix the following bugs:

https://bugzilla.mozilla.org/show_bug.cgi?id=1022098
https://bugzilla.mozilla.org/show_bug.cgi?id=1022497

In theory performance should be the same or better but I have not done a solid perf run yet on verticalhome (I will this week once I have my symbols and a linux box?)
Attachment #8437517 - Flags: review?(kgrandon)
Attachment #8437517 - Flags: feedback?(crdlc)
(Assignee)

Updated

3 years ago
Blocks: 1022098, 1022497
(Assignee)

Comment 6

3 years ago
Comment on attachment 8437517 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/20199

Moving both to f? This is not quite ready to land yet but is good enough for a first pass for approach... I need an additional test to verify the use of the cached blob (which I know is not working correct as it is not passed by all items from the db right now).
Attachment #8437517 - Flags: review?(kgrandon) → feedback?(kgrandon)
Comment on attachment 8437517 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/20199

I am very happy with this great job (congrats!!!) and also we are in the same page because if you see my pr to cancel and resume downloads you can see how we have taken the same solution :)

https://github.com/mozilla-b2g/gaia/pull/20234

Just a comment but this is a question of tastes thought. I would like that icons were clever and they would know to retry after fetching an icon and it fails instead of app.js. But it is just my idea and I could be completely wrong in my assumption.
Attachment #8437517 - Flags: feedback?(crdlc) → feedback+
Assignee: nobody → jlal
Status: NEW → ASSIGNED
QA Whiteboard: [VH-FC-blocking-]
(Assignee)

Comment 8

3 years ago
> would know to retry after fetching an icon and it fails instead of app.js. But it is just my idea and I could be completely wrong in my assumption.

Hrm- Is this just a design decision or do we want to issue icon retries on search/collections app?
Comment on attachment 8437517 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/20199

I like it. Seems very clean and easy to understand. Nice work.
Attachment #8437517 - Flags: feedback?(kgrandon) → feedback+
(Assignee)

Updated

3 years ago
Attachment #8437517 - Flags: review?(kgrandon)
Comment on attachment 8437517 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/20199

Continuing to look over the pull request, let's just make sure we get the icon rounding issue solved before landing.
Attachment #8437517 - Flags: review?(kgrandon) → review+
(Assignee)

Comment 11

3 years ago
https://github.com/mozilla-b2g/gaia/commit/5438f4773400a77f4d04f9e120a3c488c699ef7b
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Blocks two blockers - blocking+
blocking-b2g: --- → 2.0+
QA Whiteboard: [VH-FC-blocking-] → [VH-FC-blocking+]
I believe this was uplifted as: https://github.com/mozilla-b2g/gaia/commit/d016f4aa9add440c244ce3c3f615cac82101031a
status-b2g-v2.0: --- → fixed
status-b2g-v2.1: --- → fixed
You need to log in before you can comment on or make changes to this bug.