Speed up NetworkPolicy thread during user creation

Network Policy Manager makes a ton of calls to
UsageStats.isAppIdle, which in turn tries to get
the network scorer and results in a lot of calls to
PackageManager. Caching the network scorer reduces a
lot of the lock contention with PackageManager.

Also, caching calls to check for an app's internet
permission.

Handler for USER_ADDED broadcast reduced in wallclock
duration from 2+ seconds to 114ms.

Bug: 156582823
Test: atest UserLifecycleTests#createAndStartUser
Test: atest AppStandbyTests
Change-Id: I369f9c129ca8a13016e00b1bec2776111fa04513

2 files changed, 61 insertions, 19 deletions

diff --git a/apex/jobscheduler/service/java/com/android/server/usage/AppStandbyController.java b/apex/jobscheduler/service/java/com/android/server/usage/AppStandbyController.java
index 280a6870a5e1..8b5602bc7cd9 100644
--- a/apex/jobscheduler/service/java/com/android/server/usage/AppStandbyController.java
+++ b/apex/jobscheduler/service/java/com/android/server/usage/AppStandbyController.java
@@ -90,6 +90,7 @@ import android.os.Process;
 import android.os.RemoteException;
 import android.os.ServiceManager;
 import android.os.SystemClock;
+import android.os.Trace;
 import android.os.UserHandle;
 import android.provider.Settings.Global;
 import android.telephony.TelephonyManager;
@@ -238,6 +239,14 @@ public class AppStandbyController implements AppStandbyInternal {
 
     private final CountDownLatch mAdminDataAvailableLatch = new CountDownLatch(1);
 
+    // Cache the active network scorer queried from the network scorer service
+    private volatile String mCachedNetworkScorer = null;
+    // The last time the network scorer service was queried
+    private volatile long mCachedNetworkScorerAtMillis = 0L;
+    // How long before querying the network scorer again. During this time, subsequent queries will
+    // get the cached value
+    private static final long NETWORK_SCORER_CACHE_DURATION_MILLIS = 5000L;
+
     // Messages for the handler
     static final int MSG_INFORM_LISTENERS = 3;
     static final int MSG_FORCE_IDLE_STATE = 4;
@@ -1154,6 +1163,8 @@ public class AppStandbyController implements AppStandbyInternal {
             return new int[0];
         }
 
+        Trace.traceBegin(Trace.TRACE_TAG_ACTIVITY_MANAGER, "getIdleUidsForUser");
+
         final long elapsedRealtime = mInjector.elapsedRealtime();
 
         List<ApplicationInfo> apps;
@@ -1189,6 +1200,7 @@ public class AppStandbyController implements AppStandbyInternal {
                 uidStates.setValueAt(index, value + 1 + (idle ? 1<<16 : 0));
             }
         }
+
         if (DEBUG) {
             Slog.d(TAG, "getIdleUids took " + (mInjector.elapsedRealtime() - elapsedRealtime));
         }
@@ -1210,6 +1222,8 @@ public class AppStandbyController implements AppStandbyInternal {
             }
         }
 
+        Trace.traceEnd(Trace.TRACE_TAG_ACTIVITY_MANAGER);
+
         return res;
     }
 
@@ -1576,8 +1590,16 @@ public class AppStandbyController implements AppStandbyInternal {
     }
 
     private boolean isActiveNetworkScorer(String packageName) {
-        String activeScorer = mInjector.getActiveNetworkScorer();
-        return packageName != null && packageName.equals(activeScorer);
+        // Validity of network scorer cache is limited to a few seconds. Fetch it again
+        // if longer since query.
+        // This is a temporary optimization until there's a callback mechanism for changes to network scorer.
+        final long now = SystemClock.elapsedRealtime();
+        if (mCachedNetworkScorer == null
+                || mCachedNetworkScorerAtMillis < now - NETWORK_SCORER_CACHE_DURATION_MILLIS) {
+            mCachedNetworkScorer = mInjector.getActiveNetworkScorer();
+            mCachedNetworkScorerAtMillis = now;
+        }
+        return packageName != null && packageName.equals(mCachedNetworkScorer);
     }
 
     private void informListeners(String packageName, int userId, int bucket, int reason,
diff --git a/services/core/java/com/android/server/net/NetworkPolicyManagerService.java b/services/core/java/com/android/server/net/NetworkPolicyManagerService.java
index 3283fd9b2c51..87f0fb14ee33 100644
--- a/services/core/java/com/android/server/net/NetworkPolicyManagerService.java
+++ b/services/core/java/com/android/server/net/NetworkPolicyManagerService.java
@@ -578,6 +578,10 @@ public class NetworkPolicyManagerService extends INetworkPolicyManager.Stub {
 
     private final NetworkPolicyLogger mLogger = new NetworkPolicyLogger();
 
+    /** List of apps indexed by appId and whether they have the internet permission */
+    @GuardedBy("mUidRulesFirstLock")
+    private final SparseBooleanArray mInternetPermissionMap = new SparseBooleanArray();
+
     // TODO: keep whitelist of system-critical services that should never have
     // rules enforced, such as system, phone, and radio UIDs.
 
@@ -958,7 +962,9 @@ public class NetworkPolicyManagerService extends INetworkPolicyManager.Stub {
                 // update rules for UID, since it might be subject to
                 // global background data policy
                 if (LOGV) Slog.v(TAG, "ACTION_PACKAGE_ADDED for uid=" + uid);
+                // Clear the cache for the app
                 synchronized (mUidRulesFirstLock) {
+                    mInternetPermissionMap.delete(UserHandle.getAppId(uid));
                     updateRestrictionRulesForUidUL(uid);
                 }
             }
@@ -1000,7 +1006,7 @@ public class NetworkPolicyManagerService extends INetworkPolicyManager.Stub {
                     synchronized (mUidRulesFirstLock) {
                         // Remove any persistable state for the given user; both cleaning up after a
                         // USER_REMOVED, and one last sanity check during USER_ADDED
-                        removeUserStateUL(userId, true);
+                        removeUserStateUL(userId, true, false);
                         // Removing outside removeUserStateUL since that can also be called when
                         // user resets app preferences.
                         mMeteredRestrictedUids.remove(userId);
@@ -2613,7 +2619,7 @@ public class NetworkPolicyManagerService extends INetworkPolicyManager.Stub {
         setUidPolicyUncheckedUL(uid, policy, false);
 
         final boolean notifyApp;
-        if (!isUidValidForWhitelistRules(uid)) {
+        if (!isUidValidForWhitelistRulesUL(uid)) {
             notifyApp = false;
         } else {
             final boolean wasBlacklisted = oldPolicy == POLICY_REJECT_METERED_BACKGROUND;
@@ -2689,7 +2695,7 @@ public class NetworkPolicyManagerService extends INetworkPolicyManager.Stub {
      * if any changes that are made.
      */
     @GuardedBy("mUidRulesFirstLock")
-    boolean removeUserStateUL(int userId, boolean writePolicy) {
+    boolean removeUserStateUL(int userId, boolean writePolicy, boolean updateGlobalRules) {
 
         mLogger.removingUserState(userId);
         boolean changed = false;
@@ -2719,7 +2725,9 @@ public class NetworkPolicyManagerService extends INetworkPolicyManager.Stub {
             changed = true;
         }
         synchronized (mNetworkPoliciesSecondLock) {
-            updateRulesForGlobalChangeAL(true);
+            if (updateGlobalRules) {
+                updateRulesForGlobalChangeAL(true);
+            }
             if (writePolicy && changed) {
                 writePolicyAL();
             }
@@ -3859,7 +3867,7 @@ public class NetworkPolicyManagerService extends INetworkPolicyManager.Stub {
                         // quick check: if this uid doesn't have INTERNET permission, it
                         // doesn't have network access anyway, so it is a waste to mess
                         // with it here.
-                        if (hasInternetPermissions(uid)) {
+                        if (hasInternetPermissionUL(uid)) {
                             uidRules.put(uid, FIREWALL_RULE_DENY);
                         }
                     }
@@ -3874,7 +3882,7 @@ public class NetworkPolicyManagerService extends INetworkPolicyManager.Stub {
 
     @GuardedBy("mUidRulesFirstLock")
     void updateRuleForAppIdleUL(int uid) {
-        if (!isUidValidForBlacklistRules(uid)) return;
+        if (!isUidValidForBlacklistRulesUL(uid)) return;
 
         if (Trace.isTagEnabled(Trace.TRACE_TAG_NETWORK)) {
             Trace.traceBegin(Trace.TRACE_TAG_NETWORK, "updateRuleForAppIdleUL: " + uid );
@@ -4056,18 +4064,20 @@ public class NetworkPolicyManagerService extends INetworkPolicyManager.Stub {
 
     // TODO: the MEDIA / DRM restriction might not be needed anymore, in which case both
     // methods below could be merged into a isUidValidForRules() method.
-    private boolean isUidValidForBlacklistRules(int uid) {
+    @GuardedBy("mUidRulesFirstLock")
+    private boolean isUidValidForBlacklistRulesUL(int uid) {
         // allow rules on specific system services, and any apps
         if (uid == android.os.Process.MEDIA_UID || uid == android.os.Process.DRM_UID
-            || (UserHandle.isApp(uid) && hasInternetPermissions(uid))) {
+                || isUidValidForWhitelistRulesUL(uid)) {
             return true;
         }
 
         return false;
     }
 
-    private boolean isUidValidForWhitelistRules(int uid) {
-        return UserHandle.isApp(uid) && hasInternetPermissions(uid);
+    @GuardedBy("mUidRulesFirstLock")
+    private boolean isUidValidForWhitelistRulesUL(int uid) {
+        return UserHandle.isApp(uid) && hasInternetPermissionUL(uid);
     }
 
     /**
@@ -4143,12 +4153,20 @@ public class NetworkPolicyManagerService extends INetworkPolicyManager.Stub {
      * <p>
      * Useful for the cases where the lack of network access can simplify the rules.
      */
-    private boolean hasInternetPermissions(int uid) {
+    @GuardedBy("mUidRulesFirstLock")
+    private boolean hasInternetPermissionUL(int uid) {
         try {
-            if (mIPm.checkUidPermission(Manifest.permission.INTERNET, uid)
-                    != PackageManager.PERMISSION_GRANTED) {
-                return false;
+            final int appId = UserHandle.getAppId(uid);
+            final boolean hasPermission;
+            if (mInternetPermissionMap.indexOfKey(appId) < 0) {
+                hasPermission =
+                        mIPm.checkUidPermission(Manifest.permission.INTERNET, uid)
+                                == PackageManager.PERMISSION_GRANTED;
+                mInternetPermissionMap.put(appId, hasPermission);
+            } else {
+                hasPermission = mInternetPermissionMap.get(appId);
             }
+            return hasPermission;
         } catch (RemoteException e) {
         }
         return true;
@@ -4253,7 +4271,7 @@ public class NetworkPolicyManagerService extends INetworkPolicyManager.Stub {
     }
 
     private void updateRulesForDataUsageRestrictionsULInner(int uid) {
-        if (!isUidValidForWhitelistRules(uid)) {
+        if (!isUidValidForWhitelistRulesUL(uid)) {
             if (LOGD) Slog.d(TAG, "no need to update restrict data rules for uid " + uid);
             return;
         }
@@ -4400,6 +4418,7 @@ public class NetworkPolicyManagerService extends INetworkPolicyManager.Stub {
      *
      * @return the new computed rules for the uid
      */
+    @GuardedBy("mUidRulesFirstLock")
     private int updateRulesForPowerRestrictionsUL(int uid, int oldUidRules, boolean paroled) {
         if (Trace.isTagEnabled(Trace.TRACE_TAG_NETWORK)) {
             Trace.traceBegin(Trace.TRACE_TAG_NETWORK,
@@ -4413,8 +4432,9 @@ public class NetworkPolicyManagerService extends INetworkPolicyManager.Stub {
         }
     }
 
+    @GuardedBy("mUidRulesFirstLock")
     private int updateRulesForPowerRestrictionsULInner(int uid, int oldUidRules, boolean paroled) {
-        if (!isUidValidForBlacklistRules(uid)) {
+        if (!isUidValidForBlacklistRulesUL(uid)) {
             if (LOGD) Slog.d(TAG, "no need to update restrict power rules for uid " + uid);
             return RULE_NONE;
         }
@@ -5198,7 +5218,7 @@ public class NetworkPolicyManagerService extends INetworkPolicyManager.Stub {
         @Override
         public void resetUserState(int userId) {
             synchronized (mUidRulesFirstLock) {
-                boolean changed = removeUserStateUL(userId, false);
+                boolean changed = removeUserStateUL(userId, false, true);
                 changed = addDefaultRestrictBackgroundWhitelistUidsUL(userId) || changed;
                 if (changed) {
                     synchronized (mNetworkPoliciesSecondLock) {