Bug 142925 – [WK2] NetworkCache retrievals sometimes fail on browser startup

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Chris Dumez]

NetworkCache retrievals sometimes fail on browser startup for resources that are actually cached. The reason is that we are using a bloom filter for performance reasons to avoid unnecessary disk I/O and this bloom filter is populated on start up in a background thread by traversing the cache files on disk. However, when restoring the tabs on start-up we query this bloom filter before it is completely populated and we fails to retrieve the cached entry because we think it is not there.

Radar: <rdar://problem/20245368>

[Chris Dumez]

Created attachment 249138 [details]
Patch

[Darin Adler]

Comment on attachment 249138 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=249138&action=review

> Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h:112
> +    std::atomic<bool> m_isPopulatingContentsFilter { false };

Doesn’t seem good to have a time window where this is false at first, and then have it become true later. I think this should be:

    std::atomic<bool> m_hasPopulatedContentsFilter { false };

Then we could just set it to true once we are done with that.

[Antti Koivisto]

Comment on attachment 249138 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=249138&action=review

> Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.cpp:377
> -    if (!m_contentsFilter.mayContain(key.shortHash())) {
> +    if (!m_isPopulatingContentsFilter && !m_contentsFilter.mayContain(key.shortHash())) {

There are number of other places where we check the bloom filter. Might those need this check too?

In any case it would be nicer to factor this into a function.

[Chris Dumez]

Created attachment 249142 [details]
Patch

[Chris Dumez]

(In reply to comment #2)
> Comment on attachment 249138 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=249138&action=review
> 
> > Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h:112
> > +    std::atomic<bool> m_isPopulatingContentsFilter { false };
> 
> Doesn’t seem good to have a time window where this is false at first, and
> then have it become true later. I think this should be:
> 
>     std::atomic<bool> m_hasPopulatedContentsFilter { false };
> 
> Then we could just set it to true once we are done with that.

Oh, I have just seen your comment. Yes, this makes sense. I'll update.

[Chris Dumez]

Created attachment 249143 [details]
Patch

[Chris Dumez]

Comment on attachment 249143 [details]
Patch

Clearing flags on attachment: 249143

Committed r181816: <http://trac.webkit.org/changeset/181816>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

I don't think that an atomic value is sufficient - all it does is enforce consistency when accessing itself, not when accessing the filter data. You need an actual mutex or semaphore, and there are surprisingly few opportunities for smarts.

[Gavin Barraclough]

(In reply to comment #9)
> I don't think that an atomic value is sufficient - all it does is enforce
> consistency when accessing itself, not when accessing the filter data. You
> need an actual mutex or semaphore, and there are surprisingly few
> opportunities for smarts.

I think all errors are fine in this case. False negative from the bloom filter would result in a cache miss, false positive would result in looking up in the file system index. Additional locking overhead would probably be a net loss.

('atomic' might be entirely moot, though it may usefully be preventing the compiler from coalescing bits into a bitfield.)

[Alexey Proskuryakov]

> Additional locking overhead would probably be a net loss.

Sounds good to me, as long as the filter structure wouldn't cause a crash when incompletely visible to the thread using it.