Bug 150951 – [WK2][SpeculativeRevalidation] Save to disk cache relationship between resources

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Chris Dumez]

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>

[Chris Dumez]

Created attachment 264892 [details]
Patch

[WebKit Commit Bot]

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.

[Chris Dumez]

Created attachment 264893 [details]
Patch

[WebKit Commit Bot]

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.

[Chris Dumez]

Created attachment 264895 [details]
Patch

[WebKit Commit Bot]

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.

[Alex Christensen]

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?

[Chris Dumez]

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.

[Darin Adler]

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.

[Alex Christensen]

(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.

[Chris Dumez]

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.

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 264942 [details]
Patch

[WebKit Commit Bot]

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.

[WebKit Commit Bot]

Comment on attachment 264942 [details]
Patch

Clearing flags on attachment: 264942

Committed r192111: <http://trac.webkit.org/changeset/192111>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

(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.

[Csaba Osztrogonác]

Apple Mac CMake buildfix landed in https://trac.webkit.org/changeset/192150