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>
Created attachment 249138 [details] Patch
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.
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.
Created attachment 249142 [details] Patch
(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.
Created attachment 249143 [details] Patch
Comment on attachment 249143 [details] Patch Clearing flags on attachment: 249143 Committed r181816: <http://trac.webkit.org/changeset/181816>
All reviewed patches have been landed. Closing bug.
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.
(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.)
> 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.