Bug 150221 – [WK2] Generalize NetworkCacheStorage API so it can store different types of metadata

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

[Chris Dumez]

Generalize NetworkCacheStorage API so it can store different types of metadata alongside the network resources. This is a pre-requirement to making our NetworkCache smarter by storing information about the resources.

[Chris Dumez]

rdar://problem/23092196

[Chris Dumez]

Created attachment 263243 [details]
WIP Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.cpp:188:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.cpp:943:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h:70:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h:177:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 4 in 4 files


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

[Chris Dumez]

Created attachment 263244 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.cpp:188:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.cpp:948:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h:70:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h:177:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 4 in 5 files


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

[Alexey Proskuryakov]

Comment on attachment 263244 [details]
Patch

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

> Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:55
> +    static NeverDestroyed<const String> resource(ASCIILiteral("resource"));

I like to assert that we are on a known (usually main) thread in functions like this, to make sure that the code isn't accidentally used from a secondary thread.

> Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:361
> +    m_storage->retrieve(storageKey, resourceType(), priority, [this, originalRequest, completionHandler, startTime, storageKey, webPageID](std::unique_ptr<Storage::Record> record) {

And here it looks like maybe it is?

[Chris Dumez]

Comment on attachment 263244 [details]
Patch

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

>> Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:55
>> +    static NeverDestroyed<const String> resource(ASCIILiteral("resource"));
> 
> I like to assert that we are on a known (usually main) thread in functions like this, to make sure that the code isn't accidentally used from a secondary thread.

This is a good idea, I'll add a check.

>> Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:361
>> +    m_storage->retrieve(storageKey, resourceType(), priority, [this, originalRequest, completionHandler, startTime, storageKey, webPageID](std::unique_ptr<Storage::Record> record) {
> 
> And here it looks like maybe it is?

No, this is executed in the main thread.

[Chris Dumez]

Created attachment 263248 [details]
Patch

[Chris Dumez]

Comment on attachment 263244 [details]
Patch

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

>>> Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:55
>>> +    static NeverDestroyed<const String> resource(ASCIILiteral("resource"));
>> 
>> I like to assert that we are on a known (usually main) thread in functions like this, to make sure that the code isn't accidentally used from a secondary thread.
> 
> This is a good idea, I'll add a check.

Done.

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.cpp:188:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.cpp:948:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h:70:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h:177:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 4 in 5 files


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

[Antti Koivisto]

Comment on attachment 263248 [details]
Patch

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

> Source/WebKit2/ChangeLog:17
> +        Generalize NetworkCacheStorage API so it can store different types of
> +        metadata alongside the network resources. This is a pre-requirement to
> +        making our NetworkCache smarter by storing information about the
> +        resources.
> +
> +        To simplify the code, all cache entries now have a suffix, so that they
> +        all following the following format:
> +        - Regular entries: [hash]-[type]
> +        - Blobs: [hash]-[type]blob

Not that I look this, wouldn't we get pretty much the same features by simply expanding Key to include the entry type?

I was thinking of having single record file and multiple associated data blobs. That scheme would have advantage of having less files in general case. However may it is not worth the complexity.

> Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.cpp:60
> +static bool typeUsesBlobStorage(const String& type)
> +{
> +    return type == resourceType;
> +}
> +
> +static bool typeUsesHeader(const String& type)
> +{
> +    return type == resourceType;
> +}

Are these necessary? Wouldn't the existing solution that just automatically inlines the data to record file if it is small (<16KB) work as is?

> Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.cpp:67
> +    ReadOperation(const Key& key, const String& type, const RetrieveCompletionHandler& completionHandler)

Type could be AtomicString (here and elsewhere).

[Chris Dumez]

Comment on attachment 263248 [details]
Patch

Will re-upload another integration based on initial feedback.

[Radar WebKit Bug Importer]

<rdar://problem/23149771>

[Chris Dumez]

Created attachment 263327 [details]
Patch

[Chris Dumez]

Ok Antti, https://www.youtube.com/watch?v=x5kisPBwZOM :)

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.cpp:156:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.cpp:876:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h:68:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h:173:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 4 in 7 files


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

[Darin Adler]

Comment on attachment 263327 [details]
Patch

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

> Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:54
> +static const String resourceType()

More efficient to return const String& to avoid reference count churn every time this is called.

[Antti Koivisto]

Comment on attachment 263327 [details]
Patch

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

r=me as well, looks good.

> Source/WebKit2/ChangeLog:18
> +        - WebKitCache/Version 5/[Partition]/[Type]/[Hash]-bodyblob (Optional)

"bodyblob" is bit strange. How about "blob" or "data"?

>> Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:54
>> +static const String resourceType()
> 
> More efficient to return const String& to avoid reference count churn every time this is called.

It would also make sense to use AtomicString for type identifier. Doesn't need to be in this patch if there are complications.

> Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.cpp:902
>          deleteEmptyRecordsDirectories(recordsPath);

I think this patch will leave empty partition directories behind. This can be solved separately though.

[Chris Dumez]

Created attachment 263496 [details]
Patch

[Chris Dumez]

Comment on attachment 263327 [details]
Patch

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

>> Source/WebKit2/ChangeLog:18
>> +        - WebKitCache/Version 5/[Partition]/[Type]/[Hash]-bodyblob (Optional)
> 
> "bodyblob" is bit strange. How about "blob" or "data"?

Renamed to "blob"

>>> Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:54
>>> +static const String resourceType()
>> 
>> More efficient to return const String& to avoid reference count churn every time this is called.
> 
> It would also make sense to use AtomicString for type identifier. Doesn't need to be in this patch if there are complications.

Ok, now returns a "const AtomicString&"

>> Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.cpp:902
>>          deleteEmptyRecordsDirectories(recordsPath);
> 
> I think this patch will leave empty partition directories behind. This can be solved separately though.

Right, the last iteration updates deleteEmptyRecordsDirectories() to delete empty [type] subdirectories before attempting to delete the [partitition] ones.

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.cpp:156:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.cpp:888:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h:68:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h:173:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 4 in 7 files


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

[WebKit Commit Bot]

Comment on attachment 263496 [details]
Patch

Clearing flags on attachment: 263496

Committed r191306: <http://trac.webkit.org/changeset/191306>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 150371

[Chris Dumez]

Created attachment 263621 [details]
Patch

[Chris Dumez]

Comment on attachment 263496 [details]
Patch

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

> Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.cpp:165
> +            if (!actualType.isEmpty() && expectedType != actualType)

This was supposed to be:
if (! expectedType.isEmpty() && expectedType != actualType)

> Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.cpp:168
> +            traverseDirectory(partitionPath, [&function, &recordDirectoryPath, &actualType](const String& fileName, DirectoryEntryType entryType) {

Should pass recordDirectoryPath instead of partitionPath here.

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.cpp:156:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.cpp:888:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h:68:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h:173:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 4 in 7 files


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

[Chris Dumez]

Waiting for bots to recover before I land again.

[WebKit Commit Bot]

Comment on attachment 263621 [details]
Patch

Clearing flags on attachment: 263621

Committed r191377: <http://trac.webkit.org/changeset/191377>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.