Bug 216048

Summary:

Safari takes too long to fetch images from memory cache

Product:

WebKit

Reporter:

Aditya <androiditya>

Component:

Images

Assignee:

youenn fablet <youennf>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

aakash_jain, achristensen, cdumez, clopez, darin, ews-watchlist, japhet, koivisto, malsup, nvasilyev, webkit-bot-watchers-bugzilla, webkit-bug-importer, youennf

Priority:

Keywords:

InRadar

Version:

Safari 13

Hardware:

Unspecified

OS:

Unspecified

See Also:

https://github.com/whatwg/fetch/issues/1088
https://github.com/web-platform-tests/wpt/pull/25380

Bug Depends on:

Bug Blocks:

195628

Attachments:

Description	Flags
Low speed of cache fetching	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

Aditya

Reported 2020-09-01 12:41:21 PDT

Created attachment 407702 [details] Low speed of cache fetching Even if the images are in browser memory cache, Safari weirdly takes too long fetch the images. Whereas in Chrome, once the images are stored in the browser cache, it doesn't even make an extra network call to fetch them. Problem: I am showing different images on hover since it is a hover, the images should switch quickly, this works perfectly in Chrome, but Safari fetches the cached images on every hover which takes about half a second. Is this common with safari or am I missing something? Thanks in advance

Attachments
Low speed of cache fetching (213.13 KB, image/png) 2020-09-01 12:41 PDT, Aditya	no flags	Details
Patch (5.61 KB, patch) 2020-09-03 06:15 PDT, youenn fablet	no flags	Details Formatted Diff Diff
Patch (14.22 KB, patch) 2020-09-04 04:41 PDT, youenn fablet	no flags	Details Formatted Diff Diff
Patch (14.34 KB, patch) 2020-09-04 06:07 PDT, youenn fablet	no flags	Details Formatted Diff Diff
Patch (17.09 KB, patch) 2020-09-07 05:08 PDT, youenn fablet	no flags	Details Formatted Diff Diff
Patch (17.50 KB, patch) 2020-09-07 07:42 PDT, youenn fablet	no flags	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Alexey Proskuryakov

Comment 1 2020-09-02 10:03:26 PDT

Thank you for the report! Could you please provide a reproducible test case?

Chris Dumez

Comment 2 2020-09-02 10:18:37 PDT

Note that as I remember it, the values reported by Web Inspector are wrong when the resource comes from the cache. Web Inspector reports actual networking values (when the resource was originally fetched from the network) even the resource is returned by the memory cache. Network metrics are stored in the cache and that's what Web Inspector use. However, I assume you also see visibly slower loading? Or are you submitting this bug report based on bad Web Inspector data?

Aditya

Comment 3 2020-09-02 21:58:24 PDT

I see slow loading aswel, I am not sure if this is because of React or the CSS or do I have to load the images on a CDN, but basically if you go on this link: http://ironvillage-esports.surge.sh Click on "All games" and then try to hover on the games circular cards, you see that the images are slow to load up. If I try to the same link on chrome or firefox, things are working fine. How I am doing this CSS effect is that is: I have 3 states of the game cards: "active", "inactive", "hovered" and "disabled", Unfortunately all the states have different images, to avoid lag, I am loading all images together (Don't quote me on the approach :D) and then stacking them together using position absolute. Finally according the "state". I change the z-index of the images. I tried to replicate a similar thing using codepen(https://codepen.io/androiditya/full/GRZmrqR) but this seems to work fine🤷🏽‍♂️ I hope this helps

Aditya

Comment 4 2020-09-02 23:17:53 PDT

Is it because of how I am adding hashing the images in webpack, because I basically tried the same code on a codesandbox and deployed it, which seems to work just fine: Code: https://codesandbox.io/s/reverent-brown-cbsf0?file=/src/App.js Deployed: https://cbsf0.csb.app/

youenn fablet

Comment 5 2020-09-02 23:54:29 PDT

Most probably, the memory cache is not hit and either the HTTP cache is used or revalidation happens. If you have access to your website logs, you might be able to see whether it receives requests for these images.

youenn fablet

Comment 6 2020-09-02 23:57:51 PDT

Looking at the images, Cache-Control is 'Cache-Control: public, max-age=31536000, no-cache' Given there is 'no-cache', Safari is probably revalidating the cached resource.

youenn fablet

Comment 7 2020-09-03 00:02:30 PDT

Testing on Chrome and Firefox, it does not seem there is any revalidation being done.

youenn fablet

Comment 8 2020-09-03 00:23:04 PDT

Reduced test: <html> <body> <img onclick='newImage()' src='http://ironvillage-esports.surge.sh/ca5a87e6413ecf84fb4e9f21aa5d4487.png'></img> <div id='images'></div> <script> function newImage() { images.innerHTML += "<img src='http://ironvillage-esports.surge.sh/ca5a87e6413ecf84fb4e9f21aa5d4487.png'></img>"; } </script> </body> </html> Clicking on the top level image triggers a revalidation load in Safari, not Chrome and Firefox. Also, the past images being loaded disappear until the revalidation load is complete. It seems there are two issues: - Safari may be revalidating resources in cases where it is not needed. - Invalidating a CachedImage in the memory cache seems to trigger some blinking.

Radar WebKit Bug Importer

Comment 9 2020-09-03 00:23:21 PDT

<rdar://problem/68260952>

youenn fablet

Comment 10 2020-09-03 00:51:58 PDT

> - Safari may be revalidating resources in cases where it is not needed. HTTP spec seems clear we should revalidate. If I change this to a fetch load, revalidation happens in both Chrome and Firefox. Skipping the revalidation in Chrome and Firefox might be specific to images.

youenn fablet

Comment 11 2020-09-03 00:54:56 PDT

https://source.chromium.org/chromium/chromium/src/+/master:third_party/blink/renderer/platform/loader/fetch/resource_fetcher.cc;l=1628;drc=90815a3ab6230a067f0c31935d60838dfccca7a0 seems to indicate Chrome has a specific behavior for images.

youenn fablet

Comment 12 2020-09-03 06:15:21 PDT

Created attachment 407879 [details] Patch

youenn fablet

Comment 13 2020-09-03 06:15:55 PDT

Submitted web-platform-tests pull request: https://github.com/web-platform-tests/wpt/pull/25380

EWS Watchlist

Comment 14 2020-09-03 06:16:09 PDT

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

Antti Koivisto

Comment 15 2020-09-03 09:22:59 PDT

Maybe related to this https://html.spec.whatwg.org/multipage/images.html#the-list-of-available-images (which we don't currently fully implement)?

youenn fablet

Comment 16 2020-09-03 09:45:57 PDT

(In reply to Antti Koivisto from comment #15) > Maybe related to this > https://html.spec.whatwg.org/multipage/images.html#the-list-of-available- > images (which we don't currently fully implement)? Yes, patch is trying to implement a very simple version of it but it apparently fails some existing test. Yoav also told me that the current implementation (not up to date with the spec) is creating some annoying inconsistencies that would be worth fixing.

youenn fablet

Comment 17 2020-09-04 04:41:19 PDT

Created attachment 407959 [details] Patch

youenn fablet

Comment 18 2020-09-04 06:07:18 PDT

Created attachment 407965 [details] Patch

Antti Koivisto

Comment 19 2020-09-04 06:18:34 PDT

Bug 195628 is another instance of this.

Darin Adler

Comment 20 2020-09-04 13:14:37 PDT

Comment on attachment 407965 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407965&action=review > Source/WebCore/loader/cache/CachedResourceLoader.h:77 > -class CachedResourceLoader : public RefCounted<CachedResourceLoader> { > +class CachedResourceLoader : public RefCounted<CachedResourceLoader>, public CanMakeWeakPtr<CachedResourceLoader> { Can’t the CachedImage hang on to a weak pointer to the Document instead of to the CachedResourceLoader? I wouldn't add CanMakeWeakPtr to another class lightly, since it makes the objects bigger and more expensive to create/destroy.

Antti Koivisto

Comment 21 2020-09-04 22:35:00 PDT

Comment on attachment 407965 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407965&action=review Can you check that https://bugs.webkit.org/show_bug.cgi?id=195628 is also fixed? There is a test there that could be added. > Source/WebCore/loader/cache/CachedImage.cpp:736 > + // Skip revalidation as per https://html.spec.whatwg.org/#ignore-higher-layer-caching. > + if (loader && m_cachedResourceLoader && loader->document() == m_cachedResourceLoader->document()) > + return RevalidationDecision::No; It would be good to mention this doesn't really implement the algorithm as described (we don't maintain a per-Document list of available images). Maybe we should at some point? > Source/WebCore/loader/cache/CachedImage.h:190 > + WeakPtr<CachedResourceLoader> m_cachedResourceLoader; Like Darin said, this could be Document. It might also be helpful to add comment why it is here since CachedResources are generally not associated with any particular Document.

youenn fablet

Comment 22 2020-09-07 02:42:07 PDT

> > Source/WebCore/loader/cache/CachedResourceLoader.h:77 > > -class CachedResourceLoader : public RefCounted<CachedResourceLoader> { > > +class CachedResourceLoader : public RefCounted<CachedResourceLoader>, public CanMakeWeakPtr<CachedResourceLoader> { > > Can’t the CachedImage hang on to a weak pointer to the Document instead of > to the CachedResourceLoader? I wouldn't add CanMakeWeakPtr to another class > lightly, since it makes the objects bigger and more expensive to > create/destroy. OK > Can you check that https://bugs.webkit.org/show_bug.cgi?id=195628 is also > fixed? There is a test there that could be added. OK > > Source/WebCore/loader/cache/CachedImage.cpp:736 > > + // Skip revalidation as per https://html.spec.whatwg.org/#ignore-higher-layer-caching. > > + if (loader && m_cachedResourceLoader && loader->document() == m_cachedResourceLoader->document()) > > + return RevalidationDecision::No; > > It would be good to mention this doesn't really implement the algorithm as > described (we don't maintain a per-Document list of available images). Maybe > we should at some point? I will add a comment. FWIW, spec is loosely implemented and we should specify a memory cache behavior that HTML would piggy back on for images.

youenn fablet

Comment 23 2020-09-07 03:47:29 PDT

> > Can you check that https://bugs.webkit.org/show_bug.cgi?id=195628 is also > > fixed? There is a test there that could be added. Current patch does not handle redirections, I'll update it and it should fix bug 195628.

youenn fablet

Comment 24 2020-09-07 05:08:24 PDT

Created attachment 408173 [details] Patch

youenn fablet

Comment 25 2020-09-07 05:09:57 PDT

*** Bug 195628 has been marked as a duplicate of this bug. ***

youenn fablet

Comment 26 2020-09-07 07:42:13 PDT

Created attachment 408179 [details] Patch

EWS

Comment 27 2020-09-07 08:51:29 PDT

Committed r266699: <https://trac.webkit.org/changeset/266699> All reviewed patches have been landed. Closing bug and clearing flags on attachment 408179 [details].

Ryan Haddad

Comment 28 2020-09-07 20:53:30 PDT

As seen on EWS before landing, http/wpt/html/dom/elements/images/hover-image-change.html is consistently timing out on iOS bots: https://ews-build.webkit.org/#/builders/24/builds/25132

Ryan Haddad

Comment 29 2020-09-07 20:55:25 PDT

(In reply to Ryan Haddad from comment #28) > As seen on EWS before landing, > http/wpt/html/dom/elements/images/hover-image-change.html is consistently > timing out on iOS bots: > https://ews-build.webkit.org/#/builders/24/builds/25132 Correction: the result wasn't posted by EWS before this patch was landed.

youenn fablet

Comment 30 2020-09-08 01:10:12 PDT

mouse events are not a thing in iOS. I will fix this

youenn fablet

Comment 31 2020-09-08 01:12:46 PDT

(In reply to youenn fablet from comment #30) > mouse events are not a thing in iOS. > I will fix this Filed https://bugs.webkit.org/show_bug.cgi?id=216265

Note You need to log in before you can comment on or make changes to this bug.