Changeset 197879 in webkit


Ignore:
Timestamp:
Mar 9, 2016, 1:22:36 PM (9 years ago)
Author:
Chris Dumez
Message:

Speculative disk cache resource revalidations are sometimes wasted
​https://bugs.webkit.org/show_bug.cgi?id=155187
<rdar://problem/25032905>

Reviewed by Antti Koivisto.

Speculative disk cache resource revalidations were sometimes wasted.

We would sometimes correctly revalidate a resource but the
NetworkResourceLoader then either:

  1. Fail to reuse the speculatively validated entry
  2. Reuse the speculatively validated entry but then validate it again

Bug 1 was caused by the revalidated entry key sometimes being
different from the cached entry key. This could happen when
revalidation fails (the server did not send back a 304) in
which case we call NetworkCache::store() which creates a new
cache Entry, generating a cache key from our revalidation
request. If the original request has a cache partition or a
range, then the keys would not match because we did not set
the cache partition or the range on the revalidation request.
This has been addressed by setting the cache partition on the
revalidation request in constructRevalidationRequest() and by
not doing revalidation if the original request had a 'range'
header.

Bug 2 was caused by us marking a speculatively revalidated entry
as "not needing revalidating" only in Cache::update(). Cache::update()
is only called in the case the revalidation was successful (server
returned a 304). If revalidation was not successful, Cache::store()
would be called instead was we would fail to update the
needsRevalidation flag. NetworkResourceLoader would then validate
again the resource that was already speculatively revalidated.
To address the problem, we now update the 'needsRevalidation' flag
as soon as the speculative revalidation completes, in
SpeculativeLoad::didComplete().

  • NetworkProcess/cache/NetworkCache.cpp:

(WebKit::NetworkCache::Cache::retrieve):
(WebKit::NetworkCache::makeCacheKey):
(WebKit::NetworkCache::Cache::update):

  • NetworkProcess/cache/NetworkCacheEntry.cpp:

(WebKit::NetworkCache::Entry::setNeedsValidation):

  • NetworkProcess/cache/NetworkCacheEntry.h:
  • NetworkProcess/cache/NetworkCacheKey.cpp:

(WebKit::NetworkCache::noPartitionString):
(WebKit::NetworkCache::Key::Key):
(WebKit::NetworkCache::Key::hasPartition):

  • NetworkProcess/cache/NetworkCacheKey.h:
  • NetworkProcess/cache/NetworkCacheSpeculativeLoad.cpp:

(WebKit::NetworkCache::SpeculativeLoad::didComplete):

  • NetworkProcess/cache/NetworkCacheSpeculativeLoadManager.cpp:

(WebKit::NetworkCache::constructRevalidationRequest):
(WebKit::NetworkCache::SpeculativeLoadManager::retrieveEntryFromStorage):
(WebKit::NetworkCache::SpeculativeLoadManager::revalidateEntry):

Location:
trunk/Source/WebKit2
Files:
8 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebKit2/ChangeLog

    r197870 r197879  
     12016-03-09  Chris Dumez  <cdumez@apple.com>
     2
     3        Speculative disk cache resource revalidations are sometimes wasted
     4        https://bugs.webkit.org/show_bug.cgi?id=155187
     5        <rdar://problem/25032905>
     6
     7        Reviewed by Antti Koivisto.
     8
     9        Speculative disk cache resource revalidations were sometimes wasted.
     10
     11        We would sometimes correctly revalidate a resource but the
     12        NetworkResourceLoader then either:
     13        1. Fail to reuse the speculatively validated entry
     14        2. Reuse the speculatively validated entry but then validate it again
     15
     16        Bug 1 was caused by the revalidated entry key sometimes being
     17        different from the cached entry key. This could happen when
     18        revalidation fails (the server did not send back a 304) in
     19        which case we call NetworkCache::store() which creates a new
     20        cache Entry, generating a cache key from our revalidation
     21        request. If the original request has a cache partition or a
     22        range, then the keys would not match because we did not set
     23        the cache partition or the range on the revalidation request.
     24        This has been addressed by setting the cache partition on the
     25        revalidation request in constructRevalidationRequest() and by
     26        not doing revalidation if the original request had a 'range'
     27        header.
     28
     29        Bug 2 was caused by us marking a speculatively revalidated entry
     30        as "not needing revalidating" only in Cache::update(). Cache::update()
     31        is only called in the case the revalidation was successful (server
     32        returned a 304). If revalidation was not successful, Cache::store()
     33        would be called instead was we would fail to update the
     34        needsRevalidation flag. NetworkResourceLoader would then validate
     35        again the resource that was already speculatively revalidated.
     36        To address the problem, we now update the 'needsRevalidation' flag
     37        as soon as the speculative revalidation completes, in
     38        SpeculativeLoad::didComplete().
     39
     40        * NetworkProcess/cache/NetworkCache.cpp:
     41        (WebKit::NetworkCache::Cache::retrieve):
     42        (WebKit::NetworkCache::makeCacheKey):
     43        (WebKit::NetworkCache::Cache::update):
     44        * NetworkProcess/cache/NetworkCacheEntry.cpp:
     45        (WebKit::NetworkCache::Entry::setNeedsValidation):
     46        * NetworkProcess/cache/NetworkCacheEntry.h:
     47        * NetworkProcess/cache/NetworkCacheKey.cpp:
     48        (WebKit::NetworkCache::noPartitionString):
     49        (WebKit::NetworkCache::Key::Key):
     50        (WebKit::NetworkCache::Key::hasPartition):
     51        * NetworkProcess/cache/NetworkCacheKey.h:
     52        * NetworkProcess/cache/NetworkCacheSpeculativeLoad.cpp:
     53        (WebKit::NetworkCache::SpeculativeLoad::didComplete):
     54        * NetworkProcess/cache/NetworkCacheSpeculativeLoadManager.cpp:
     55        (WebKit::NetworkCache::constructRevalidationRequest):
     56        (WebKit::NetworkCache::SpeculativeLoadManager::retrieveEntryFromStorage):
     57        (WebKit::NetworkCache::SpeculativeLoadManager::revalidateEntry):
     58
    1592016-03-09  Brent Fulgham  <bfulgham@apple.com>
    260

  • trunk/Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp

    r197180 r197879  
    122122    String partition;
    123123#endif
    124     if (partition.isEmpty())
    125         partition = ASCIILiteral("No partition");
    126124
    127125    // FIXME: This implements minimal Range header disk cache support. We don't parse
    … …  
    403401            break;
    404402        case UseDecision::Validate:
    405             entry->setNeedsValidation();
     403            entry->setNeedsValidation(true);
    406404            break;
    407405        default:
    … …  
    496494    WebCore::ResourceResponse response = existingEntry.response();
    497495    WebCore::updateResponseHeadersAfterRevalidation(response, validatingResponse);
    498     response.setSource(WebCore::ResourceResponse::Source::DiskCache);
    499496
    500497    auto updateEntry = std::make_unique<Entry>(existingEntry.key(), response, existingEntry.buffer(), collectVaryingRequestHeaders(originalRequest, response));

  • trunk/Source/WebKit2/NetworkProcess/cache/NetworkCacheEntry.cpp

    r194496 r197879  
    189189}
    190190
    191 void Entry::setNeedsValidation()
    192 {
    193     ASSERT(m_response.source() == WebCore::ResourceResponse::Source::DiskCache);
    194     m_response.setSource(WebCore::ResourceResponse::Source::DiskCacheAfterValidation);
     191void Entry::setNeedsValidation(bool value)
     192{
     193    m_response.setSource(value ? WebCore::ResourceResponse::Source::DiskCacheAfterValidation : WebCore::ResourceResponse::Source::DiskCache);
    195194}
    196195

  • trunk/Source/WebKit2/NetworkProcess/cache/NetworkCacheEntry.h

    r194313 r197879  
    6767
    6868    bool needsValidation() const;
    69     void setNeedsValidation();
     69    void setNeedsValidation(bool);
    7070
    7171    const Storage::Record& sourceStorageRecord() const { return m_sourceStorageRecord; }

  • trunk/Source/WebKit2/NetworkProcess/cache/NetworkCacheKey.cpp

    r192111 r197879  
    3131#include "NetworkCacheCoders.h"
    3232#include <wtf/ASCIICType.h>
     33#include <wtf/NeverDestroyed.h>
    3334#include <wtf/text/CString.h>
    3435#include <wtf/text/StringBuilder.h>
    … …  
    3637namespace WebKit {
    3738namespace NetworkCache {
     39
     40static const String& noPartitionString()
     41{
     42    static NeverDestroyed<String> noPartition(ASCIILiteral("No partition"));
     43    return noPartition;
     44}
    3845
    3946Key::Key(const Key& o)
    … …  
    4754
    4855Key::Key(const String& partition, const String& type, const String& range, const String& identifier)
    49     : m_partition(partition.isolatedCopy())
     56    : m_partition(partition.isEmpty() ? noPartitionString().isolatedCopy() : partition.isolatedCopy())
    5057    , m_type(type.isolatedCopy())
    5158    , m_identifier(identifier.isolatedCopy())
    … …  
    5865    : m_identifier(WTF::HashTableDeletedValue)
    5966{
     67}
     68
     69bool Key::hasPartition() const
     70{
     71    return m_partition != noPartitionString();
    6072}
    6173

  • trunk/Source/WebKit2/NetworkProcess/cache/NetworkCacheKey.h

    r192111 r197879  
    5555    bool isNull() const { return m_identifier.isNull(); }
    5656
     57    bool hasPartition() const;
    5758    const String& partition() const { return m_partition; }
    5859    const String& identifier() const { return m_identifier; }

  • trunk/Source/WebKit2/NetworkProcess/cache/NetworkCacheSpeculativeLoad.cpp

    r194496 r197879  
    140140    m_networkLoad = nullptr;
    141141
     142    // Make sure speculatively revalidated resources do not get validated by the NetworkResourceLoader again.
     143    if (m_cacheEntryForValidation)
     144        m_cacheEntryForValidation->setNeedsValidation(false);
     145
    142146    m_completionHandler(WTFMove(m_cacheEntryForValidation));
    143147}

  • trunk/Source/WebKit2/NetworkProcess/cache/NetworkCacheSpeculativeLoadManager.cpp

    r194496 r197879  
    8888{
    8989    ResourceRequest revalidationRequest(entry.key().identifier());
     90#if ENABLE(CACHE_PARTITIONING)
     91    if (entry.key().hasPartition())
     92        revalidationRequest.setCachePartition(entry.key().partition());
     93#endif
     94    ASSERT_WITH_MESSAGE(entry.key().range().isEmpty(), "range is not supported");
    9095
    9196    String eTag = entry.response().httpHeaderField(HTTPHeaderName::ETag);
    … …  
    345350
    346351        if (responseNeedsRevalidation(response, entry->timeStamp()))
    347             entry->setNeedsValidation();
     352            entry->setNeedsValidation(true);
    348353
    349354        completionHandler(WTFMove(entry));
    … …  
    370375
    371376    auto key = entry->key();
     377
     378    // Range is not supported.
     379    if (!key.range().isEmpty())
     380        return;
     381
    372382    LOG(NetworkCacheSpeculativePreloading, "(NetworkProcess) Speculatively revalidating '%s':", key.identifier().utf8().data());
    373383    auto revalidator = std::make_unique<SpeculativeLoad>(frameID, constructRevalidationRequest(*entry), WTFMove(entry), [this, key, frameID](std::unique_ptr<Entry> revalidatedEntry) {
    374384        ASSERT(!revalidatedEntry || !revalidatedEntry->needsValidation());
     385        ASSERT(!revalidatedEntry || revalidatedEntry->key() == key);
     386
    375387        auto protectRevalidator = m_pendingPreloads.take(key);
    376388        LOG(NetworkCacheSpeculativePreloading, "(NetworkProcess) Speculative revalidation completed for '%s':", key.identifier().utf8().data());
Note: See TracChangeset for help on using the changeset viewer.