Doing persistent caching in the engine itself instead of hiding it under the networking layer would have some advantages: - The engine has more information about the usage of resources and can limit disk writing to those that are more likely to be needed again. - Lazy writing from memory cache to disk, no need for thenetworking layer to keep duplicate copies of the data. - Possibility to cache processed resources (js bytecode, token steams etc.) in the future - More consistent behavior when caching and revalidation semantics are implemented in one place only. - Potentially simpler, more focused networking layer. - Closer cache integration with the engine open up new optimization possibilities.
Created attachment 41083 [details] experimental patch (frontend) This patch contains the engine side changes. It uses the existing WebCore memory cache validations mechanism to revalidate expired resources retrieved from the persistent cache. The idea is that the backend has an in-memory hash table of all cache content (containing hashes only). This way existence of a resource can always be determined synchronously without ever waiting for disk access. There are two new classes, loader/PersistentCache.h and platform/network/PersistentCacheStorage.h. The latter is an abstract base class for backend implementation.
Created attachment 41085 [details] experimental patch (libdispatch based backend) Experimental libdispatch based backend for Snow Leopard (or anything with a libdispatch port). A memory mapped hash table tracks all cache content. To keep the size reasonable it only stores hashes for each item. Collisions are ignored which means the cache can not contain more than one item with the same hash (a pretty rare case). The data is stored in per-domain storage files. Each storage file has an associated memory mapped allocation table that tracks the free space. Data integrity is verified with MD5 hash over both resource data and headers. It is non-blocking, highly parallel and seems pretty fast. Some things that are missing: - not entirely multiprocess safe (shouldn't be too hard) - no pruning based on data size - no shrinking of storage files - can not write resource headers only, after revalidation the entire resource needs to be rewritten.
Created attachment 41149 [details] updated backend patch, now with the mmap related classes included
<rdar://problem/7401055>
This sounds like a good idea to me. Qt's network layer pumps data as fast as possible to QtWebKit so the user sees something. Caching can be then done "later" in that process. (Haven't looked at any patches)
Created attachment 238659 [details] WIP patch
Attachment 238659 [details] did not pass style-queue: ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.mm:49: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.mm:49: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.mm:351: Use 'WTF::move()' instead of 'std::move()'. [runtime/wtf_move] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.mm:374: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.mm:389: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.h:31: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.h:35: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.h:63: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.h:84: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.h:85: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.h:95: The parameter name "key" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.h:101: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkCache.h:36: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkCache.h:40: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkCache.h:65: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkCache.h:67: The parameter name "entry" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebKit2/NetworkProcess/NetworkCache.cpp:64: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] ERROR: Source/WebKit2/NetworkProcess/NetworkCache.cpp:78: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkCache.cpp:79: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkCache.cpp:85: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkCache.cpp:169: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkCache.cpp:188: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkCache.cpp:201: Use 'WTF::move()' instead of 'std::move()'. [runtime/wtf_move] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:152: Use 'WTF::move()' instead of 'std::move()'. [runtime/wtf_move] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:155: Use 'WTF::move()' instead of 'std::move()'. [runtime/wtf_move] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:292: Use 'WTF::move()' instead of 'std::move()'. [runtime/wtf_move] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:514: Use 'WTF::move()' instead of 'std::move()'. [runtime/wtf_move] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkProcessCocoa.mm:73: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkProcessCocoa.mm:80: Should have a space between // and comment [whitespace/comments] [4] Total errors found: 29 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 238660 [details] build fix
Attachment 238660 [details] did not pass style-queue: ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.mm:49: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.mm:49: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.mm:351: Use 'WTF::move()' instead of 'std::move()'. [runtime/wtf_move] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.mm:374: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.mm:389: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.h:31: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.h:35: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.h:63: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.h:84: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.h:85: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.h:95: The parameter name "key" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.h:101: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkCache.h:36: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkCache.h:40: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkCache.h:65: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkCache.h:67: The parameter name "entry" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebKit2/NetworkProcess/NetworkCache.cpp:64: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] ERROR: Source/WebKit2/NetworkProcess/NetworkCache.cpp:78: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkCache.cpp:79: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkCache.cpp:85: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkCache.cpp:169: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkCache.cpp:188: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkCache.cpp:201: Use 'WTF::move()' instead of 'std::move()'. [runtime/wtf_move] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:152: Use 'WTF::move()' instead of 'std::move()'. [runtime/wtf_move] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:155: Use 'WTF::move()' instead of 'std::move()'. [runtime/wtf_move] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:292: Use 'WTF::move()' instead of 'std::move()'. [runtime/wtf_move] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:514: Use 'WTF::move()' instead of 'std::move()'. [runtime/wtf_move] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkProcessCocoa.mm:73: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkProcessCocoa.mm:80: Should have a space between // and comment [whitespace/comments] [4] Total errors found: 29 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 238667 [details] another
Attachment 238667 [details] did not pass style-queue: ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.mm:49: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.mm:49: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.mm:374: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.mm:389: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.h:84: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.h:85: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.h:101: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkCache.h:65: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkCache.cpp:78: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkCache.cpp:79: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkCache.cpp:85: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkCache.cpp:169: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkCache.cpp:188: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkProcessCocoa.mm:73: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkProcessCocoa.mm:80: Should have a space between // and comment [whitespace/comments] [4] Total errors found: 15 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.
Note that the storage backend design is likely to evolve significantly.
Created attachment 240309 [details] another
Attachment 240309 [details] did not pass style-queue: ERROR: Source/WebKit2/NetworkProcess/NetworkCache.h:65: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/network/ResourceResponseBase.h:119: The parameter name "source" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.h:88: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.h:89: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.h:105: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkCache.cpp:74: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebKit2/NetworkProcess/NetworkCache.cpp:171: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkCache.cpp:217: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkCache.cpp:236: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/testing/Internals.cpp:105: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.mm:43: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.mm:43: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.mm:59: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.mm:59: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.mm:410: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.mm:424: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 16 in 28 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 240387 [details] Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-10 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Created attachment 240390 [details] yet another
Attachment 240390 [details] did not pass style-queue: ERROR: Source/WebKit2/NetworkProcess/NetworkCache.h:65: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/network/ResourceResponseBase.h:119: The parameter name "source" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.h:88: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.h:89: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.h:105: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkCache.cpp:74: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebKit2/NetworkProcess/NetworkCache.cpp:171: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkCache.cpp:217: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkCache.cpp:236: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/testing/Internals.cpp:105: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.mm:43: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.mm:43: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.mm:59: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.mm:59: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.mm:410: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.mm:424: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 16 in 28 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 240390 [details] yet another View in context: https://bugs.webkit.org/attachment.cgi?id=240390&action=review > Source/WebCore/testing/Internals.cpp:393 > + return "Null xhr"; Shouldn't we use ASCIILiteral() for these? > Source/WebKit2/NetworkProcess/NetworkCache.cpp:213 > + partition = WTF::ASCIILiteral("No Partition"); We probably don't need the WTF:: > Source/WebKit2/NetworkProcess/NetworkCache.cpp:264 > + default: I find it weird default is first :) > Source/WebKit2/NetworkProcess/NetworkCache.cpp:310 > + RefPtr<WebCore::SharedBuffer> buffer = nullptr; We don't need the = nullptr; > Source/WebKit2/NetworkProcess/NetworkCache.h:52 > + NetworkCache(); Does this need to be public? > Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:515 > + (std::unique_ptr<NetworkCache::Entry> entry) We are not passing ownership, why is this a unique_ptr and not a reference? > Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:526 > + if (entry && entry->buffer) Why the entry null check here?
Created attachment 240398 [details] Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-16 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Created attachment 240419 [details] with printfs to figure out why the test fails on bots
Attachment 240419 [details] did not pass style-queue: ERROR: Source/WebKit2/NetworkProcess/NetworkCache.h:65: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/network/ResourceResponseBase.h:119: The parameter name "source" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.h:88: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.h:89: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.h:105: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkCache.cpp:76: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebKit2/NetworkProcess/NetworkCache.cpp:173: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkCache.cpp:219: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkCache.cpp:238: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/testing/Internals.cpp:105: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.mm:43: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.mm:43: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.mm:59: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.mm:59: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.mm:410: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.mm:424: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 16 in 28 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 240428 [details] Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-15 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Created attachment 240549 [details] another
Attachment 240549 [details] did not pass style-queue: ERROR: Source/WebKit2/NetworkProcess/NetworkCache.h:65: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/network/ResourceResponseBase.h:120: The parameter name "source" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.h:88: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.h:89: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.h:110: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkCache.cpp:80: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebKit2/NetworkProcess/NetworkCache.cpp:192: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkCache.cpp:211: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/testing/Internals.cpp:105: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.mm:45: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.mm:45: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.mm:61: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.mm:61: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.mm:424: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.mm:438: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 15 in 29 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 240740 [details] yet yet another
Attachment 240740 [details] did not pass style-queue: ERROR: Source/WebKit2/NetworkProcess/NetworkCache.h:65: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkCacheKey.cpp:27: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkCache.cpp:207: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkCache.cpp:226: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.h:81: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.h:82: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.h:98: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.mm:46: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.mm:46: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.mm:62: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.mm:62: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.mm:200: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.mm:378: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.mm:394: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 14 in 33 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 240549 [details] another View in context: https://bugs.webkit.org/attachment.cgi?id=240549&action=review > Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.mm:267 > + IPC::ArgumentDecoder decoder(reinterpret_cast<const uint8_t*>(data), size); You can't use the argument decoder for this. We want to be able to modify the IPC serialization format without having to deal with any disk formats. Instead, take a look at KeyedDecoder.
(In reply to comment #27) > You can't use the argument decoder for this. We want to be able to modify > the IPC serialization format without having to deal with any disk formats. > > Instead, take a look at KeyedDecoder. Ok. The general idea is to verify cache entries on read and simply delete the ones that fail. It is not very important that entries stay valid over WebKit changes. antti
Comment on attachment 240740 [details] yet yet another View in context: https://bugs.webkit.org/attachment.cgi?id=240740&action=review > Source/WebKit2/NetworkProcess/NetworkCache.cpp:39 > +#include <dispatch/dispatch.h> Is dispatch used in this file? > Source/WebKit2/NetworkProcess/NetworkCache.cpp:172 > + RefPtr<SharedMemory> sharedMemory = storageEntry.body.size() ? SharedMemory::createFromVMBuffer((void*)storageEntry.body.data(), storageEntry.body.size()) : nullptr; hmm, SharedMemory::createFromVMBuffer is mac only, I wonder why it's not defined inside #ifdefs though. > Source/WebKit2/NetworkProcess/NetworkCache.cpp:173 > + RefPtr<ShareableResource> memoryMap = sharedMemory ? ShareableResource::create(sharedMemory.release(), 0, storageEntry.body.size()) : nullptr; And ShareableResource is mac only too, and defined only when ENABLE_SHAREABLE_RESOURCE is defined. > Source/WebKit2/NetworkProcess/NetworkCache.cpp:178 > + if (memoryMap && memoryMap->createHandle(entry->memoryMapHandle)) > + entry->buffer = entry->memoryMapHandle.tryWrapInSharedBuffer(); > + else > + entry->buffer = WebCore::SharedBuffer::create(storageEntry.body.data(), storageEntry.body.size()); So, I guess we can move all this inside a #if ENABLE(SHAREABLE_RESOURCE) block, falling back to SharedBuffer when not defined > Source/WebKit2/NetworkProcess/NetworkCache.cpp:269 > + case 200: > + case 203: > + case 300: > + case 301: > + case 302: > + case 307: > + case 410: There's HTTPStatusCodes.h in platform/network, that it seems to be used only by EFL port, but maybe we could add the missing status codes to the enum, and use the enum values instead. I think it would make this code easier to read. > Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:241 > +#if ENABLE(NETWORK_CACHE) > +static bool isConditionalRequest(const WebCore::ResourceRequest& request) > +{ > + if (!request.httpHeaderField(WebCore::HTTPHeaderName::IfNoneMatch).isEmpty()) > + return true; > + if (!request.httpHeaderField(WebCore::HTTPHeaderName::IfModifiedSince).isEmpty()) > + return true; > + return false; > +} > +#endif Could we use ResourceRequest::isConditional()? or de we really want to check only these two headers? Could we use httpHeaderFields().contains() instead? > Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:533 > +void NetworkResourceLoader::didFinishCacheRetrieve > + (std::unique_ptr<NetworkCache::Entry> entry) This should be a single line
Created attachment 240872 [details] now without IPC encoding
Attachment 240872 [details] did not pass style-queue: ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.h:81: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.h:82: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.h:98: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkCache.h:65: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkCacheKey.cpp:27: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkCache.cpp:217: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkCache.cpp:236: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkCacheEncoder.h:95: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkCacheEncoder.h:96: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkCacheEncoder.h:97: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkCacheEncoder.h:98: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkCacheEncoder.h:99: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkCacheEncoder.h:100: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkCacheEncoder.h:101: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkCacheEncoder.h:102: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkCacheEncoder.h:103: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkCacheEncoder.h:104: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.mm:46: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.mm:46: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.mm:62: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.mm:62: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.mm:200: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.mm:382: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cocoa/NetworkCacheStorage.mm:398: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 24 in 42 files If any of these errors are false positives, please file a bug against check-webkit-style.
These are my comments based from my experience implementing the libsoup's HTTP cache, I guess this is an initial implementation that will be iterated so feel free to ignore most of my comments: * having 1 file per resource is a source of disk fragmentation, many resources are smaller than disk block size so the cache ends up being bigger than expected (at least this is an issue we have in libsoup) * missing entry eviction based of cache size. That will likely require some other missing pieces, like an LRU for evicting old resources, code to make room for new resources should the cache be full... * key filter is built by transversing the directory, are you planning to have some kind of index file (perhaps with precomputed data) instead of doing that? (the index file brings many other issues BTW specially related to consistency after crashes and so). * shouldn't we have some extra check to avoid issues with clashes in the key filter? (in chromium they have [key,linkedlist] pairs to avoid this IIRC) If I understood this correctly this is a first (very nice!) step, so I guess the most important thing is to have a good API for the cache. Later on, it could be evolved to improve its efficiency like having block-files storage for small resources alà Firefox), mmap'ed writable indexes to reduce the size of the key hashes in memory, etc...
> * having 1 file per resource is a source of disk fragmentation, many > resources are smaller than disk block size so the cache ends up being bigger > than expected (at least this is an issue we have in lib soup) Note that the serialization combines meta data, headers and body data to a single file so even empty resources are typically a few kBs. It is still true that there is about (entry count)*(block size)/2 of wasted disk space. With my current ~97MB cache with 3157 items this is ~7%. But it is not like any other storage and meta-data scheme is overhead free either. > * missing entry eviction based of cache size. That will likely require some > other missing pieces, like an LRU for evicting old resources, code to make > room for new resources should the cache be full... Yeah, size limit is still unimplemented. It will probably be random eviction at least initially. Naive LRU is generally a bad strategy for caches. > * key filter is built by transversing the directory, are you planning to > have some kind of index file (perhaps with precomputed data) instead of > doing that? (the index file brings many other issues BTW specially related > to consistency after crashes and so). Hopefully not. Avoiding global metadata is one of things I'm trying out here. As you note if this works out it will eliminate lots of complexity and whole classes of robustness issues. Note that the initialization really just needs the directory, not the actual cache files or their inodes. This should be quick even with very large caches. > * shouldn't we have some extra check to avoid issues with clashes in the key > filter? (in chromium they have [key,linkedlist] pairs to avoid this IIRC) Clashes are dealt purely as correctness issue by verifying the key against the retrieved cache entry. Since clash probability is very low doing anything more complex doesn't seem justified. Hashes could also be much longer than the current 32 bits without any real design changes. > If I understood this correctly this is a first (very nice!) step, so I guess > the most important thing is to have a good API for the cache. Later on, it > could be evolved to improve its efficiency like having block-files storage > for small resources alà Firefox), mmap'ed writable indexes to reduce the > size of the key hashes in memory, etc... Yeah, the idea is to separate the storage backend from the logic and have a simple internal interface for. People might want to try out completely different designs. With the current implementation I'm going for the "minimal viable" backend. And perhaps pushing the boundary of that a bit too.
> It will probably be random eviction at least initially. Naive LRU is generally a bad strategy for caches. Won't this have repercussions for correctness and performance testing?
> Won't this have repercussions for correctness and performance testing? What do you mean by correctness?
(In reply to comment #34) > > It will probably be random eviction at least initially. Naive LRU is generally a bad strategy for caches. > > Won't this have repercussions for correctness and performance testing? Interesting question. Obviously these problems already exist (since the networking layers beneath us currently likely have caches, and their cache eviction policies are presumably outside of our control). Currently I don't think we do anything to try to specify a particular cache eviction policy during testing - but it seems there is no reason why we could not do so. It certainly seems possible that we could add a different caching mode for testing purposes - e.g. unrestricted cache size, LRU, or pseudo-random with fixed seed. The most important configuration to test is probably the one we plan to ship enabled by default, so initially the right thing to do here is probably just to determine the cache eviction policy we intend to actually use (likely random), and then test against that. But gaining the ability to control the eviction policy certainly seems like a nice side effect of moving persistent caching into WebKit; it definitely seems worth keeping in mind the possibility of taking advantage of this in the future.
Created attachment 241302 [details] patch The patch has the feature enabled on Mac but I would land it disabled and add a few more things (some sort of size limiter at least) before enabling in ToT.
Attachment 241302 [details] did not pass style-queue: ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheKey.cpp:27: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCache.h:65: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h:85: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h:86: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h:102: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:216: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:235: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:46: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:46: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:62: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:62: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:200: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:382: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:398: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 14 in 25 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 241303 [details] now with tests included
Attachment 241303 [details] did not pass style-queue: ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheKey.cpp:27: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCache.h:65: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h:85: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h:86: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h:102: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:216: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:235: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:46: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:46: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:62: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:62: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:200: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:382: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:398: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 14 in 32 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #33) > It will probably be random eviction > at least initially. Naive LRU is generally a bad strategy for caches. Why is LRU expected to perform worse than random eviction?
> Why is LRU expected to perform worse than random eviction? It is very easy to hit pathological cases with LRU. Repeatedly cycle through n unrelated pages with just enough content to overflow the cache -> zero cache hits ever.
> > Won't this have repercussions for correctness and performance testing? > What do you mean by correctness? Correctness testing. Just our own regression tests - hopefully we won't need to disable cache and loading tests as part of this effort, due to increased instability of what is in the cache.
Comment on attachment 241303 [details] now with tests included View in context: https://bugs.webkit.org/attachment.cgi?id=241303&action=review > Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:543 > + if (!entry->memoryMapHandle.isNull()) > + send(Messages::WebResourceLoader::DidReceiveResource(entry->memoryMapHandle, currentTime())); > + else { Could you use #if ENABLE(SHAREABLE_RESOURCE) for this? > Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:98 > + NetworkCacheStorage::Data header(encoder.buffer(), encoder.bufferSize()); Are we copying the data here? The encoder object won't be used anymore after this, could we add a method to take the buffer instead? or use a refcounted bufffer? > Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:186 > + RefPtr<SharedMemory> sharedMemory = storageEntry.body.size() ? SharedMemory::createFromVMBuffer((void*)storageEntry.body.data(), storageEntry.body.size()) : nullptr; > + RefPtr<ShareableResource> memoryMap = sharedMemory ? ShareableResource::create(sharedMemory.release(), 0, storageEntry.body.size()) : nullptr; > + > + if (memoryMap && memoryMap->createHandle(entry->memoryMapHandle)) > + entry->buffer = entry->memoryMapHandle.tryWrapInSharedBuffer(); > + else #if ENABLE(SHAREABLE_RESOURCE) > Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:213 > + String partition = request.cachePartition(); > + if (partition.isEmpty()) > + partition = ASCIILiteral("No partition"); > + return NetworkCacheKey(request.httpMethod(), partition, request.url().string()); Cache partition is also mac only, we should use #if ENABLE(CACHE_PARTITIONING) here. For non mac ports I guess we could use an emptyString instead of "No partition" and handle it as a special case to not create another directory level. But we could do that as a follow up patch when we implement the cache for other ports. > Source/WebKit2/NetworkProcess/cache/NetworkCache.h:62 > + ShareableResource::Handle memoryMapHandle; #if ENABLE(SHAREABLE_RESOURCE) > Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h:35 > +#include <wtf/OSObjectPtr.h> #if PLATFORM(COCOA)
(In reply to comment #33) > > * key filter is built by transversing the directory, are you planning to > > have some kind of index file (perhaps with precomputed data) instead of > > doing that? (the index file brings many other issues BTW specially related > > to consistency after crashes and so). > > Hopefully not. Avoiding global metadata is one of things I'm trying out > here. As you note if this works out it will eliminate lots of complexity and > whole classes of robustness issues. > > Note that the initialization really just needs the directory, not the actual > cache files or their inodes. This should be quick even with very large > caches. At some point we will need to know the size of the files too, to implement the maximum cache size.
(In reply to comment #33) > > * key filter is built by transversing the directory, are you planning to > > have some kind of index file (perhaps with precomputed data) instead of > > doing that? (the index file brings many other issues BTW specially related > > to consistency after crashes and so). > > Hopefully not. Avoiding global metadata is one of things I'm trying out > here. As you note if this works out it will eliminate lots of complexity and > whole classes of robustness issues. > > Note that the initialization really just needs the directory, not the actual > cache files or their inodes. This should be quick even with very large > caches. It should. Anyway I was not really worried about the initialization phase but the fact that we always need an IO operation to read metadata of the cache entry, something that is not needed with a global metadata structure. In any case I don't have data to back the potential performance gain (perhaps the number of times we find an expired resource is so low that it don't pay off) and after years dealing with robustness issues in libsoup I'm also starting to seriously consider the option of not using global metadata at all.
Created attachment 241347 [details] cmake build fix
Attachment 241347 [details] did not pass style-queue: ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheKey.cpp:27: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCache.h:65: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h:85: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h:86: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h:102: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:216: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:235: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:46: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:46: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:62: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:62: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:200: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:382: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:398: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 14 in 33 files If any of these errors are false positives, please file a bug against check-webkit-style.
> At some point we will need to know the size of the files too, to implement the maximum cache size. There is a number of operations that operate on the cache globally, such as "delete all cache entries modified within the last hour". While these are not as common as page loading, dramatic performance problems during these would be unfortunate.
Comment on attachment 241347 [details] cmake build fix View in context: https://bugs.webkit.org/attachment.cgi?id=241347&action=review > Source/WebKit2/config.h:100 > +#ifndef ENABLE_NETWORK_CACHE > +#if PLATFORM(MAC) && ENABLE(NETWORK_PROCESS) > +#define ENABLE_NETWORK_CACHE 1 > +#endif > +#endif Will this enable the cache by default? I don't think we want to do that right now. > Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:145 > + RefPtr<NetworkResourceLoader> loader = this; I think there should be a newline before here. > Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:146 > + NetworkCache::shared().retrieve(originalRequest(), [loader](std::unique_ptr<NetworkCache::Entry> entry) { Is there just a single network cache? It seems like you would want it to be per session? > Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:332 > + double now = currentTime(); > + double responseEndOfValidity = now + WebCore::computeFreshnessLifetimeForHTTPFamily(m_response, now) - WebCore::computeCurrentAge(m_response, now); I think this should use std::chrono. > Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:533 > +void NetworkResourceLoader::didFinishCacheRetrieve(std::unique_ptr<NetworkCache::Entry> entry) I think this should be named didRetrieveCacheEntry or something along those lines. > Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:542 > + send(Messages::WebResourceLoader::DidReceiveResource(entry->memoryMapHandle, currentTime())); This should also use std::chrono instead of currentTime(). > Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:51 > +NetworkCache& NetworkCache::shared() > +{ > + static NeverDestroyed<NetworkCache> instance; > + return instance; > +} Like I said earlier, I don't think we want a singleton network cache - We want one per session or NetworkContext or something. > Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:57 > +bool NetworkCache::initialize(const String& applicationCachePath) This shouldn't be called applicationCachePath because it can be confused with the app cache. > Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:97 > + auto timeStamp = std::chrono::duration_cast<std::chrono::milliseconds>(std::chrono::duration<double>(currentTime())); This can just use std::chrono::system_clock::current_time() instead of currentTime(). > Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:103 > + return NetworkCacheStorage::Entry { timeStamp, header, body }; Very nice with the uniform initialization here! > Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:137 > + fprintf(stderr, "decodeStorageEntry\n"); Lol wut. > Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:164 > + auto timeStamp = std::chrono::duration_cast<std::chrono::duration<double>>(storageEntry.timeStamp); > + double age = WebCore::computeCurrentAge(cachedResponse, timeStamp.count()); > + double lifetime = WebCore::computeFreshnessLifetimeForHTTPFamily(cachedResponse, timeStamp.count()); Would be nice if this all used std::chrono. > Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:172 > + return nullptr; Should the entry be removed from the cache in this case? > Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:181 > + RefPtr<SharedMemory> sharedMemory = storageEntry.body.size() ? SharedMemory::createFromVMBuffer((void*)storageEntry.body.data(), storageEntry.body.size()) : nullptr; Why does this have to cast to void*? > Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:182 > + RefPtr<ShareableResource> memoryMap = sharedMemory ? ShareableResource::create(sharedMemory.release(), 0, storageEntry.body.size()) : nullptr; Why is this called memoryMap? What is it a map of? > Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:212 > + if (partition.isEmpty()) > + partition = ASCIILiteral("No partition"); Can't we let an empty partition mean no partition? Seems stupid to write out "No partition" to disk (if that's what we're doing). > Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:216 > +void NetworkCache::retrieve(const WebCore::ResourceRequest& originalRequest, std::function<void (std::unique_ptr<Entry>)> completion) completion should be completionHandler. > Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:223 > + completion(nullptr); It's dangerous that this can call the completionHandler from inside the retrieve call. You should either document this, or call it later on the main thread. > Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:227 > + double start = currentTime(); std::chrono. > Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:237 > + // FIXME: With C++14 this could use unique_ptr and initilized lambda capture Typo, “initilized”. > Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:238 > + auto capture = std::make_shared<Capture>(Capture { originalRequest, completion }); Can use WTF::move to move the original request. > Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:259 > + if (originalRequest.httpMethod() != "GET") { I think you want equalIgnoringCase here? > Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:278 > + case 200: > + case 203: > + case 300: > + case 301: > + case 302: > + case 307: > + case 410: It would be nice if each of these cases had a comment stating the name of each status code. > Source/WebKit2/NetworkProcess/cache/NetworkCacheKey.h:68 > + HashType m_hash; I think you want to store some type of hash version here as well so we can bump the version if we decide to change the WTF hash (which is likely to happen in the future). > Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h:62 > + Data(OSObjectPtr<dispatch_data_t>); Should be explicit. > Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h:81 > + std::chrono::milliseconds timeStamp; std::chrono - very nice! > Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h:100 > + struct Retrieve { RetrieveOperation? > Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:46 > +static void traverseDirectory(const String& path, uint8_t type, std::function<void (const String&)> function) { I think this can be a const std::function, or maybe even make this a function template (function having a template parameter type). > Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:70 > + traverseDirectory(cachePath, DT_DIR, [&cachePath, &function](const String& subdirName) { > + String partitionPath = WebCore::pathByAppendingComponent(cachePath, subdirName); > + traverseDirectory(partitionPath, DT_REG, [&function, &partitionPath](const String& fileName) { > + if (fileName.length() != 8) > + return; > + function(fileName, partitionPath); > + }); > + }); What happens if there are symlinks inside the directory that links to the parent here? > Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:181 > + dispatch_io_set_low_water(channel.get(), SIZE_MAX); Should use std::numeric_limits here instead of SIZE_MAX. > Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:258 > + if (metaData.headerChecksum != hashData(headerData.get())) { Should version the hash here too. > Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:314 > + Vector<uint8_t> filler(dataOffset - headerSize, 0); This can be Vector<uint8_t, 4096> if you want to ;) > Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:329 > + unlink(path.data()); File I/O on the main thread! > Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:364 > +void NetworkCacheStorage::dispatchPendingRetrieves() retrieveOperations. > Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:368 > + const unsigned maximumActiveRetrieveCount = 5; retrieveOperationCount? > Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:403 > + dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_BACKGROUND, 0), [this, key, entry, cachePathCapture, completion]() { It'd be somewhat cleaner to create a queue for the network cache storage instead of using the global queue everywhere. > Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:433 > + dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), [directoryPathCapture] { What happens if you try to update the cache while it's being cleared? > Source/WebKit2/NetworkProcess/cocoa/NetworkProcessCocoa.mm:149 > + if (NetworkCache::shared().isEnabled()) { > + NetworkCache::shared().setMaximumSize(urlCacheDiskCapacity); > + return; > + } Like I've stated before, we need the ability to support multiple caches in the network process - shouldn't be too hard to do. > Source/WebKit2/Platform/IPC/ArgumentDecoder.h:44 > + size_t currentOffset() const { return m_bufferPos - m_buffer; } I think this can be removed now.
(In reply to comment #50) > Will this enable the cache by default? I don't think we want to do that > right now. No. As I mentioned above I'll land disabled. It is enabled just so the bots see the code.
(In reply to comment #50) > Is there just a single network cache? It seems like you would want it to be > per session? The initial patch has a singleton cache but there are very few singleton assumptions. It won't be difficult to transition to many-caches model later as needed. > > Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:172 > > + return nullptr; > > Should the entry be removed from the cache in this case? It is. We return decoding failure to the storage layer (from retrieve lambda) and the entry gets deleted. > Why does this have to cast to void*? "Cannot initialize a parameter of type 'void *' with an rvalue of type 'const uint8_t *' (aka 'const unsigned char *')". I'll switch to C++ cast at least. > > Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:212 > > + if (partition.isEmpty()) > > + partition = ASCIILiteral("No partition"); > > Can't we let an empty partition mean no partition? Seems stupid to write out > "No partition" to disk (if that's what we're doing). Currently cache entries are organised directory-per-partition on disk. It looks somewhat cleaner when non-partitioned entries also go to a directory of their own (called "No partition" for now, space guarantees no clashes with partitions). > > Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:259 > > + if (originalRequest.httpMethod() != "GET") { > > I think you want equalIgnoringCase here? I believe the method is always uppercase and all the code using equalIgnoringCase is just cargo-culting. But I may be wrong! > I think you want to store some type of hash version here as well so we can > bump the version if we decide to change the WTF hash (which is likely to > happen in the future). The general idea has been that we don't mind too much if cache entries become invalid due to format changes, just that we can detect now-invalid entries and ignore them. They'll get freshened quickly in normal browser use. > What happens if there are symlinks inside the directory that links to the > parent here? I think nothing. We ignore DT_LNK entries when traversing. If someone adds symlinks to cache directories we won't empty them so can't delete them either. > File I/O on the main thread! Oh no! > What happens if you try to update the cache while it's being cleared? New entries may be created during clearing and depending where they are they may get immediately deleted. I'm not sure this is a practical problem. Thanks for reviews, I'll update the patch.
Created attachment 241863 [details] implement review comments Implemented most of these. Didn't convert to std::chrono universally yet because that requires some more WebCore side refactoring.
Attachment 241863 [details] did not pass style-queue: ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCache.h:66: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h:86: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h:87: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h:103: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:214: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:233: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:47: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:63: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:63: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:205: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:395: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:411: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 12 in 32 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 241955 [details] fix LOG_DISABLED build
Attachment 241955 [details] did not pass style-queue: ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCache.h:66: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h:86: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h:87: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h:103: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:214: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:233: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:47: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:63: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:63: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:205: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:395: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:411: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 12 in 32 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 241955 [details] fix LOG_DISABLED build View in context: https://bugs.webkit.org/attachment.cgi?id=241955&action=review > Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:47 > +static void traverseDirectory(const String& path, uint8_t type, const Function& function) { { on the wrong line. > Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:63 > +static void traverseCacheFiles(const String& cachePath, std::function<void (const String& fileName, const String& partitionPath)> function) { { on the wrong line. > Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:205 > + EntryMetaData(const NetworkCacheKey& key) : cacheStorageVersion(NetworkCacheStorage::version), key(key) { } explicit?
https://trac.webkit.org/r177294 (landed disabled)
Comment on attachment 241955 [details] fix LOG_DISABLED build View in context: https://bugs.webkit.org/attachment.cgi?id=241955&action=review > Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:150 > + LOG(NetworkCache, "(NetworkProcess) varying header mistmatch\n"); Typo: mistmatch => mismatch? > Source/WebKit2/NetworkProcess/cache/NetworkCache.h:36 > +#include <wtf/BloomFilter.h> > +#include <wtf/Deque.h> > +#include <wtf/HashSet.h> Are these includes still needed? > Source/WebKit2/NetworkProcess/cache/NetworkCache.h:75 > + String fileNameForURL(const WebCore::URL&); > + String directoryPathForCachePartition(const String&); > + String filePathForRequest(const WebCore::ResourceRequest&); Can these be removed? It looks like they were moved to NetworkCacheStorage and no longer have an imp in this class. > Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h:36 > +#include <wtf/RetainPtr.h> Is this include still needed? > Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:172 > + case IOChannelType::Read: > + oflag = O_RDONLY | O_NONBLOCK; > + mode = 0; > + } I would still expect a "break;" here anyways to avoid possible compiler warnings. > Source/WebKit2/NetworkProcess/cocoa/NetworkProcessCocoa.mm:68 > + NSURLCache *URLCache = [[NSURLCache alloc] initWithMemoryCapacity:0 diskCapacity:0 diskPath:nil]; Possible leak / analyzer warning. You could adopt into a RetainPtr (see other cases use of adoptNS). This object will probably be around forever, but we should exercise good memory management.
Fixed those in https://trac.webkit.org/r177356