This patch is a first step towards speculative revalidation support in the WebKit network cache. It maps sub resources to the main resource that caused them to be requested. We then write this information to the network cache. <rdar://problem/23092196>
Created attachment 264892 [details] Patch
Attachment 264892 [details] did not pass style-queue: ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheSpeculativeLoader.cpp:61: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:346: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCache.h:103: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 3 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 264893 [details] Patch
Attachment 264893 [details] did not pass style-queue: ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheSpeculativeLoader.cpp:61: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:346: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCache.h:103: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 3 in 18 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 264895 [details] Patch
Attachment 264895 [details] did not pass style-queue: ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheSpeculativeLoader.cpp:61: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:346: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCache.h:103: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 3 in 18 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 264895 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=264895&action=review > Source/WebKit2/NetworkProcess/cache/NetworkCacheKey.h:93 > + return *reinterpret_cast<const unsigned*>(key.hash().data()); It might be nice to have a static_assert that key.hash().hashSize >= sizeof(unsigned). Also, do you need to call computeHash? > Source/WebKit2/NetworkProcess/cache/NetworkCacheSpeculativeLoader.cpp:91 > + m_completionHandler(); Is the completion handler always called on the main thread? I think it shouldn't be if we are going to call it and write to the disk before requesting a main resource. > Source/WebKit2/NetworkProcess/cache/NetworkCacheSpeculativeLoader.cpp:122 > + if (isMainResource) { > + // Mark previous load in this frame as completed if necessary. > + if (auto* pendingFrameLoad = m_pendingFrameLoads.get(frameKey)) > + pendingFrameLoad->markAsCompleted(); This could increase the time between requesting and receiving a main resource, right?
Comment on attachment 264895 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=264895&action=review >> Source/WebKit2/NetworkProcess/cache/NetworkCacheKey.h:93 >> + return *reinterpret_cast<const unsigned*>(key.hash().data()); > > It might be nice to have a static_assert that key.hash().hashSize >= sizeof(unsigned). > Also, do you need to call computeHash? The static_assert() makes sense, I'll add it. Regarding the other comment, I don't need to call computeHash() because key.hash() already returns the result of calling computeHash() (see m_hash init in Key constructor). >> Source/WebKit2/NetworkProcess/cache/NetworkCacheSpeculativeLoader.cpp:91 >> + m_completionHandler(); > > Is the completion handler always called on the main thread? I think it shouldn't be if we are going to call it and write to the disk before requesting a main resource. Yes, this is always called on the main thread. That said, I am not sure I see the problem. This will end up: 1. Removing this PendingFrameLoad object from the HashMap. 2. Calling Storage::store() which will do the i/o write *in a background priority thread*. 3. Destroy the PendingFrameLoad object. >> Source/WebKit2/NetworkProcess/cache/NetworkCacheSpeculativeLoader.cpp:122 >> + pendingFrameLoad->markAsCompleted(); > > This could increase the time between requesting and receiving a main resource, right? I don't see why. The only expensive thing calling markAsCompleted() will do is the i/o write but this will happen in a background priority thread. Resource retrievals happen in a high priority thread.
Comment on attachment 264895 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=264895&action=review > Source/WebKit2/NetworkProcess/cache/NetworkCache.h:92 > + bool enableEfficacyLogging; These should have { false } after them. > Source/WebKit2/NetworkProcess/cache/NetworkCache.h:94 > + bool enableNetworkCacheSpeculativeRevalidation; These should have { false } after them. > Source/WebKit2/NetworkProcess/cache/NetworkCacheSpeculativeLoader.cpp:50 > +static inline Key makeSubResourcesKey(const Key& resourceKey) The word is “subresource” so it should not have a capital “R”. > Source/WebKit2/NetworkProcess/cache/NetworkCacheSpeculativeLoader.cpp:64 > + void registerSubResource(const Key& subResourceKey) The word is “subresource” so it should not have a capital “R”. > Source/WebKit2/NetworkProcess/cache/NetworkCacheSpeculativeLoader.cpp:70 > + Optional<Storage::Record> encodeAsSubResourcesRecord() The word is “subresource” so it should not have a capital “R”. > Source/WebKit2/NetworkProcess/cache/NetworkCacheSpeculativeLoader.cpp:76 > + Vector<Key> subResourceKeys; The word is “subresource” so it should not have a capital “R”. > Source/WebKit2/NetworkProcess/cache/NetworkCacheSpeculativeLoader.cpp:86 > + SubResourcesEntry subresourceEntry(subresourcesStorageKey, subResourceKeys); > + return subresourceEntry.encodeAsStorageRecord(); Maybe write this without a local variable? > Source/WebKit2/NetworkProcess/cache/NetworkCacheSpeculativeLoader.cpp:96 > + HashSet<Key> m_subResourceKeys; The word is “subresource” so it should not have a capital “R”. > Source/WebKit2/NetworkProcess/cache/NetworkCacheSubResourcesEntry.cpp:51 > + std::unique_ptr<SubResourcesEntry> entry(new SubResourcesEntry(storageEntry)); Should use make_unique here, not new. Also use auto to avoid repeating the type twice. > Source/WebKit2/NetworkProcess/cache/NetworkCacheSubResourcesEntry.h:36 > +class SubResourcesEntry { The word is “subresources” and so it should not have a capital “R”. > Source/WebKit2/NetworkProcess/cache/NetworkCacheSubResourcesEntry.h:39 > + SubResourcesEntry(const Key&, const Vector<Key>& subresourceKeys); Should take a Vector<Key>&& and move it in. > Source/WebKit2/NetworkProcess/cocoa/NetworkProcessCocoa.mm:107 > + RetainPtr<NSURLCache> urlCache(adoptNS([[NSURLCache alloc] initWithMemoryCapacity:0 diskCapacity:0 diskPath:nil])); > + [NSURLCache setSharedURLCache:urlCache.get()]; I think this would be nicer without the local variable. Or could use auto; no need to say NSURLCache twice or say RetainPtr explicitly in either case.
(In reply to comment #8) > 2. Calling Storage::store() which will do the i/o write *in a background > priority thread*. Ok, I see this in NetworkCacheStorage.cpp now.
Comment on attachment 264895 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=264895&action=review >> Source/WebKit2/NetworkProcess/cache/NetworkCache.h:92 >> + bool enableEfficacyLogging; > > These should have { false } after them. If I do this, then I will no longer be able to initialize this struct like so: { false, false } which is how I initialize it in NetworkProcessCocoa.mm. To maintain this initialization pattern while doing what you suggest, I would also need to add an explicit construct that takes 2 parameters in. I don't think this is worth the extra code.
Comment on attachment 264895 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=264895&action=review >> Source/WebKit2/NetworkProcess/cache/NetworkCacheSubResourcesEntry.cpp:51 >> + std::unique_ptr<SubResourcesEntry> entry(new SubResourcesEntry(storageEntry)); > > Should use make_unique here, not new. Also use auto to avoid repeating the type twice. Sadly, this means I need to make that constructor public I think, but OK.
Created attachment 264942 [details] Patch
Attachment 264942 [details] did not pass style-queue: ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheSpeculativeLoader.cpp:61: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:346: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCache.h:103: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 3 in 20 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 264942 [details] Patch Clearing flags on attachment: 264942 Committed r192111: <http://trac.webkit.org/changeset/192111>
All reviewed patches have been landed. Closing bug.
(In reply to comment #11) > If I do this, then I will no longer be able to initialize this struct like > so: { false, false } which is how I initialize it in NetworkProcessCocoa.mm. Sure, but if you do it the structure will be initialized that way without the code in NetworkProcessCocoa.mm having to specify any initialization arguments at all. I think it would be good.
Apple Mac CMake buildfix landed in https://trac.webkit.org/changeset/192150