Bug 155187

Summary: Speculative disk cache resource revalidations are sometimes wasted
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebKit2Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: aestes, cgarcia, commit-queue, koivisto, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=155254
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Chris Dumez
Reported 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
Attachments
Patch (6.94 KB, patch)
2016-03-08 13:38 PST, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Patch (7.28 KB, patch)
2016-03-08 14:04 PST, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Patch (11.58 KB, patch)
2016-03-08 18:54 PST, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Chris Dumez
Comment 1 2016-03-08 13:25:35 PST
rdar://problem/25032905
Chris Dumez
Comment 2 2016-03-08 13:38:19 PST
Created attachment 273323 [details] Patch
Chris Dumez
Comment 3 2016-03-08 14:04:59 PST
Created attachment 273339 [details] Patch
Chris Dumez
Comment 4 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.
Chris Dumez
Comment 5 2016-03-08 18:54:50 PST
Created attachment 273371 [details] Patch
Antti Koivisto
Comment 6 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.
Chris Dumez
Comment 7 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>
Chris Dumez
Comment 8 2016-03-09 13:22:29 PST
All reviewed patches have been landed. Closing bug.
Chris Dumez
Comment 9 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.
Note You need to log in before you can comment on or make changes to this bug.