NetworkCache efficacy logging is using too much CPU. Around ~11% of the NetworkProcess' CPU usage on a PLT on iPhone. We should improve this and re-enable it. Radar: <rdar://problem/19632080>
Created attachment 247704 [details] Patch
Created attachment 247721 [details] Patch
Created attachment 247724 [details] Patch
Created attachment 247732 [details] Patch
Comment on attachment 247732 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=247732&action=review Looks fine except... > Source/WebKit2/NetworkProcess/cache/NetworkCacheStatisticsCocoa.mm:57 > + if (!result) { > LOG_ERROR("Network cache statistics: failed to execute statement \"%s\" error \"%s\"", sql.utf8().data(), database.lastErrorMsg()); > + ASSERT_NOT_REACHED(); Why make these fatal? > Source/WebKit2/NetworkProcess/cache/NetworkCacheStatisticsCocoa.mm:311 > + HashSet<String> hashesToAdd; > + hashesToAdd.swap(m_hashesToAdd); > + HashMap<String, NetworkCache::StoreDecision> storeDecisionsToAdd; > + storeDecisionsToAdd.swap(m_storeDecisionsToAdd); > > shrinkIfNeeded(); > + > + dispatch_async(m_backgroundIOQueue.get(), [this, hashesToAdd, storeDecisionsToAdd] { This isn't thread safe because String refcounting is not thread safe. You need to take care that you pass isolated copies to the queue.
Comment on attachment 247732 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=247732&action=review >> Source/WebKit2/NetworkProcess/cache/NetworkCacheStatisticsCocoa.mm:57 >> + ASSERT_NOT_REACHED(); > > Why make these fatal? To make it more obvious when you get your queries wrong. There should no no other reason for them to fail. When refactoring, I got one of my queries wrong and it took me a while to figure it out because nothing was complaining. >> Source/WebKit2/NetworkProcess/cache/NetworkCacheStatisticsCocoa.mm:311 >> + dispatch_async(m_backgroundIOQueue.get(), [this, hashesToAdd, storeDecisionsToAdd] { > > This isn't thread safe because String refcounting is not thread safe. You need to take care that you pass isolated copies to the queue. Ok.
Created attachment 247787 [details] Patch
Comment on attachment 247787 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=247787&action=review > Source/WebKit2/NetworkProcess/cache/NetworkCacheStatisticsCocoa.mm:332 > + auto storeDecisionsToAdd = isolatedCopy(m_storeDecisionsToAdd); > + m_storeDecisionsToAdd.clear(); > > shrinkIfNeeded(); > + > + dispatch_async(m_backgroundIOQueue.get(), [this, hashesToAdd, storeDecisionsToAdd] { This still don't look thread safe. The vector gets copied to lambda, the contained strings get reffed by the copy, the original gets dereffed in main thread while the copy is dereffed in the queue. We have StringCapture for single strings but here you are probably better of bundling all the stuff you want to pass to a heap allocated struct. C++14 will solve this problem by allowing move initialization of the lambda capture.
(In reply to comment #8) > Comment on attachment 247787 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=247787&action=review > > > Source/WebKit2/NetworkProcess/cache/NetworkCacheStatisticsCocoa.mm:332 > > + auto storeDecisionsToAdd = isolatedCopy(m_storeDecisionsToAdd); > > + m_storeDecisionsToAdd.clear(); > > > > shrinkIfNeeded(); > > + > > + dispatch_async(m_backgroundIOQueue.get(), [this, hashesToAdd, storeDecisionsToAdd] { > > This still don't look thread safe. The vector gets copied to lambda, the > contained strings get reffed by the copy, the original gets dereffed in main > thread while the copy is dereffed in the queue. > > We have StringCapture for single strings but here you are probably better of > bundling all the stuff you want to pass to a heap allocated struct. > > C++14 will solve this problem by allowing move initialization of the lambda > capture. I initially tried to use a Vector<StringCapture> but this did not compile because StringCapture doesn't have an assignment operator.
(In reply to comment #8) > Comment on attachment 247787 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=247787&action=review > > > Source/WebKit2/NetworkProcess/cache/NetworkCacheStatisticsCocoa.mm:332 > > + auto storeDecisionsToAdd = isolatedCopy(m_storeDecisionsToAdd); > > + m_storeDecisionsToAdd.clear(); > > > > shrinkIfNeeded(); > > + > > + dispatch_async(m_backgroundIOQueue.get(), [this, hashesToAdd, storeDecisionsToAdd] { > > This still don't look thread safe. The vector gets copied to lambda, the > contained strings get reffed by the copy, the original gets dereffed in main > thread while the copy is dereffed in the queue. > > We have StringCapture for single strings but here you are probably better of > bundling all the stuff you want to pass to a heap allocated struct. > > C++14 will solve this problem by allowing move initialization of the lambda > capture. Could you please clarify what you are proposing? I'm afraid I don't understand.
> Could you please clarify what you are proposing? I'm afraid I don't > understand. Something like struct Capture { Vector<String> strings; ... }; Capture* capture = new Capture { isolatedCopy(m_strings), ... } dsipatch_async(..., [capture] { delete capture; } (shared_ptr works too if you want to avoid explicit new/delete)
> I initially tried to use a Vector<StringCapture> but this did not compile > because StringCapture doesn't have an assignment operator. Maybe you could just add it. It doesn't have one because there didn't seem to be need.
Created attachment 247881 [details] Patch
Attachment 247881 [details] did not pass style-queue: ERROR: Source/WTF/wtf/text/WTFString.h:642: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 247881 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=247881&action=review > Source/WTF/wtf/HashMap.h:512 > + iterator it = collection.begin(); > + iterator end = collection.end(); > + for (unsigned i = 0; it != end; ++it, ++i) > + vector[i] = { (*it).key, (*it).value }; This might look slightly nicer with range-for. > Source/WTF/wtf/text/WTFString.h:641 > + StringCapture() = default; I'm not sure '= default;' is better than { } > Source/WebKit2/NetworkProcess/cache/NetworkCacheStatisticsCocoa.mm:46 > +static const double minWriteIntervalSeconds = 10; minimum std::chrono::duration would be nicer and allow losing 'Seconds' qualifier. > Source/WebKit2/NetworkProcess/cache/NetworkCacheStatisticsCocoa.mm:72 > + if (statement.step() != WebCore::SQLResultDone) { > LOG_ERROR("Network cache statistics: failed to execute statement \"%s\" error \"%s\"", statement.query().utf8().data(), statement.database().lastErrorMsg()); > + ASSERT_NOT_REACHED(); > + return false; Is SQL failure in logging code really important enough to interrupt someone debugging something completely different? (this and other new asserts). If this can't happen then we should just have the assert without branch. If it can happen then it shouldn't assert.
Comment on attachment 247881 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=247881&action=review >> Source/WTF/wtf/HashMap.h:512 >> + vector[i] = { (*it).key, (*it).value }; > > This might look slightly nicer with range-for. But I need the index for vector assignment. Otherwise I guess I could use Vector::append() instead. However, I followed the pattern used in all the copyToVector() utility functions.
Created attachment 247963 [details] Patch
Attachment 247963 [details] did not pass style-queue: ERROR: Source/WTF/wtf/text/WTFString.h:642: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 247963 [details] Patch Clearing flags on attachment: 247963 Committed r181086: <http://trac.webkit.org/changeset/181086>
All reviewed patches have been landed. Closing bug.