New issue
Advanced search Search tips

Issue 769051 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Feature



Sign in to add a comment

Cache image decodes across navigations

Project Member Reported by pdr@chromium.org, Sep 26 2017

Issue description

When navigating between pages we re-decode images even if it's the same image. This is likely due to the "ClearImageCacheOnNavigation();" call.

I created a demo of this behavior: https://pr.gg/decode/decode1.html

In this demo a single BitmapImage.h object is created on the first page and is stored in blink's memory cache. This image is decoded. When navigating to the second page, the same BitmapImage.h object is re-used, but the image is decoded again.

Using cached decodes would reduce decode costs on large websites such as amazon and google.
 

Comment 1 by pdr@chromium.org, Sep 28 2017

Cc: ericrk@chromium.org
Owner: pdr@chromium.org
Status: Assigned (was: Untriaged)
We currently use the most conservative approach (memory-wise) and always clear decoded images. On the other end of the spectrum, we could retain decoded images until the Image object is removed from the memory cache. There are heuristics inbetween.

We should gather data about how often an image is used (painted) after a navigation which would give an upper bound on the usefulness of caching decoded images. If 10% of images are used again, our current approach likely makes sense. If 90% of images are used again, we should remove ClearImageCacheOnNavigation immediately. Etc..

To do this, create a navigation generation id that's incremented when Page navigation occurs. When an Image is first loaded, record the current navigation generation. When an Image is used and the current navigation generation does not match the Image's initial navigation generation, mark the Image as being used after a navigation. On Image destruction, record a UMA about whether the Image was ever used after a navigation.

Comment 2 by ericrk@chromium.org, Sep 29 2017

Good point. We actually thought about this when we did the initial clearing. Local testing indicated that even if we don't clear the decoded image cache we still can't re-use these entries as the Skia Image IDs we get from Blink change after navigation/reload. Ongoing work like Khushal's changes to image IDs may make this no longer true, in which case it's worth revisiting. We could also just do the work to make these stable, if they aren't. Not saying this isn't a good thing to pursue, but there may be more needed than just removing the cache clear.
The skia IDs used to be tied to the lifetime of blink::Image so that part wouldn't be affected by my recent changes. I wasn't sure about blink::Image lifetimes, but if that's cached and reused across navigations, then we should be able to reuse decodes in cc as well.

I have a change up to do the latter pdr@ mentioned in #1: retain decoded images until the Image object is removed (https://chromium-review.googlesource.com/c/chromium/src/+/644317). We weren't sure if its worth it, given the additional overhead/plumbing for only purging on Image destruction . But if that helps with this use-case, then may be its another reason to try it.

Comment 4 by pdr@chromium.org, Oct 4 2017

Small update: I ran into issues measuring the upper bound of decode caching. We need to correctly handle iframe image loads to get meaningful data from this metric, but with frames there is no longer a single "document generation id" for a given Page. Even if a unique stable id were available, I think the plumbing through the fragile dom->layout->paint->image pipeline is a nonstarter.

We may be able to use another metric, Blink.MemoryCache.RevalidationPolicy.Image, to get a rough idea of the benefit of this approach. This metric has two issues:
1) overcounting of image atlases and image re-use in general.
2) complexity due to requests coming from both content and the preload scanner (see: RevalidationPolicy.Preload.Image).
If we assume no image re-use: UMA data shows between 55% and 70% on android and between 60% and 85% on windows, of image loads are re-loads that could have re-used a previous decode (the range is due to  issue #2 ). My intuition is that image re-use is high so the potential savings are lower.

Here's a slightly different experiment we could run to decode the feasibility of this approach:
1) Add a UMA for total time spent on image decode tasks
2) Add a UMA metric for cache reuse entirely in the decode cache.
3) Do a finch experiment on Canary with ClearImageCacheOnNavigation entirely removed.
Related bug:  crbug.com/766653 

Sign in to add a comment