Bug 1398847 - Enabling RCWN causes tp6_facebook regression, r=valentin
authorMichal Novotny <michal.novotny@gmail.com>
Mon, 11 Sep 2017 22:16:13 +0200
changeset 380215 586734464b215638dacfcb66a2592d06500444d8
parent 380214 33a2372e73997ce75a3d67c2a208e599295828f8
child 380216 76192a7c0257b1f0ee6b46a3b6ec8d29b418d0ef
push id32482
push userarchaeopteryx@coole-files.de
push dateTue, 12 Sep 2017 09:35:40 +0000
treeherdermozilla-central@b0e945eed81d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersvalentin
bugs1398847
milestone57.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1398847 - Enabling RCWN causes tp6_facebook regression, r=valentin For some reason, triggering network directly from MaybeRaceCacheWithNetwork() causes performance regression of tp6_facebook tests. This patch changes it so that an event is posted instead. The patch also adds network.http.rcwn.min_wait_before_racing_ms preference which can be used by users to avoid immediate racing.
modules/libpref/init/all.js
netwerk/protocol/http/nsHttpChannel.cpp
netwerk/protocol/http/nsHttpChannel.h
--- a/modules/libpref/init/all.js
+++ b/modules/libpref/init/all.js
@@ -1830,16 +1830,17 @@ pref("network.http.max_response_header_s
 // If we should attempt to race the cache and network
 pref("network.http.rcwn.enabled", false);
 pref("network.http.rcwn.cache_queue_normal_threshold", 8);
 pref("network.http.rcwn.cache_queue_priority_threshold", 2);
 // We might attempt to race the cache with the network only if a resource
 // is smaller than this size.
 pref("network.http.rcwn.small_resource_size_kb", 256);
 
+pref("network.http.rcwn.min_wait_before_racing_ms", 0);
 pref("network.http.rcwn.max_wait_before_racing_ms", 500);
 
 // The ratio of the transaction count for the focused window and the count of
 // all available active connections.
 pref("network.http.focused_window_transaction_ratio", "0.9");
 
 // Whether or not we give more priority to active tab.
 // Note that this requires restart for changes to take effect.
--- a/netwerk/protocol/http/nsHttpChannel.cpp
+++ b/netwerk/protocol/http/nsHttpChannel.cpp
@@ -123,16 +123,17 @@ namespace {
 
 // Monotonically increasing ID for generating unique cache entries per
 // intercepted channel.
 static uint64_t gNumIntercepted = 0;
 static bool sRCWNEnabled = false;
 static uint32_t sRCWNQueueSizeNormal = 50;
 static uint32_t sRCWNQueueSizePriority = 10;
 static uint32_t sRCWNSmallResourceSizeKB = 256;
+static uint32_t sRCWNMinWaitMs = 0;
 static uint32_t sRCWNMaxWaitMs = 500;
 
 // True if the local cache should be bypassed when processing a request.
 #define BYPASS_LOCAL_CACHE(loadFlags) \
         (loadFlags & (nsIRequest::LOAD_BYPASS_CACHE | \
                       nsICachingChannel::LOAD_BYPASS_LOCAL_CACHE))
 
 #define RECOVER_FROM_CACHE_FILE_ERROR(result) \
@@ -620,17 +621,17 @@ nsHttpChannel::ConnectOnTailUnblock()
     // When racing, if OnCacheEntryAvailable is called before AsyncOpenURI
     // returns, then we may not have started reading from the cache.
     // If the content is valid, we should attempt to do so, as technically the
     // cache has won the race.
     if (mRaceCacheWithNetwork && mCachedContentIsValid) {
         Unused << ReadFromCache(true);
     }
 
-    return TriggerNetwork(0);
+    return TriggerNetwork();
 }
 
 nsresult
 nsHttpChannel::TryHSTSPriming()
 {
     bool isHttpScheme;
     nsresult rv = mURI->SchemeIs("http", &isHttpScheme);
     NS_ENSURE_SUCCESS(rv, rv);
@@ -4508,17 +4509,17 @@ nsHttpChannel::OnCacheEntryAvailableInte
         // request was already sent (see bug 1377223).
         AccumulateCategorical(Telemetry::LABELS_NETWORK_RACE_CACHE_VALIDATION::NotSent);
     }
 
     if (mRaceCacheWithNetwork && mCachedContentIsValid) {
         Unused << ReadFromCache(true);
     }
 
-    return TriggerNetwork(0);
+    return TriggerNetwork();
 }
 
 nsresult
 nsHttpChannel::OnNormalCacheEntryAvailable(nsICacheEntry *aEntry,
                                            bool aNew,
                                            nsresult aEntryStatus)
 {
     mCacheEntriesToWaitFor &= ~WAIT_FOR_CACHE_ENTRY;
@@ -6129,16 +6130,17 @@ nsHttpChannel::AsyncOpen(nsIStreamListen
 
     static bool sRCWNInited = false;
     if (!sRCWNInited) {
         sRCWNInited = true;
         Preferences::AddBoolVarCache(&sRCWNEnabled, "network.http.rcwn.enabled");
         Preferences::AddUintVarCache(&sRCWNQueueSizeNormal, "network.http.rcwn.cache_queue_normal_threshold");
         Preferences::AddUintVarCache(&sRCWNQueueSizePriority, "network.http.rcwn.cache_queue_priority_threshold");
         Preferences::AddUintVarCache(&sRCWNSmallResourceSizeKB, "network.http.rcwn.small_resource_size_kb");
+        Preferences::AddUintVarCache(&sRCWNMinWaitMs, "network.http.rcwn.min_wait_before_racing_ms");
         Preferences::AddUintVarCache(&sRCWNMaxWaitMs, "network.http.rcwn.max_wait_before_racing_ms");
     }
 
     rv = NS_CheckPortSafety(mURI);
     if (NS_FAILED(rv)) {
         ReleaseListeners();
         return rv;
     }
@@ -9297,17 +9299,53 @@ nsHttpChannel::Test_triggerDelayedOpenCa
     std::function<void(nsHttpChannel*)> cacheOpenFunc = nullptr;
     std::swap(cacheOpenFunc, mCacheOpenFunc);
     cacheOpenFunc(this);
 
     return NS_OK;
 }
 
 nsresult
-nsHttpChannel::TriggerNetwork(int32_t aTimeout)
+nsHttpChannel::TriggerNetworkWithDelay(uint32_t aDelay)
+{
+    MOZ_ASSERT(NS_IsMainThread(), "Must be called on the main thread");
+
+    LOG(("nsHttpChannel::TriggerNetworkWithDelay [this=%p, delay=%u]\n",
+         this, aDelay));
+
+    if (mCanceled) {
+        LOG(("  channel was canceled.\n"));
+        return mStatus;
+    }
+
+    // If a network request has already gone out, there is no point in
+    // doing this again.
+    if (mNetworkTriggered) {
+        LOG(("  network already triggered. Returning.\n"));
+        return NS_OK;
+    }
+
+    if (!aDelay) {
+        // We cannot call TriggerNetwork() directly here, because it would
+        // cause performance regression in tp6 tests, see bug 1398847.
+        return NS_DispatchToMainThread(
+            NewRunnableMethod("net::nsHttpChannel::TriggerNetworkWithDelay",
+                              this, &nsHttpChannel::TriggerNetwork),
+            NS_DISPATCH_NORMAL);
+    }
+
+    if (!mNetworkTriggerTimer) {
+        mNetworkTriggerTimer = do_CreateInstance(NS_TIMER_CONTRACTID);
+    }
+    mNetworkTriggerTimer->InitWithCallback(this, aDelay, nsITimer::TYPE_ONE_SHOT);
+    return NS_OK;
+}
+
+nsresult
+nsHttpChannel::TriggerNetwork()
 {
     MOZ_ASSERT(NS_IsMainThread(), "Must be called on the main thread");
 
     LOG(("nsHttpChannel::TriggerNetwork [this=%p]\n", this));
 
     if (mCanceled) {
         LOG(("  channel was canceled.\n"));
         return mStatus;
@@ -9315,46 +9353,39 @@ nsHttpChannel::TriggerNetwork(int32_t aT
 
     // If a network request has already gone out, there is no point in
     // doing this again.
     if (mNetworkTriggered) {
         LOG(("  network already triggered. Returning.\n"));
         return NS_OK;
     }
 
-    if (!aTimeout) {
-        mNetworkTriggered = true;
-        if (mNetworkTriggerTimer) {
-            mNetworkTriggerTimer->Cancel();
-            mNetworkTriggerTimer = nullptr;
-        }
-
-        // If we are waiting for a proxy request, that means we can't trigger
-        // the next step just yet. We need for mConnectionInfo to be non-null
-        // before we call TryHSTSPriming. OnProxyAvailable will trigger
-        // BeginConnect, and Connect will call TryHSTSPriming even if it's
-        // for the cache callbacks.
-        if (mProxyRequest) {
-            LOG(("  proxy request in progress. Delaying network trigger.\n"));
-            mWaitingForProxy = true;
-            return NS_OK;
-        }
-
-        if (mCacheAsyncOpenCalled && !mOnCacheAvailableCalled) {
-            mRaceCacheWithNetwork = true;
-        }
-
-        LOG(("  triggering network\n"));
-        return TryHSTSPriming();
-    }
-
-    LOG(("  setting timer to trigger network: %d ms\n", aTimeout));
-    mNetworkTriggerTimer = do_CreateInstance(NS_TIMER_CONTRACTID);
-    mNetworkTriggerTimer->InitWithCallback(this, aTimeout, nsITimer::TYPE_ONE_SHOT);
-    return NS_OK;
+    mNetworkTriggered = true;
+    if (mNetworkTriggerTimer) {
+        mNetworkTriggerTimer->Cancel();
+        mNetworkTriggerTimer = nullptr;
+    }
+
+    // If we are waiting for a proxy request, that means we can't trigger
+    // the next step just yet. We need for mConnectionInfo to be non-null
+    // before we call TryHSTSPriming. OnProxyAvailable will trigger
+    // BeginConnect, and Connect will call TryHSTSPriming even if it's
+    // for the cache callbacks.
+    if (mProxyRequest) {
+        LOG(("  proxy request in progress. Delaying network trigger.\n"));
+        mWaitingForProxy = true;
+        return NS_OK;
+    }
+
+    if (mCacheAsyncOpenCalled && !mOnCacheAvailableCalled) {
+        mRaceCacheWithNetwork = true;
+    }
+
+    LOG(("  triggering network\n"));
+    return TryHSTSPriming();
 }
 
 nsresult
 nsHttpChannel::MaybeRaceCacheWithNetwork()
 {
     // Don't trigger the network if the load flags say so.
     if (mLoadFlags & (LOAD_ONLY_FROM_CACHE | LOAD_NO_NETWORK_IO)) {
         return NS_OK;
@@ -9375,43 +9406,42 @@ nsHttpChannel::MaybeRaceCacheWithNetwork
         mRaceDelay = 0;
     } else {
         // Give cache a headstart of 3 times the average cache entry open time.
         mRaceDelay = CacheFileUtils::CachePerfStats::GetAverage(
                      CacheFileUtils::CachePerfStats::ENTRY_OPEN, true) * 3;
         // We use microseconds in CachePerfStats but we need milliseconds
         // for TriggerNetwork.
         mRaceDelay /= 1000;
-        if (mRaceDelay > sRCWNMaxWaitMs) {
-            mRaceDelay = sRCWNMaxWaitMs;
-        }
-    }
-
-    MOZ_ASSERT(sRCWNEnabled, "The pref must be truned on.");
+    }
+
+    mRaceDelay = clamped<uint32_t>(mRaceDelay, sRCWNMinWaitMs, sRCWNMaxWaitMs);
+
+    MOZ_ASSERT(sRCWNEnabled, "The pref must be turned on.");
     LOG(("nsHttpChannel::MaybeRaceCacheWithNetwork [this=%p, delay=%u]\n",
          this, mRaceDelay));
 
-    return TriggerNetwork(mRaceDelay);
+    return TriggerNetworkWithDelay(mRaceDelay);
 }
 
 NS_IMETHODIMP
 nsHttpChannel::Test_triggerNetwork(int32_t aTimeout)
 {
     MOZ_ASSERT(NS_IsMainThread(), "Must be called on the main thread");
-    return TriggerNetwork(aTimeout);
+    return TriggerNetworkWithDelay(aTimeout);
 }
 
 NS_IMETHODIMP
 nsHttpChannel::Notify(nsITimer *aTimer)
 {
     RefPtr<nsHttpChannel> self(this);
     if (aTimer == mCacheOpenTimer) {
         return Test_triggerDelayedOpenCacheEntry();
     } else if (aTimer == mNetworkTriggerTimer) {
-        return TriggerNetwork(0);
+        return TriggerNetwork();
     } else {
         MOZ_CRASH("Unknown timer");
     }
 
     return NS_OK;
 }
 
 bool
--- a/netwerk/protocol/http/nsHttpChannel.h
+++ b/netwerk/protocol/http/nsHttpChannel.h
@@ -717,17 +717,18 @@ private:
         RESPONSE_FROM_NETWORK = 2   // response coming from the network
     };
     Atomic<ResponseSource, Relaxed> mFirstResponseSource;
 
     // Determines if it's possible and advisable to race the network request
     // with the cache fetch, and proceeds to do so.
     nsresult MaybeRaceCacheWithNetwork();
 
-    nsresult TriggerNetwork(int32_t aTimeout);
+    nsresult TriggerNetworkWithDelay(uint32_t aDelay);
+    nsresult TriggerNetwork();
     void CancelNetworkRequest(nsresult aStatus);
     // Timer used to delay the network request, or to trigger the network
     // request if retrieving the cache entry takes too long.
     nsCOMPtr<nsITimer> mNetworkTriggerTimer;
     // Is true if the network request has been triggered.
     bool mNetworkTriggered = false;
     bool mWaitingForProxy = false;
     // Is true if the onCacheEntryAvailable callback has been called.