Add logging to measure the network cache efficacy. This is part 1, containing the console logging code inside the NetworkCache. The diagnostic logging wiring will be added in a follow-up patch. Radar: <rdar://problem/19632080>
Created attachment 246063 [details] WIP patch Still needs another pass but uploading already in case Antti has some early feedback on the overall approach.
Attachment 246063 [details] did not pass style-queue: ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h:144: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:256: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:142: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:167: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorageCocoa.mm:470: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 5 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 246076 [details] WIP Patch
Attachment 246076 [details] did not pass style-queue: ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheFileSystemPosix.h:56: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStatsStorageCocoa.mm:30: You should not add a blank line before implementation file's own header. [build/include_order] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStatsStorageCocoa.mm:93: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStatsStorage.h:42: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 4 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 246082 [details] WIP Patch
Attachment 246082 [details] did not pass style-queue: ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheFileSystemPosix.h:56: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStatsStorageCocoa.mm:30: You should not add a blank line before implementation file's own header. [build/include_order] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStatsStorageCocoa.mm:109: Extra space before last semicolon. If this should be an empty statement, use { } instead. [whitespace/semicolon] [5] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStatsStorageCocoa.mm:165: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStatsStorage.h:42: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 5 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 246083 [details] WIP Patch Ok, this latest patch takes into consideration Antti's feedback from IRC, except the setting to make this logging conditional at run time (I'll add that tomorrow and double check everything). In the mean time, please let me know if you have other comments.
Attachment 246083 [details] did not pass style-queue: ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheFileSystemPosix.h:56: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStatsStorageCocoa.mm:30: You should not add a blank line before implementation file's own header. [build/include_order] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStatsStorageCocoa.mm:109: Extra space before last semicolon. If this should be an empty statement, use { } instead. [whitespace/semicolon] [5] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStatsStorageCocoa.mm:165: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStatsStorage.h:42: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 5 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 246090 [details] WIP Patch
Attachment 246090 [details] did not pass style-queue: ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheFileSystemPosix.h:56: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStatsStorageCocoa.mm:30: You should not add a blank line before implementation file's own header. [build/include_order] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStatsStorageCocoa.mm:110: Extra space before last semicolon. If this should be an empty statement, use { } instead. [whitespace/semicolon] [5] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStatsStorageCocoa.mm:166: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStatsStorage.h:42: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 5 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 246123 [details] WIP Patch
Attachment 246123 [details] did not pass style-queue: ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheFileSystemPosix.h:56: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStatsStorageCocoa.mm:30: You should not add a blank line before implementation file's own header. [build/include_order] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStatsStorageCocoa.mm:110: Extra space before last semicolon. If this should be an empty statement, use { } instead. [whitespace/semicolon] [5] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStatsStorageCocoa.mm:167: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStatsStorage.h:42: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStatsStorage.h:54: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 6 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 246131 [details] WIP Patch
Attachment 246131 [details] did not pass style-queue: ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheFileSystemPosix.h:56: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStatsStorageCocoa.mm:30: You should not add a blank line before implementation file's own header. [build/include_order] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStatsStorageCocoa.mm:110: Extra space before last semicolon. If this should be an empty statement, use { } instead. [whitespace/semicolon] [5] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStatsStorageCocoa.mm:167: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStatsStorage.h:42: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStatsStorage.h:54: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 6 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 246139 [details] Patch
Attachment 246139 [details] did not pass style-queue: ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheFileSystemPosix.h:56: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 1 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 246141 [details] Patch
Attachment 246141 [details] did not pass style-queue: ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheFileSystemPosix.h:56: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 1 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 246141 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=246141&action=review r+ since there will be subsequent patches. I'm still bit confused by some aspects here. > Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:58 > + m_statisticsStorage = NetworkCacheStatistics::open(cachePath); m_statisticsStorage -> m_statistics > Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:231 > + WebCore::URL requestURL = originalRequest.url(); > + NetworkCacheKey storageKey = makeCacheKey(originalRequest); > if (!canRetrieve(originalRequest)) { > + if (isEfficacyLoggingEnabled()) { > + m_statisticsStorage->wasPreviouslySeen(storageKey, [this, storageKey, requestURL](bool wasPreviouslySeen) { You are capturing 'this' just so you can call back to m_statisticsStorage, not a very nice pattern. Generally I would prefer encapsulating more of this to NetworkCacheStatistics: if (m_statistics) m_statistics->recordNotUsingCacheForRequest(originalRequest); and move the equivalent code there. I'd like NetworkCache to be about HTTP semantics. > Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:234 > + if (wasPreviouslySeen) { > + LOG(NetworkCache, "(NetworkProcess) %s was previously seen but wasn't cacheable", requestURL.string().ascii().data()); > + // FIXME: Do diagnostic logging. The log message here is not really correct. Cacheability is a property of the response, not request. Here we filter out some request that we know can't be handled by the cache. > Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:257 > + if (isEfficacyLoggingEnabled()) { > + m_statisticsStorage->wasPreviouslySeen(storageKey, [this, storageKey, requestURL](bool wasPreviouslySeen) { > + if (wasPreviouslySeen) { > + LOG(NetworkCache, "(NetworkProcess) %s was previously cached but is no longer in the cache", requestURL.string().ascii().data()); > + // FIXME: Do diagnostic logging. > + } else > + m_statisticsStorage->markAsSeen(storageKey); > + }); > + } Similarly this would be if (m_statistics) m_statistics->recordRetrieveFailed(originalRequest); I'm confused what markAsSeen() indicates. It probably needs a more descriptive name. Is it "did we ever try to request this resource"? If so shouldn't you also set it when the retrieve succeeds? > Source/WebKit2/NetworkProcess/cache/NetworkCacheStatistics.h:39 > + typedef std::function<void (bool wasPreviouslySeen)> CompletionHandler; This could use a less generic name than "CompletionHandler" > Source/WebKit2/NetworkProcess/cache/NetworkCacheStatistics.h:43 > + void wasPreviouslySeen(const NetworkCacheKey&, const CompletionHandler&) const; queryPreviouslySeen? > Source/WebKit2/NetworkProcess/cache/NetworkCacheStatisticsCocoa.mm:104 > + // Create empty file in cache statistics folder. > + String statisticsPartitionPath = partitionPath; > + statisticsPartitionPath.replace(cachePath, cacheStatisticsPath); > + createEmptyFile(WebCore::pathByAppendingComponent(statisticsPartitionPath, fileName)); This is a rather funky way of storing information. Something bit more traditional (like SQLite) might be appropriate for statistics collection.
Created attachment 246184 [details] Patch
Now using SQLite as backend.
Attachment 246184 [details] did not pass style-queue: ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheFileSystemPosix.h:56: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 1 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 246189 [details] Patch
Attachment 246189 [details] did not pass style-queue: ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheFileSystemPosix.h:56: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 1 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 246197 [details] Patch
Attachment 246197 [details] did not pass style-queue: ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheFileSystemPosix.h:56: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 1 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 246197 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=246197&action=review > Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:256 > auto decodedEntry = decodeStorageEntry(*entry, originalRequest); > bool success = !!decodedEntry; > + if (m_statistics) { > + if (success) > + m_statistics->recordUsingCachedEntry(storageKey, originalRequest); > + else > + m_statistics->recordNotUsingCachedEntry(storageKey, originalRequest); > + } This could perhaps be a single function with a success argument. We should add the reason we failed to use the entry at some point. > Source/WebKit2/NetworkProcess/cache/NetworkCacheStatistics.h:71 > + BloomFilter<20> m_everRequestedFilter; Was there some indication that this large filter is actually needed? The simple queries done here might be pretty well-optimized in SQLite and lookups happen in a low-priority dispatch queue so shouldn't affect normal cache operation. > Source/WebKit2/NetworkProcess/cache/NetworkCacheStatisticsCocoa.mm:306 > +bool NetworkCacheStatistics::addHashToDatabase(NetworkCacheKey::HashType hash) > +{ > + ASSERT(!RunLoop::isMain()); > + ASSERT(WebCore::SQLiteDatabaseTracker::hasTransactionInProgress()); > + ASSERT(m_database.isOpen()); > + > + WebCore::SQLiteStatement statement(m_database, ASCIILiteral("INSERT INTO AlreadyRequested (hash) VALUES (?)")); > + if (statement.prepare() != WebCore::SQLResultOk) > + return false; > + > + statement.bindInt64(1, hash); Note that bug 141356 makes NetworkCacheKey::HashType an MD5 digest so this needs some adjustment, perhaps switching to key.hashAsString(). Similarly key.hash() -> key.shortHash() if you need 32bits.
Comment on attachment 246197 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=246197&action=review >> Source/WebKit2/NetworkProcess/cache/NetworkCacheStatistics.h:71 >> + BloomFilter<20> m_everRequestedFilter; > > Was there some indication that this large filter is actually needed? The simple queries done here might be pretty well-optimized in SQLite and lookups happen in a low-priority dispatch queue so shouldn't affect normal cache operation. Without the bloom filter, we'll get ~2x the amount of disk I/O for each resource request. That said, this is a simple DB query on an indexed column, in a background thread. Since this is merely used for logging, we don't really need to worry about latency. I'll test locally without the bloom filter and see how it goes. Worse case scenario, we can always reintroduce the bloom filter if it causes regressions. >> Source/WebKit2/NetworkProcess/cache/NetworkCacheStatisticsCocoa.mm:306 >> + statement.bindInt64(1, hash); > > Note that bug 141356 makes NetworkCacheKey::HashType an MD5 digest so this needs some adjustment, perhaps switching to key.hashAsString(). > > Similarly key.hash() -> key.shortHash() if you need 32bits. Ok, I'll adjust accordingly, assuming you land your change before me.
Created attachment 246220 [details] Patch
Attachment 246220 [details] did not pass style-queue: ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheFileSystemPosix.h:56: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 1 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 246221 [details] Patch
It'd be great if you could have one last look Antti. I made the following changes: - Removed the bloom filter for now - Store new MD5 hashes as Strings in the database
Attachment 246221 [details] did not pass style-queue: ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheFileSystemPosix.h:56: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 1 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 246197 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=246197&action=review >> Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:256 >> + } > > This could perhaps be a single function with a success argument. > > We should add the reason we failed to use the entry at some point. Oh, and I made this change too.
Comment on attachment 246221 [details] Patch Clearing flags on attachment: 246221 Committed r179804: <http://trac.webkit.org/changeset/179804>
All reviewed patches have been landed. Closing bug.