NetworkCache efficacy logging is using too much CPU

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

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>

[Chris Dumez]

Created attachment 247704 [details]
Patch

[Chris Dumez]

Created attachment 247721 [details]
Patch

[Chris Dumez]

Created attachment 247724 [details]
Patch

[Chris Dumez]

Created attachment 247732 [details]
Patch

[Antti Koivisto]

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.

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 247787 [details]
Patch

[Antti Koivisto]

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.

[Chris Dumez]

(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.

[Chris Dumez]

(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.

[Antti Koivisto]

> 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)

[Antti Koivisto]

> 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.

[Chris Dumez]

Created attachment 247881 [details]
Patch

[WebKit Commit Bot]

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.

[Antti Koivisto]

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.

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 247963 [details]
Patch

[WebKit Commit Bot]

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.

[WebKit Commit Bot]

Comment on attachment 247963 [details]
Patch

Clearing flags on attachment: 247963

Committed r181086: <http://trac.webkit.org/changeset/181086>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.