Summary: | [WK2] Generalize NetworkCacheStorage API so it can store different types of metadata | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||||||
Component: | WebKit2 | Assignee: | 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
Chris Dumez
2015-10-15 20:45:34 PDT
Created attachment 263243 [details]
WIP Patch
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.
Created attachment 263244 [details]
Patch
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 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 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. Created attachment 263248 [details]
Patch
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. 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 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 on attachment 263248 [details]
Patch
Will re-upload another integration based on initial feedback.
Created attachment 263327 [details]
Patch
Ok Antti, https://www.youtube.com/watch?v=x5kisPBwZOM :) 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 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 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. Created attachment 263496 [details]
Patch
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. 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 on attachment 263496 [details] Patch Clearing flags on attachment: 263496 Committed r191306: <http://trac.webkit.org/changeset/191306> All reviewed patches have been landed. Closing bug. Re-opened since this is blocked by bug 150371 Created attachment 263621 [details]
Patch
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. 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.
Waiting for bots to recover before I land again. Comment on attachment 263621 [details] Patch Clearing flags on attachment: 263621 Committed r191377: <http://trac.webkit.org/changeset/191377> All reviewed patches have been landed. Closing bug. |