Bug 143226 – Don't cache resources that are very unlikely to be reused

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Antti Koivisto]

Cache entries with 0 cache lifetime and no validation headers are generally not useful.

[Radar WebKit Bug Importer]

<rdar://problem/20347160>

[Chris Dumez]

What about history navigation and tab restoring after jetsam / crash?

[Antti Koivisto]

I keep the current policy for main resources.

[Chris Dumez]

The current pruning algorithm already takes care of dropping resources that were not reused first. I am slightly worried about being more aggressive here and completely not cache such resources. It will potentially cause us to use the radio more when using history navigations and on tab restore.

[Antti Koivisto]

That balances against concerns of wasting cache space (so having less space for useful item) and power and performance effects of unnecessary disk writes.

[Chris Dumez]

The pruning policy should hopefully address the issue for wasting cache space. This change could negatively affect <rdar://problem/19869257>. In airplane mode, it is not only an efficiency issue but the user would potentially loose useful content as we wouldn't be able to restore it.

[Antti Koivisto]

Created attachment 249744 [details]
patch

[WebKit Commit Bot]

Attachment 249744 [details] did not pass style-queue:


ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:296:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antti Koivisto]

The patch is still very conservative. Resources marked this uncacheable are generally one time xhr, ad ping images/scripts and similar. They are probably very rarely needed for a reasonably good tab restore. If we find problematic cases we can change the policy.

[Geoffrey Garen]

Comment on attachment 249744 [details]
patch

r=me

[Build Bot]

Comment on attachment 249744 [details]
patch

Attachment 249744 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6148567194402816

New failing tests:
http/tests/cache/disk-cache/disk-cache-request-max-stale.html

[Build Bot]

Created attachment 249752 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Chris Dumez]

Comment on attachment 249744 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=249744&action=review

> Source/WebKit2/NetworkProcess/cache/NetworkCache.h:-69
> -    NoDueToAttachmentResponse,

We need to keep this one. We cannot shift values in this enum as they are stored in integers in the database.

[Chris Dumez]

Comment on attachment 249744 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=249744&action=review

>> Source/WebKit2/NetworkProcess/cache/NetworkCache.h:-69
>> -    NoDueToAttachmentResponse,
> 
> We need to keep this one. We cannot shift values in this enum as they are stored in integers in the database.

We should probably add a comment for this enum to explain we cannot do this :) Sorry about that.

[Chris Dumez]

Comment on attachment 249744 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=249744&action=review

> Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:274
> +    bool hasExipirationHeaders = std::isfinite(response.expires()) || std::isfinite(response.cacheControlMaxAge());

hasExipirationHeaders ? :D typo.

[Chris Dumez]

Comment on attachment 249744 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=249744&action=review

> Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:283
> +    bool storeUnconditionallyForBackNavigation = originalRequest.priority() == WebCore::ResourceLoadPriorityVeryHigh;

I would say "ForHistoryNavigation".

[Antti Koivisto]

Created attachment 249759 [details]
patch

[Chris Dumez]

Comment on attachment 249759 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=249759&action=review

ok

> Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:281
> +        auto currentTime = std::chrono::duration<double>(std::chrono::system_clock::now().time_since_epoch());

auto now = currentTime(); ?

c.f. NetworkResourceLoader::didFinishLoading().

> LayoutTests/http/tests/cache/disk-cache/disk-cache-request-max-stale.html:30
> +

nit: extra line?

[Antti Koivisto]

> > +        auto currentTime = std::chrono::duration<double>(std::chrono::system_clock::now().time_since_epoch());
> 
> auto now = currentTime(); ?

We should move to std::chrono everywhere.

[Antti Koivisto]

https://trac.webkit.org/r182152

[Antti Koivisto]

Created attachment 249766 [details]
follow-up to cover no-cache

[Chris Dumez]

Comment on attachment 249766 [details]
follow-up to cover no-cache

r=me

[Antti Koivisto]

Follow-up for no-cache case: https://trac.webkit.org/r182154