[webkit-changes] [197879] trunk/Source/WebKit2

Wed, 09 Mar 2016 13:23:04 -0800

Title: [197879] trunk/Source/WebKit2
Revision
197879
Author
[email protected]
Date
2016-03-09 13:22:36 -0800 (Wed, 09 Mar 2016)

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

Modified Paths

Diff

Modified: trunk/Source/WebKit2/ChangeLog (197878 => 197879)


--- trunk/Source/WebKit2/ChangeLog	2016-03-09 21:15:00 UTC (rev 197878)
+++ trunk/Source/WebKit2/ChangeLog	2016-03-09 21:22:36 UTC (rev 197879)
@@ -1,3 +1,61 @@
+2016-03-09  Chris Dumez  <[email protected]>
+
+        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):
+
 2016-03-09  Brent Fulgham  <[email protected]>
 
         Local HTML should be blocked from localStorage access unless "Disable Local File Restrictions" is checked

Modified: trunk/Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp (197878 => 197879)


--- trunk/Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp	2016-03-09 21:15:00 UTC (rev 197878)
+++ trunk/Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp	2016-03-09 21:22:36 UTC (rev 197879)
@@ -121,8 +121,6 @@
 #else
     String partition;
 #endif
-    if (partition.isEmpty())
-        partition = ASCIILiteral("No partition");
 
     // FIXME: This implements minimal Range header disk cache support. We don't parse
     // ranges so only the same exact range request will be served from the cache.
@@ -402,7 +400,7 @@
         case UseDecision::Use:
             break;
         case UseDecision::Validate:
-            entry->setNeedsValidation();
+            entry->setNeedsValidation(true);
             break;
         default:
             entry = nullptr;
@@ -495,7 +493,6 @@
 
     WebCore::ResourceResponse response = existingEntry.response();
     WebCore::updateResponseHeadersAfterRevalidation(response, validatingResponse);
-    response.setSource(WebCore::ResourceResponse::Source::DiskCache);
 
     auto updateEntry = std::make_unique<Entry>(existingEntry.key(), response, existingEntry.buffer(), collectVaryingRequestHeaders(originalRequest, response));
     auto updateRecord = updateEntry->encodeAsStorageRecord();

Modified: trunk/Source/WebKit2/NetworkProcess/cache/NetworkCacheEntry.cpp (197878 => 197879)


--- trunk/Source/WebKit2/NetworkProcess/cache/NetworkCacheEntry.cpp	2016-03-09 21:15:00 UTC (rev 197878)
+++ trunk/Source/WebKit2/NetworkProcess/cache/NetworkCacheEntry.cpp	2016-03-09 21:22:36 UTC (rev 197879)
@@ -188,10 +188,9 @@
     return m_response.source() == WebCore::ResourceResponse::Source::DiskCacheAfterValidation;
 }
 
-void Entry::setNeedsValidation()
+void Entry::setNeedsValidation(bool value)
 {
-    ASSERT(m_response.source() == WebCore::ResourceResponse::Source::DiskCache);
-    m_response.setSource(WebCore::ResourceResponse::Source::DiskCacheAfterValidation);
+    m_response.setSource(value ? WebCore::ResourceResponse::Source::DiskCacheAfterValidation : WebCore::ResourceResponse::Source::DiskCache);
 }
 
 void Entry::asJSON(StringBuilder& json, const Storage::RecordInfo& info) const

Modified: trunk/Source/WebKit2/NetworkProcess/cache/NetworkCacheEntry.h (197878 => 197879)


--- trunk/Source/WebKit2/NetworkProcess/cache/NetworkCacheEntry.h	2016-03-09 21:15:00 UTC (rev 197878)
+++ trunk/Source/WebKit2/NetworkProcess/cache/NetworkCacheEntry.h	2016-03-09 21:22:36 UTC (rev 197879)
@@ -66,7 +66,7 @@
 #endif
 
     bool needsValidation() const;
-    void setNeedsValidation();
+    void setNeedsValidation(bool);
 
     const Storage::Record& sourceStorageRecord() const { return m_sourceStorageRecord; }

Modified: trunk/Source/WebKit2/NetworkProcess/cache/NetworkCacheKey.cpp (197878 => 197879)


--- trunk/Source/WebKit2/NetworkProcess/cache/NetworkCacheKey.cpp	2016-03-09 21:15:00 UTC (rev 197878)
+++ trunk/Source/WebKit2/NetworkProcess/cache/NetworkCacheKey.cpp	2016-03-09 21:22:36 UTC (rev 197879)
@@ -30,12 +30,19 @@
 
 #include "NetworkCacheCoders.h"
 #include <wtf/ASCIICType.h>
+#include <wtf/NeverDestroyed.h>
 #include <wtf/text/CString.h>
 #include <wtf/text/StringBuilder.h>
 
 namespace WebKit {
 namespace NetworkCache {
 
+static const String& noPartitionString()
+{
+    static NeverDestroyed<String> noPartition(ASCIILiteral("No partition"));
+    return noPartition;
+}
+
 Key::Key(const Key& o)
     : m_partition(o.m_partition.isolatedCopy())
     , m_type(o.m_type.isolatedCopy())
@@ -46,7 +53,7 @@
 }
 
 Key::Key(const String& partition, const String& type, const String& range, const String& identifier)
-    : m_partition(partition.isolatedCopy())
+    : m_partition(partition.isEmpty() ? noPartitionString().isolatedCopy() : partition.isolatedCopy())
     , m_type(type.isolatedCopy())
     , m_identifier(identifier.isolatedCopy())
     , m_range(range.isolatedCopy())
@@ -59,6 +66,11 @@
 {
 }
 
+bool Key::hasPartition() const
+{
+    return m_partition != noPartitionString();
+}
+
 Key& Key::operator=(const Key& other)
 {
     m_partition = other.m_partition.isolatedCopy();

Modified: trunk/Source/WebKit2/NetworkProcess/cache/NetworkCacheKey.h (197878 => 197879)


--- trunk/Source/WebKit2/NetworkProcess/cache/NetworkCacheKey.h	2016-03-09 21:15:00 UTC (rev 197878)
+++ trunk/Source/WebKit2/NetworkProcess/cache/NetworkCacheKey.h	2016-03-09 21:22:36 UTC (rev 197879)
@@ -54,6 +54,7 @@
 
     bool isNull() const { return m_identifier.isNull(); }
 
+    bool hasPartition() const;
     const String& partition() const { return m_partition; }
     const String& identifier() const { return m_identifier; }
     const String& type() const { return m_type; }

Modified: trunk/Source/WebKit2/NetworkProcess/cache/NetworkCacheSpeculativeLoad.cpp (197878 => 197879)


--- trunk/Source/WebKit2/NetworkProcess/cache/NetworkCacheSpeculativeLoad.cpp	2016-03-09 21:15:00 UTC (rev 197878)
+++ trunk/Source/WebKit2/NetworkProcess/cache/NetworkCacheSpeculativeLoad.cpp	2016-03-09 21:22:36 UTC (rev 197879)
@@ -139,6 +139,10 @@
 
     m_networkLoad = nullptr;
 
+    // Make sure speculatively revalidated resources do not get validated by the NetworkResourceLoader again.
+    if (m_cacheEntryForValidation)
+        m_cacheEntryForValidation->setNeedsValidation(false);
+
     m_completionHandler(WTFMove(m_cacheEntryForValidation));
 }

Modified: trunk/Source/WebKit2/NetworkProcess/cache/NetworkCacheSpeculativeLoadManager.cpp (197878 => 197879)


--- trunk/Source/WebKit2/NetworkProcess/cache/NetworkCacheSpeculativeLoadManager.cpp	2016-03-09 21:15:00 UTC (rev 197878)
+++ trunk/Source/WebKit2/NetworkProcess/cache/NetworkCacheSpeculativeLoadManager.cpp	2016-03-09 21:22:36 UTC (rev 197879)
@@ -87,6 +87,11 @@
 static inline ResourceRequest constructRevalidationRequest(const Entry& entry)
 {
     ResourceRequest revalidationRequest(entry.key().identifier());
+#if ENABLE(CACHE_PARTITIONING)
+    if (entry.key().hasPartition())
+        revalidationRequest.setCachePartition(entry.key().partition());
+#endif
+    ASSERT_WITH_MESSAGE(entry.key().range().isEmpty(), "range is not supported");
 
     String eTag = entry.response().httpHeaderField(HTTPHeaderName::ETag);
     if (!eTag.isEmpty())
@@ -344,7 +349,7 @@
         }
 
         if (responseNeedsRevalidation(response, entry->timeStamp()))
-            entry->setNeedsValidation();
+            entry->setNeedsValidation(true);
 
         completionHandler(WTFMove(entry));
         return true;
@@ -369,9 +374,16 @@
     ASSERT(entry->needsValidation());
 
     auto key = entry->key();
+
+    // Range is not supported.
+    if (!key.range().isEmpty())
+        return;
+
     LOG(NetworkCacheSpeculativePreloading, "(NetworkProcess) Speculatively revalidating '%s':", key.identifier().utf8().data());
     auto revalidator = std::make_unique<SpeculativeLoad>(frameID, constructRevalidationRequest(*entry), WTFMove(entry), [this, key, frameID](std::unique_ptr<Entry> revalidatedEntry) {
         ASSERT(!revalidatedEntry || !revalidatedEntry->needsValidation());
+        ASSERT(!revalidatedEntry || revalidatedEntry->key() == key);
+
         auto protectRevalidator = m_pendingPreloads.take(key);
         LOG(NetworkCacheSpeculativePreloading, "(NetworkProcess) Speculative revalidation completed for '%s':", key.identifier().utf8().data());
 

_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to