Bug 155187 - Speculative disk cache resource revalidations are sometimes wasted
Summary: Speculative disk cache resource revalidations are sometimes wasted
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-03-08 13:25 PST by Chris Dumez
Modified: 2016-03-09 14:02 PST (History)
5 users (show)

See Also:

Attachments
Patch (6.94 KB, patch)
2016-03-08 13:38 PST, Chris Dumez 		no flags Details | Formatted Diff | Diff
Patch (7.28 KB, patch)
2016-03-08 14:04 PST, Chris Dumez 		no flags Details | Formatted Diff | Diff
Patch (11.58 KB, patch)
2016-03-08 18:54 PST, Chris Dumez 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2016-03-08 13:25:03 PST 
Speculative disk cache resource revalidations are sometimes wasted. We sometimes successfully revalidate a resource but the NetworkResourceLoader then either:
1. Fails to reuse the speculatively validated entry
2. Reuses the speculatively validated entry but validates it again
Comment 1 Chris Dumez 2016-03-08 13:25:35 PST 
rdar://problem/25032905
Comment 2 Chris Dumez 2016-03-08 13:38:19 PST 
Created attachment 273323 [details]
Patch
Comment 3 Chris Dumez 2016-03-08 14:04:59 PST 
Created attachment 273339 [details]
Patch
Comment 4 Chris Dumez 2016-03-08 14:05:44 PST 
We don't currently have an easy way to write a test for speculative revalidation but I will work on adding test support for this in a follow-up patch.
Comment 5 Chris Dumez 2016-03-08 18:54:50 PST 
Created attachment 273371 [details]
Patch
Comment 6 Antti Koivisto 2016-03-09 13:16:53 PST 
Comment on attachment 273371 [details]
Patch

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

> Source/WebKit2/NetworkProcess/cache/NetworkCacheEntry.cpp:193
> +    m_response.setSource(value ? WebCore::ResourceResponse::Source::DiskCacheAfterValidation : WebCore::ResourceResponse::Source::DiskCache);

Not from this patch but it is bit strange that we use these source values as needsValidation bit. This would be clearer if the authoritative data was an actual bool member, response source could be updated as a side effect.

> Source/WebKit2/NetworkProcess/cache/NetworkCacheSpeculativeLoadManager.cpp:90
> +#if ENABLE(CACHE_PARTITIONING)

Should really kill these ifdefs at some point. Platforms that don't want partition should just use a fixed partition name.
Comment 7 Chris Dumez 2016-03-09 13:22:24 PST 
Comment on attachment 273371 [details]
Patch

Clearing flags on attachment: 273371

Committed r197879: <http://trac.webkit.org/changeset/197879>
Comment 8 Chris Dumez 2016-03-09 13:22:29 PST 
All reviewed patches have been landed.  Closing bug.
Comment 9 Chris Dumez 2016-03-09 13:25:23 PST 
Comment on attachment 273371 [details]
Patch

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

>> Source/WebKit2/NetworkProcess/cache/NetworkCacheEntry.cpp:193
>> +    m_response.setSource(value ? WebCore::ResourceResponse::Source::DiskCacheAfterValidation : WebCore::ResourceResponse::Source::DiskCache);
> 
> Not from this patch but it is bit strange that we use these source values as needsValidation bit. This would be clearer if the authoritative data was an actual bool member, response source could be updated as a side effect.

I agree, I'll take a look at this in a follow-up.