From f6554485233e37bf80e324f600f795bded9a94c3 Mon Sep 17 00:00:00 2001
From: Soonil Nagarkar <sooniln@google.com>
Date: Wed, 10 Feb 2021 12:03:02 -0800
Subject: [PATCH] Further reduce LM memory cost

Reduce number of generated synthetic accessors for private var/methods,
put ProviderRequest listener into lazily loaded class, and switch to
ArrayMap instead of WeakHashMap when server-side transport removal is
not a consideration. Also adds support for replacing an old transport
in a single remote call, rather than two remote calls.

Bug: 13463193
Test: none
Change-Id: I09dab317dc82aeda51422af1047274631081c3e1
---
 .../listeners/ListenerTransportManager.java   | 56 +++++++++++++++----
 .../android/location/LocationManager.java     | 51 ++++++++++++++---
 2 files changed, 88 insertions(+), 19 deletions(-)

diff --git a/core/java/com/android/internal/listeners/ListenerTransportManager.java b/core/java/com/android/internal/listeners/ListenerTransportManager.java
index 0d5d1b7b53ffc..d5b561939836a 100644
--- a/core/java/com/android/internal/listeners/ListenerTransportManager.java
+++ b/core/java/com/android/internal/listeners/ListenerTransportManager.java
@@ -17,6 +17,7 @@
 package com.android.internal.listeners;
 
 import android.os.RemoteException;
+import android.util.ArrayMap;
 
 import com.android.internal.annotations.GuardedBy;
 
@@ -36,13 +37,17 @@ public abstract class ListenerTransportManager<TTransport extends ListenerTransp
     @GuardedBy("mRegistrations")
     private final Map<Object, WeakReference<TTransport>> mRegistrations;
 
-    protected ListenerTransportManager() {
+    protected ListenerTransportManager(boolean allowServerSideTransportRemoval) {
         // using weakhashmap means that the transport may be GCed if the server drops its reference,
         // and thus the listener may be GCed as well if the client drops that reference. if the
         // server will never drop a reference without warning (ie, transport removal may only be
         // initiated from the client side), then arraymap or similar may be used without fear of
         // memory leaks.
-        mRegistrations = new WeakHashMap<>();
+        if (allowServerSideTransportRemoval) {
+            mRegistrations = new WeakHashMap<>();
+        } else {
+            mRegistrations = new ArrayMap<>();
+        }
     }
 
     /**
@@ -53,16 +58,21 @@ public abstract class ListenerTransportManager<TTransport extends ListenerTransp
             synchronized (mRegistrations) {
                 // ordering of operations is important so that if an error occurs at any point we
                 // are left in a reasonable state
-                registerTransport(transport);
-                WeakReference<TTransport> oldTransportRef = mRegistrations.put(key,
-                        new WeakReference<>(transport));
+                TTransport oldTransport;
+                WeakReference<TTransport> oldTransportRef = mRegistrations.get(key);
                 if (oldTransportRef != null) {
-                    TTransport oldTransport = oldTransportRef.get();
-                    if (oldTransport != null) {
-                        oldTransport.unregister();
-                        unregisterTransport(oldTransport);
-                    }
+                    oldTransport = oldTransportRef.get();
+                } else {
+                    oldTransport = null;
                 }
+
+                if (oldTransport == null) {
+                    registerTransport(transport);
+                } else {
+                    registerTransport(transport, oldTransport);
+                    oldTransport.unregister();
+                }
+                mRegistrations.put(key, new WeakReference<>(transport));
             }
         } catch (RemoteException e) {
             throw e.rethrowFromSystemServer();
@@ -91,7 +101,33 @@ public abstract class ListenerTransportManager<TTransport extends ListenerTransp
         }
     }
 
+    /**
+     * Registers a new transport.
+     */
     protected abstract void registerTransport(TTransport transport) throws RemoteException;
 
+    /**
+     * Registers a new transport that is replacing the given old transport. Implementations must
+     * ensure that if they throw a remote exception, the call does not have any side effects.
+     */
+    protected void registerTransport(TTransport transport, TTransport oldTransport)
+            throws RemoteException {
+        registerTransport(transport);
+        try {
+            unregisterTransport(oldTransport);
+        } catch (RemoteException e) {
+            try {
+                // best effort to ensure there are no side effects
+                unregisterTransport(transport);
+            } catch (RemoteException suppressed) {
+                e.addSuppressed(suppressed);
+            }
+            throw e;
+        }
+    }
+
+    /**
+     * Unregisters an existing transport.
+     */
     protected abstract void unregisterTransport(TTransport transport) throws RemoteException;
 }
diff --git a/location/java/android/location/LocationManager.java b/location/java/android/location/LocationManager.java
index 088b789ea6908..b7823400695d9 100644
--- a/location/java/android/location/LocationManager.java
+++ b/location/java/android/location/LocationManager.java
@@ -20,7 +20,6 @@ import static android.Manifest.permission.ACCESS_COARSE_LOCATION;
 import static android.Manifest.permission.ACCESS_FINE_LOCATION;
 import static android.Manifest.permission.LOCATION_HARDWARE;
 import static android.Manifest.permission.WRITE_SECURE_SETTINGS;
-import static android.location.GpsStatus.GPS_EVENT_STARTED;
 import static android.location.LocationRequest.createFromDeprecatedCriteria;
 import static android.location.LocationRequest.createFromDeprecatedProvider;
 
@@ -412,7 +411,7 @@ public class LocationManager {
     private static final String CACHE_KEY_LOCATION_ENABLED_PROPERTY =
             "cache_key.location_enabled";
 
-    private static ILocationManager getService() throws RemoteException {
+    static ILocationManager getService() throws RemoteException {
         try {
             return ILocationManager.Stub.asInterface(
                     ServiceManager.getServiceOrThrow(Context.LOCATION_SERVICE));
@@ -439,11 +438,13 @@ public class LocationManager {
                 new GnssNavigationTransportManager();
     }
 
-    private static final ProviderRequestTransportManager sProviderRequestListeners =
-            new ProviderRequestTransportManager();
+    private static class ProviderRequestLazyLoader {
+        static final ProviderRequestTransportManager sProviderRequestListeners =
+                new ProviderRequestTransportManager();
+    }
 
-    private final Context mContext;
-    private final ILocationManager mService;
+    final Context mContext;
+    final ILocationManager mService;
 
     private volatile PropertyInvalidatedCache<Integer, Boolean> mLocationEnabledCache =
             new PropertyInvalidatedCache<Integer, Boolean>(
@@ -2790,7 +2791,7 @@ public class LocationManager {
     public boolean registerProviderRequestListener(
             @NonNull @CallbackExecutor Executor executor,
             @NonNull Listener listener) {
-        sProviderRequestListeners.addListener(listener,
+        ProviderRequestLazyLoader.sProviderRequestListeners.addListener(listener,
                 new ProviderRequestTransport(executor, listener));
         return true;
     }
@@ -2805,7 +2806,7 @@ public class LocationManager {
     @RequiresPermission(Manifest.permission.LOCATION_HARDWARE)
     public void unregisterProviderRequestListener(
             @NonNull Listener listener) {
-        sProviderRequestListeners.removeListener(listener);
+        ProviderRequestLazyLoader.sProviderRequestListeners.removeListener(listener);
     }
 
     /**
@@ -2917,6 +2918,10 @@ public class LocationManager {
     private static class GnssStatusTransportManager extends
             ListenerTransportManager<GnssStatusTransport> {
 
+        GnssStatusTransportManager() {
+            super(false);
+        }
+
         @Override
         protected void registerTransport(GnssStatusTransport transport)
                             throws RemoteException {
@@ -2934,6 +2939,10 @@ public class LocationManager {
     private static class GnssNmeaTransportManager extends
             ListenerTransportManager<GnssNmeaTransport> {
 
+        GnssNmeaTransportManager() {
+            super(false);
+        }
+
         @Override
         protected void registerTransport(GnssNmeaTransport transport)
                             throws RemoteException {
@@ -2951,6 +2960,10 @@ public class LocationManager {
     private static class GnssMeasurementsTransportManager extends
             ListenerTransportManager<GnssMeasurementsTransport> {
 
+        GnssMeasurementsTransportManager() {
+            super(false);
+        }
+
         @Override
         protected void registerTransport(GnssMeasurementsTransport transport)
                             throws RemoteException {
@@ -2968,6 +2981,10 @@ public class LocationManager {
     private static class GnssAntennaTransportManager extends
             ListenerTransportManager<GnssAntennaInfoTransport> {
 
+        GnssAntennaTransportManager() {
+            super(false);
+        }
+
         @Override
         protected void registerTransport(GnssAntennaInfoTransport transport) {
             transport.getContext().registerReceiver(transport,
@@ -2983,6 +3000,10 @@ public class LocationManager {
     private static class GnssNavigationTransportManager extends
             ListenerTransportManager<GnssNavigationTransport> {
 
+        GnssNavigationTransportManager() {
+            super(false);
+        }
+
         @Override
         protected void registerTransport(GnssNavigationTransport transport)
                             throws RemoteException {
@@ -3000,6 +3021,10 @@ public class LocationManager {
     private static class ProviderRequestTransportManager extends
             ListenerTransportManager<ProviderRequestTransport> {
 
+        ProviderRequestTransportManager() {
+            super(false);
+        }
+
         @Override
         protected void registerTransport(ProviderRequestTransport transport)
                 throws RemoteException {
@@ -3117,6 +3142,8 @@ public class LocationManager {
         }
     }
 
+    /** @deprecated */
+    @Deprecated
     private static class GpsAdapter extends GnssStatus.Callback {
 
         private final GpsStatus.Listener mGpsListener;
@@ -3127,7 +3154,7 @@ public class LocationManager {
 
         @Override
         public void onStarted() {
-            mGpsListener.onGpsStatusChanged(GPS_EVENT_STARTED);
+            mGpsListener.onGpsStatusChanged(GpsStatus.GPS_EVENT_STARTED);
         }
 
         @Override
@@ -3204,6 +3231,8 @@ public class LocationManager {
         }
     }
 
+    /** @deprecated */
+    @Deprecated
     private static class GpsStatusTransport extends GnssStatusTransport {
 
         static volatile int sTtff;
@@ -3442,6 +3471,8 @@ public class LocationManager {
         }
     }
 
+    /** @deprecated */
+    @Deprecated
     private static class BatchedLocationCallbackWrapper implements LocationListener {
 
         private final BatchedLocationCallback mCallback;
@@ -3461,6 +3492,8 @@ public class LocationManager {
         }
     }
 
+    /** @deprecated */
+    @Deprecated
     private static class BatchedLocationCallbackTransport extends LocationListenerTransport {
 
         BatchedLocationCallbackTransport(BatchedLocationCallback callback, Handler handler) {