Reduce the number of UpdateCurrentNetworkState calls during startup
UpdateCurrentNetworkState is called 3 times during the initialization
phase of Cronet, those are redundant calls as we don't expect the state
of the network to change during startup within ~10ms and if it did
change then there would be callbacks that should notify us to update
our network state.
Reducing initialization time should allow us to process the first
request at an earlier time which allows the app to load content in a
timely fashion.
Removed base::featureFlag StoreConnectionSubtype as it has been enabled
for more than half a year (more than a single major version)
Tested: Enabled the flag and launched Cronet sample app, the perfetto
trace showed a single instance of UpdateCurrentNetworkState
Bug: 376646498
Change-Id: Ibeac34b83636ab6573e3c82bafa7d79286697961
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6111162
Reviewed-by: Stefano Duo <stefanoduo@google.com>
Auto-Submit: Mohannad Farrag <aymanm@google.com>
Commit-Queue: Mohannad Farrag <aymanm@google.com>
Cr-Commit-Position: refs/heads/main@{#1404239}
diff --git a/components/cronet/android/cronet_library_loader.cc b/components/cronet/android/cronet_library_loader.cc
index 85c9bc0..d446fd5 100644
--- a/components/cronet/android/cronet_library_loader.cc
+++ b/components/cronet/android/cronet_library_loader.cc
@@ -2,7 +2,10 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
+#include "components/cronet/android/cronet_library_loader.h"
+
#include <jni.h>
+
#include <memory>
#include <string>
#include <utility>
@@ -28,10 +31,10 @@
#include "build/build_config.h"
#include "components/cronet/android/cronet_base_feature.h"
#include "components/cronet/android/cronet_jni_registration_generated.h"
-#include "components/cronet/android/cronet_library_loader.h"
#include "components/cronet/android/proto/base_feature_overrides.pb.h"
#include "components/cronet/cronet_global_state.h"
#include "components/cronet/version.h"
+#include "net/android/network_change_notifier_delegate_android.h"
#include "net/android/network_change_notifier_factory_android.h"
#include "net/base/network_change_notifier.h"
#include "net/proxy_resolution/configured_proxy_resolution_service.h"
@@ -136,7 +139,9 @@
base::android::LibraryLoaderExitHook();
}
-void JNI_CronetLibraryLoader_CronetInitOnInitThread(JNIEnv* env) {
+void JNI_CronetLibraryLoader_CronetInitOnInitThread(
+ JNIEnv* env,
+ jboolean updateNetworkStateFromNative) {
// Initialize SingleThreadTaskExecutor for init thread.
DCHECK(!base::CurrentThread::IsSet());
DCHECK(!g_init_task_executor);
@@ -144,9 +149,15 @@
new base::SingleThreadTaskExecutor(base::MessagePumpType::JAVA);
DCHECK(!g_network_change_notifier);
+
if (!net::NetworkChangeNotifier::GetFactory()) {
net::NetworkChangeNotifier::SetFactory(
- new net::NetworkChangeNotifierFactoryAndroid());
+ new net::NetworkChangeNotifierFactoryAndroid(
+ updateNetworkStateFromNative
+ ? net::NetworkChangeNotifierDelegateAndroid::
+ ForceUpdateNetworkState::kEnabled
+ : net::NetworkChangeNotifierDelegateAndroid::
+ ForceUpdateNetworkState::kDisabled));
}
g_network_change_notifier = net::NetworkChangeNotifier::CreateIfNeeded();
DCHECK(g_network_change_notifier);
diff --git a/components/cronet/android/java/src/org/chromium/net/impl/CronetLibraryLoader.java b/components/cronet/android/java/src/org/chromium/net/impl/CronetLibraryLoader.java
index 3cfa2ab8..3fb63fa 100644
--- a/components/cronet/android/java/src/org/chromium/net/impl/CronetLibraryLoader.java
+++ b/components/cronet/android/java/src/org/chromium/net/impl/CronetLibraryLoader.java
@@ -23,6 +23,7 @@
import org.chromium.base.Log;
import org.chromium.base.metrics.ScopedSysTraceEvent;
import org.chromium.net.NetworkChangeNotifier;
+import org.chromium.net.RegistrationPolicyAlwaysRegister;
import org.chromium.net.httpflags.BaseFeature;
import org.chromium.net.httpflags.Flags;
import org.chromium.net.httpflags.HttpFlagsLoader;
@@ -56,6 +57,11 @@
private static final ConditionVariable sWaitForLibLoad = new ConditionVariable();
private static final ConditionVariable sHttpFlagsLoaded = new ConditionVariable();
+
+ @VisibleForTesting
+ public static final String UPDATE_NETWORK_STATE_ONCE_ON_STARTUP_FLAG_NAME =
+ "Cronet_UpdateNetworkStateOnlyOnceOnStartup";
+
private static ResolvedFlags sHttpFlags;
/**
@@ -226,7 +232,31 @@
// embedders do not have API access to create network change
// observers. Existing observers in the net stack do not
// perform expensive work.
- NetworkChangeNotifier.registerToReceiveNotificationsAlways();
+ //
+ // During the setup of connectivity state autodetection, the network state is updated
+ // multiple times:
+ // 1. Within Java NetworkChangeNotifierAutoDetect's constructor
+ // 2. Within Java NetworkChangeNotifier#setAutoDetectConnectivityStateInternal, after
+ // creating a NetworkChangeNotifierAutoDetect (effectively, just after 1)
+ // 3. Within C++ NetworkChangeNotifierDelegateAndroid's constructor
+ //
+ // 2 should never be needed, as 1 always runs before and takes care of updating the
+ // network state. Having said that, it will be kept to keep track of the performance
+ // improvement from this change. Once the experiment terminates, we will delete it, this
+ // should always be safe for Chrome, Cronet and Webview.
+ //
+ // As per 3, Cronet always initializes NetworkChangeNotifier first from Java (going
+ // through 1 and 2), then from C++ (going through 3).
+ // Since we would like to query the network state only once, this experiment
+ // disables 2 and 3.
+ var updateNetworkStateOnceFlagValue =
+ getHttpFlags().flags().get(UPDATE_NETWORK_STATE_ONCE_ON_STARTUP_FLAG_NAME);
+ var updateNetworkStateOnce =
+ updateNetworkStateOnceFlagValue != null
+ && updateNetworkStateOnceFlagValue.getBoolValue();
+ NetworkChangeNotifier.setAutoDetectConnectivityState(
+ new RegistrationPolicyAlwaysRegister(),
+ /* forceUpdateNetworkState= */ !updateNetworkStateOnce);
try (var libLoadTraceEvent =
ScopedSysTraceEvent.scoped(
@@ -243,7 +273,7 @@
// NetworkChangeNotifierAndroid is created, so as to avoid receiving
// the undesired initial network change observer notification, which
// will cause active requests to fail with ERR_NETWORK_CHANGED.
- CronetLibraryLoaderJni.get().cronetInitOnInitThread();
+ CronetLibraryLoaderJni.get().cronetInitOnInitThread(!updateNetworkStateOnce);
}
}
}
@@ -400,7 +430,7 @@
// Native methods are implemented in cronet_library_loader.cc.
void nativeInit();
- void cronetInitOnInitThread();
+ void cronetInitOnInitThread(boolean updateNetworkStateFromNative);
String getCronetVersion();
diff --git a/components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java b/components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java
index 94b1eba..8cfad7c4 100644
--- a/components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java
+++ b/components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java
@@ -31,7 +31,9 @@
import org.chromium.base.Log;
import org.chromium.base.test.util.DoNotBatch;
+import org.chromium.net.CronetTestRule.BoolFlag;
import org.chromium.net.CronetTestRule.CronetImplementation;
+import org.chromium.net.CronetTestRule.Flags;
import org.chromium.net.CronetTestRule.IgnoreFor;
import org.chromium.net.CronetTestRule.RequiresMinAndroidApi;
import org.chromium.net.CronetTestRule.RequiresMinApi;
@@ -40,6 +42,7 @@
import org.chromium.net.TestUrlRequestCallback.ResponseStep;
import org.chromium.net.apihelpers.UploadDataProviders;
import org.chromium.net.impl.CronetExceptionImpl;
+import org.chromium.net.impl.CronetLibraryLoader;
import org.chromium.net.impl.CronetUrlRequest;
import org.chromium.net.impl.NetworkExceptionImpl;
import org.chromium.net.impl.UrlResponseInfoImpl;
@@ -179,6 +182,18 @@
@Test
@SmallTest
+ @Flags(
+ boolFlags = {
+ @BoolFlag(
+ name = CronetLibraryLoader.UPDATE_NETWORK_STATE_ONCE_ON_STARTUP_FLAG_NAME,
+ value = true)
+ })
+ public void testSimpleGetWithReducedNetworkChangeNotifierExperiment() throws Exception {
+ testSimpleGet();
+ }
+
+ @Test
+ @SmallTest
public void testSimpleGet() throws Exception {
String url = NativeTestServer.getEchoMethodURL();
TestUrlRequestCallback callback = startAndWaitForComplete(url);
diff --git a/net/android/java/src/org/chromium/net/NetworkChangeNotifier.java b/net/android/java/src/org/chromium/net/NetworkChangeNotifier.java
index 1ed9aa0..38823b53 100644
--- a/net/android/java/src/org/chromium/net/NetworkChangeNotifier.java
+++ b/net/android/java/src/org/chromium/net/NetworkChangeNotifier.java
@@ -85,12 +85,14 @@
}
@CalledByNative
- public int getCurrentConnectionSubtype() {
+ public int getCurrentConnectionSubtype(boolean forceUpdateNetworkState) {
try (ScopedSysTraceEvent event =
ScopedSysTraceEvent.scoped("NetworkChangeNotifier.getCurrentConnectionSubtype")) {
if (mAutoDetector == null) return ConnectionSubtype.SUBTYPE_UNKNOWN;
- mAutoDetector.updateCurrentNetworkState();
+ if (forceUpdateNetworkState) {
+ mAutoDetector.updateCurrentNetworkState();
+ }
return mAutoDetector.getCurrentNetworkState().getConnectionSubtype();
}
}
@@ -261,6 +263,8 @@
}
},
policy);
+ // TODO(crbug.com/376646498): Remove this once we have finished
+ // the experiment as its definitely a redundant call.
if (forceUpdateNetworkState) mAutoDetector.updateCurrentNetworkState();
final NetworkChangeNotifierAutoDetect.NetworkState networkState =
diff --git a/net/android/network_change_notifier_delegate_android.cc b/net/android/network_change_notifier_delegate_android.cc
index 16141d6b..75a9dcc4 100644
--- a/net/android/network_change_notifier_delegate_android.cc
+++ b/net/android/network_change_notifier_delegate_android.cc
@@ -86,6 +86,11 @@
}
NetworkChangeNotifierDelegateAndroid::NetworkChangeNotifierDelegateAndroid()
+ : NetworkChangeNotifierDelegateAndroid(ForceUpdateNetworkState::kEnabled) {}
+
+NetworkChangeNotifierDelegateAndroid::NetworkChangeNotifierDelegateAndroid(
+ net::NetworkChangeNotifierDelegateAndroid::ForceUpdateNetworkState
+ force_update_network_state)
: java_network_change_notifier_(Java_NetworkChangeNotifier_init(
base::android::AttachCurrentThread())),
register_network_callback_failed_(
@@ -103,7 +108,8 @@
env, java_network_change_notifier_)));
auto connection_subtype = ConvertConnectionSubtype(
Java_NetworkChangeNotifier_getCurrentConnectionSubtype(
- env, java_network_change_notifier_));
+ env, java_network_change_notifier_,
+ force_update_network_state == ForceUpdateNetworkState::kEnabled));
SetCurrentConnectionSubtype(connection_subtype);
SetCurrentMaxBandwidth(
NetworkChangeNotifierAndroid::GetMaxBandwidthMbpsForConnectionSubtype(
@@ -146,14 +152,8 @@
NetworkChangeNotifier::ConnectionSubtype
NetworkChangeNotifierDelegateAndroid::GetCurrentConnectionSubtype() const {
- if (base::FeatureList::IsEnabled(net::features::kStoreConnectionSubtype)) {
base::AutoLock auto_lock(connection_lock_);
return connection_subtype_;
- }
- DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
- return ConvertConnectionSubtype(
- Java_NetworkChangeNotifier_getCurrentConnectionSubtype(
- base::android::AttachCurrentThread(), java_network_change_notifier_));
}
void NetworkChangeNotifierDelegateAndroid::
diff --git a/net/android/network_change_notifier_delegate_android.h b/net/android/network_change_notifier_delegate_android.h
index 12b972e..cb10007 100644
--- a/net/android/network_change_notifier_delegate_android.h
+++ b/net/android/network_change_notifier_delegate_android.h
@@ -31,6 +31,10 @@
typedef NetworkChangeNotifier::ConnectionSubtype ConnectionSubtype;
typedef NetworkChangeNotifier::NetworkList NetworkList;
+ enum class ForceUpdateNetworkState {
+ kEnabled,
+ kDisabled,
+ };
// Observer interface implemented by NetworkChangeNotifierAndroid which
// subscribes to network change notifications fired by the delegate (and
// initiated by the Java side).
@@ -61,6 +65,9 @@
// // Creates Java NetworkChangeNotifierAutoDetect class instance.
// NetworkChangeNotifier.registerToReceiveNotificationsAlways();
NetworkChangeNotifierDelegateAndroid();
+ explicit NetworkChangeNotifierDelegateAndroid(
+ net::NetworkChangeNotifierDelegateAndroid::ForceUpdateNetworkState
+ force_update_network_state);
NetworkChangeNotifierDelegateAndroid(
const NetworkChangeNotifierDelegateAndroid&) = delete;
NetworkChangeNotifierDelegateAndroid& operator=(
diff --git a/net/android/network_change_notifier_factory_android.cc b/net/android/network_change_notifier_factory_android.cc
index fe23979..df6d6feb 100644
--- a/net/android/network_change_notifier_factory_android.cc
+++ b/net/android/network_change_notifier_factory_android.cc
@@ -6,11 +6,19 @@
#include "base/memory/ptr_util.h"
#include "net/android/network_change_notifier_android.h"
+#include "net/android/network_change_notifier_delegate_android.h"
namespace net {
-NetworkChangeNotifierFactoryAndroid::NetworkChangeNotifierFactoryAndroid() =
- default;
+NetworkChangeNotifierFactoryAndroid::NetworkChangeNotifierFactoryAndroid()
+ : NetworkChangeNotifierFactoryAndroid(
+ NetworkChangeNotifierDelegateAndroid::ForceUpdateNetworkState::
+ kEnabled) {}
+
+NetworkChangeNotifierFactoryAndroid::NetworkChangeNotifierFactoryAndroid(
+ NetworkChangeNotifierDelegateAndroid::ForceUpdateNetworkState
+ force_update_network_state)
+ : delegate_(force_update_network_state) {}
NetworkChangeNotifierFactoryAndroid::~NetworkChangeNotifierFactoryAndroid() =
default;
diff --git a/net/android/network_change_notifier_factory_android.h b/net/android/network_change_notifier_factory_android.h
index c0ebaa70..415c19d 100644
--- a/net/android/network_change_notifier_factory_android.h
+++ b/net/android/network_change_notifier_factory_android.h
@@ -26,6 +26,11 @@
// Must be called on the JNI thread.
NetworkChangeNotifierFactoryAndroid();
+ // Must be called on the JNI thread.
+ explicit NetworkChangeNotifierFactoryAndroid(
+ NetworkChangeNotifierDelegateAndroid::ForceUpdateNetworkState
+ force_update_network_state);
+
NetworkChangeNotifierFactoryAndroid(
const NetworkChangeNotifierFactoryAndroid&) = delete;
NetworkChangeNotifierFactoryAndroid& operator=(
diff --git a/net/base/features.cc b/net/base/features.cc
index e777c22..b33e5a4 100644
--- a/net/base/features.cc
+++ b/net/base/features.cc
@@ -561,10 +561,6 @@
"PersistDeviceBoundSessions",
base::FEATURE_DISABLED_BY_DEFAULT);
-BASE_FEATURE(kStoreConnectionSubtype,
- "StoreConnectionSubtype",
- base::FEATURE_ENABLED_BY_DEFAULT);
-
BASE_FEATURE(kPartitionProxyChains,
"PartitionProxyChains",
base::FEATURE_ENABLED_BY_DEFAULT);
diff --git a/net/base/features.h b/net/base/features.h
index 19c1a93..fedfc933 100644
--- a/net/base/features.h
+++ b/net/base/features.h
@@ -594,10 +594,6 @@
// enabled.
NET_EXPORT BASE_DECLARE_FEATURE(kPersistDeviceBoundSessions);
-// Enables storing connection subtype in NetworkChangeNotifierDelegateAndroid to
-// save the cost of the JNI call for future access.
-NET_EXPORT BASE_DECLARE_FEATURE(kStoreConnectionSubtype);
-
// When enabled, all proxies in a proxy chain are partitioned by the NAK for the
// endpoint of the connection. When disabled, proxies carrying tunnels to other
// proxies (i.e., all proxies but the last one in the ProxyChain) are not
