Bug 150221

Summary: [WK2] Generalize NetworkCacheStorage API so it can store different types of metadata
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebKit2Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: cgarcia, commit-queue, koivisto, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 150371    
Bug Blocks:    
Attachments:
Description Flags
WIP Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Chris Dumez 2015-10-15 20:45:34 PDT
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.
Comment 1 Chris Dumez 2015-10-15 20:47:14 PDT
rdar://problem/23092196
Comment 2 Chris Dumez 2015-10-15 20:47:56 PDT
Created attachment 263243 [details]
WIP Patch
Comment 3 WebKit Commit Bot 2015-10-15 20:49:53 PDT
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.
Comment 4 Chris Dumez 2015-10-15 21:31:14 PDT
Created attachment 263244 [details]
Patch
Comment 5 WebKit Commit Bot 2015-10-15 21:33:42 PDT
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.
Comment 6 Alexey Proskuryakov 2015-10-15 22:09:24 PDT
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?
Comment 7 Chris Dumez 2015-10-15 22:23:11 PDT
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.
Comment 8 Chris Dumez 2015-10-15 22:26:43 PDT
Created attachment 263248 [details]
Patch
Comment 9 Chris Dumez 2015-10-15 22:27:03 PDT
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.
Comment 10 WebKit Commit Bot 2015-10-15 22:28:16 PDT
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.
Comment 11 Antti Koivisto 2015-10-16 06:01:01 PDT
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).
Comment 12 Chris Dumez 2015-10-16 12:34:13 PDT
Comment on attachment 263248 [details]
Patch

Will re-upload another integration based on initial feedback.
Comment 13 Radar WebKit Bug Importer 2015-10-16 13:56:28 PDT
<rdar://problem/23149771>
Comment 14 Chris Dumez 2015-10-16 14:07:15 PDT
Created attachment 263327 [details]
Patch
Comment 15 Chris Dumez 2015-10-16 14:08:53 PDT
Ok Antti, https://www.youtube.com/watch?v=x5kisPBwZOM :)
Comment 16 WebKit Commit Bot 2015-10-16 14:10:45 PDT
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.
Comment 17 Darin Adler 2015-10-18 16:14:54 PDT
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.
Comment 18 Antti Koivisto 2015-10-19 05:49:10 PDT
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.
Comment 19 Chris Dumez 2015-10-19 11:26:48 PDT
Created attachment 263496 [details]
Patch
Comment 20 Chris Dumez 2015-10-19 11:28:33 PDT
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.
Comment 21 WebKit Commit Bot 2015-10-19 11:28:48 PDT
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.
Comment 22 WebKit Commit Bot 2015-10-19 12:22:02 PDT
Comment on attachment 263496 [details]
Patch

Clearing flags on attachment: 263496

Committed r191306: <http://trac.webkit.org/changeset/191306>
Comment 23 WebKit Commit Bot 2015-10-19 12:22:07 PDT
All reviewed patches have been landed.  Closing bug.
Comment 24 WebKit Commit Bot 2015-10-20 13:46:54 PDT
Re-opened since this is blocked by bug 150371
Comment 25 Chris Dumez 2015-10-20 15:22:18 PDT
Created attachment 263621 [details]
Patch
Comment 26 Chris Dumez 2015-10-20 15:23:33 PDT
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.
Comment 27 WebKit Commit Bot 2015-10-20 15:24:40 PDT
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.
Comment 28 Chris Dumez 2015-10-20 15:41:09 PDT
Waiting for bots to recover before I land again.
Comment 29 WebKit Commit Bot 2015-10-20 23:41:55 PDT
Comment on attachment 263621 [details]
Patch

Clearing flags on attachment: 263621

Committed r191377: <http://trac.webkit.org/changeset/191377>
Comment 30 WebKit Commit Bot 2015-10-20 23:42:03 PDT
All reviewed patches have been landed.  Closing bug.