From 47c0d852da282cbb8c326e3985e17dc0d41fd528 Mon Sep 17 00:00:00 2001 From: Tobias Thierer Date: Wed, 31 Oct 2018 16:55:08 +0000 Subject: [PATCH] Refactor Network's use of mUrlConnectionFactory. 1.) Replace maybeInitUrlConnectionFactory() with a static method createUrlConnectionFactory(Dns) and move the logic to acquire the lock and initialize mUrlConnectionFactory out into openConnection. This makes it a bit clearer that the lock is acquired during openConnection(). 2.) Use @GuardedBy("mLock") rather than a human readable comment on mUrlConnectionFactory. 3.) Make mUrlConnectionFactory non-volatile (since it's guarded by mLock), as recommended by Narayan on the review thread for http://r.android.com/370652 Alternatively, the field could have remained volatile and we could have used double-checked locking to avoid acquiring the lock in the common case. The lock is only acquired during getSocketFactory() and openConnection(), so it shouldn't usually be contended. This CL is a pure refactoring that shouldn't have any observable behavior change. Bug: 38311512 Test: Treehugger Exempt-From-Owner-Approval: refactoring only, owner didn't respond in time Change-Id: I1cf6075dc7cd994657b11d6a82de3ec63235fb1e --- core/java/android/net/Network.java | 73 ++++++++++++++++-------------- 1 file changed, 38 insertions(+), 35 deletions(-) diff --git a/core/java/android/net/Network.java b/core/java/android/net/Network.java index c145b2bc18..2bac7a97b7 100644 --- a/core/java/android/net/Network.java +++ b/core/java/android/net/Network.java @@ -27,6 +27,7 @@ import android.system.Os; import android.system.OsConstants; import android.util.proto.ProtoOutputStream; +import com.android.internal.annotations.GuardedBy; import com.android.okhttp.internalandroidapi.Dns; import com.android.okhttp.internalandroidapi.HttpURLConnectionFactory; @@ -70,9 +71,9 @@ public class Network implements Parcelable { // Objects used to perform per-network operations such as getSocketFactory // and openConnection, and a lock to protect access to them. private volatile NetworkBoundSocketFactory mNetworkBoundSocketFactory = null; - // mLock should be used to control write access to mUrlConnectionFactory. - // maybeInitUrlConnectionFactory() must be called prior to reading this field. - private volatile HttpURLConnectionFactory mUrlConnectionFactory; + // mUrlConnectionFactory is initialized lazily when it is first needed. + @GuardedBy("mLock") + private HttpURLConnectionFactory mUrlConnectionFactory; private final Object mLock = new Object(); // Default connection pool values. These are evaluated at startup, just @@ -284,36 +285,16 @@ public class Network implements Parcelable { return mNetworkBoundSocketFactory; } - // TODO: This creates a connection pool and host resolver for - // every Network object, instead of one for every NetId. This is - // suboptimal, because an app could potentially have more than one - // Network object for the same NetId, causing increased memory footprint - // and performance penalties due to lack of connection reuse (connection - // setup time, congestion window growth time, etc.). - // - // Instead, investigate only having one connection pool and host resolver - // for every NetId, perhaps by using a static HashMap of NetIds to - // connection pools and host resolvers. The tricky part is deciding when - // to remove a map entry; a WeakHashMap shouldn't be used because whether - // a Network is referenced doesn't correlate with whether a new Network - // will be instantiated in the near future with the same NetID. A good - // solution would involve purging empty (or when all connections are timed - // out) ConnectionPools. - private void maybeInitUrlConnectionFactory() { - synchronized (mLock) { - if (mUrlConnectionFactory == null) { - // Set configuration on the HttpURLConnectionFactory that will be good for all - // connections created by this Network. Configuration that might vary is left - // until openConnection() and passed as arguments. - Dns dnsLookup = hostname -> Arrays.asList(Network.this.getAllByName(hostname)); - HttpURLConnectionFactory urlConnectionFactory = new HttpURLConnectionFactory(); - urlConnectionFactory.setDns(dnsLookup); // Let traffic go via dnsLookup - // A private connection pool just for this Network. - urlConnectionFactory.setNewConnectionPool(httpMaxConnections, - httpKeepAliveDurationMs, TimeUnit.MILLISECONDS); - mUrlConnectionFactory = urlConnectionFactory; - } - } + private static HttpURLConnectionFactory createUrlConnectionFactory(Dns dnsLookup) { + // Set configuration on the HttpURLConnectionFactory that will be good for all + // connections created by this Network. Configuration that might vary is left + // until openConnection() and passed as arguments. + HttpURLConnectionFactory urlConnectionFactory = new HttpURLConnectionFactory(); + urlConnectionFactory.setDns(dnsLookup); // Let traffic go via dnsLookup + // A private connection pool just for this Network. + urlConnectionFactory.setNewConnectionPool(httpMaxConnections, + httpKeepAliveDurationMs, TimeUnit.MILLISECONDS); + return urlConnectionFactory; } /** @@ -354,9 +335,31 @@ public class Network implements Parcelable { */ public URLConnection openConnection(URL url, java.net.Proxy proxy) throws IOException { if (proxy == null) throw new IllegalArgumentException("proxy is null"); - maybeInitUrlConnectionFactory(); + // TODO: This creates a connection pool and host resolver for + // every Network object, instead of one for every NetId. This is + // suboptimal, because an app could potentially have more than one + // Network object for the same NetId, causing increased memory footprint + // and performance penalties due to lack of connection reuse (connection + // setup time, congestion window growth time, etc.). + // + // Instead, investigate only having one connection pool and host resolver + // for every NetId, perhaps by using a static HashMap of NetIds to + // connection pools and host resolvers. The tricky part is deciding when + // to remove a map entry; a WeakHashMap shouldn't be used because whether + // a Network is referenced doesn't correlate with whether a new Network + // will be instantiated in the near future with the same NetID. A good + // solution would involve purging empty (or when all connections are timed + // out) ConnectionPools. + final HttpURLConnectionFactory urlConnectionFactory; + synchronized (mLock) { + if (mUrlConnectionFactory == null) { + Dns dnsLookup = hostname -> Arrays.asList(getAllByName(hostname)); + mUrlConnectionFactory = createUrlConnectionFactory(dnsLookup); + } + urlConnectionFactory = mUrlConnectionFactory; + } SocketFactory socketFactory = getSocketFactory(); - return mUrlConnectionFactory.openConnection(url, socketFactory, proxy); + return urlConnectionFactory.openConnection(url, socketFactory, proxy); } /**