Closed Bug 1022247 Opened 6 years ago Closed 6 years ago

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


(Firefox OS Graveyard :: Gaia::Homescreen, defect)

Not set


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

blocking-b2g 2.0+
Tracking Status
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed


(Reporter: jlal, Assigned: jlal)




(1 file)

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.
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
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).
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.
^ 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.
User Story: (updated)
Detailed notes in github about what this does... This also should fix the following bugs:

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)
Blocks: 1022098, 1022497
Comment on attachment 8437517 [details] [review]

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]

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 :)

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
QA Whiteboard: [VH-FC-blocking-]
> 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]

I like it. Seems very clean and easy to understand. Nice work.
Attachment #8437517 - Flags: feedback?(kgrandon) → feedback+
Attachment #8437517 - Flags: review?(kgrandon)
Comment on attachment 8437517 [details] [review]

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+
Closed: 6 years ago
Resolution: --- → FIXED
Blocks two blockers - blocking+
blocking-b2g: --- → 2.0+
QA Whiteboard: [VH-FC-blocking-] → [VH-FC-blocking+]
You need to log in before you can comment on or make changes to this bug.