Merge "Reduce lock contention on setting write." into tm-dev

services/core/java/com/android/server/pm/PackageManagerService.java

@@ -1430,7 +1430,7 @@ public class PackageManagerService implements PackageSender, TestUtilityService
                 (i, pm) -> new Settings(Environment.getDataDirectory(),
                         RuntimePermissionsPersistence.createInstance(),
                         i.getPermissionManagerServiceInternal(),
-                        domainVerificationService, lock),
+                        domainVerificationService, backgroundHandler, lock),
                 (i, pm) -> AppsFilterImpl.create(i,
                         i.getLocalService(PackageManagerInternal.class)),
                 (i, pm) -> (PlatformCompat) ServiceManager.getService("platform_compat"),

services/core/java/com/android/server/pm/Settings.java

@@ -360,6 +360,8 @@ public final class Settings implements Watchable, Snappable {
     private static final String ATTR_VALUE = "value";
     private static final String ATTR_FIRST_INSTALL_TIME = "first-install-time";
 
+    private final Handler mHandler;
+
     private final PackageManagerTracedLock mLock;
 
     @Watched(manual = true)
@@ -583,6 +585,8 @@ public final class Settings implements Watchable, Snappable {
                                          "Settings.mInstallerPackages");
         mKeySetManagerService = new KeySetManagerService(mPackages);
 
+        // Test-only handler working on background thread.
+        mHandler = new Handler(BackgroundThread.getHandler().getLooper());
         mLock = new PackageManagerTracedLock();
         mPackages.putAll(pkgSettings);
         mAppIds = new AppIdSettingMap();
@@ -607,6 +611,7 @@ public final class Settings implements Watchable, Snappable {
     Settings(File dataDir, RuntimePermissionsPersistence runtimePermissionsPersistence,
             LegacyPermissionDataProvider permissionDataProvider,
             @NonNull DomainVerificationManagerInternal domainVerificationManager,
+            @NonNull Handler handler,
             @NonNull PackageManagerTracedLock lock)  {
         mPackages = new WatchedArrayMap<>();
         mPackagesSnapshot  =
@@ -620,6 +625,7 @@ public final class Settings implements Watchable, Snappable {
                                          "Settings.mInstallerPackages");
         mKeySetManagerService = new KeySetManagerService(mPackages);
 
+        mHandler = handler;
         mLock = lock;
         mAppIds = new AppIdSettingMap();
         mPermissions = new LegacyPermissionSettings(lock);
@@ -627,10 +633,8 @@ public final class Settings implements Watchable, Snappable {
                 runtimePermissionsPersistence, new Consumer<Integer>() {
             @Override
             public void accept(Integer userId) {
-                synchronized (mLock) {
+                mRuntimePermissionsPersistence.writeStateForUser(userId,
-                    mRuntimePermissionsPersistence.writeStateForUserSync(userId,
+                        mPermissionDataProvider, mPackages, mSharedUsers, mHandler, mLock);
-                            mPermissionDataProvider, mPackages, mSharedUsers);
-                }
             }
         });
         mPermissionDataProvider = permissionDataProvider;
@@ -679,6 +683,7 @@ public final class Settings implements Watchable, Snappable {
         // needed by the read-only methods.  Note especially that the lock
         // is not required because this clone is meant to support lock-free
         // read-only methods.
+        mHandler = null;
         mLock = null;
         mRuntimePermissionsPersistence = r.mRuntimePermissionsPersistence;
         mSettingsFilename = null;
@@ -5286,8 +5291,8 @@ public final class Settings implements Watchable, Snappable {
 
     public void writePermissionStateForUserLPr(int userId, boolean sync) {
         if (sync) {
-            mRuntimePermissionsPersistence.writeStateForUserSync(userId, mPermissionDataProvider,
+            mRuntimePermissionsPersistence.writeStateForUser(userId, mPermissionDataProvider,
-                    mPackages, mSharedUsers);
+                    mPackages, mSharedUsers, /*handler=*/null, mLock);
         } else {
             mRuntimePermissionsPersistence.writeStateForUserAsync(userId);
         }
@@ -5373,9 +5378,14 @@ public final class Settings implements Watchable, Snappable {
 
         private String mExtendedFingerprint;
 
+        @GuardedBy("mPersistenceLock")
         private final RuntimePermissionsPersistence mPersistence;
+        private final Object mPersistenceLock = new Object();
 
-        private final Handler mHandler = new MyHandler();
+        // Low-priority handlers running on SystemBg thread.
+        private final Handler mAsyncHandler = new MyHandler();
+        private final Handler mPersistenceHandler = new Handler(
+                BackgroundThread.getHandler().getLooper());
 
         private final Object mLock = new Object();
 
@@ -5398,6 +5408,11 @@ public final class Settings implements Watchable, Snappable {
         // The mapping keys are user ids.
         private final SparseBooleanArray mPermissionUpgradeNeeded = new SparseBooleanArray();
 
+        @GuardedBy("mLock")
+        // Staging area for states prepared to be written.
+        private final SparseArray<RuntimePermissionsState> mPendingStatesToWrite =
+                new SparseArray<>();
+
         // This is a hack to allow this class to invoke a write using Settings's data structures,
         // to facilitate moving to a finer scoped lock without a significant refactor.
         private final Consumer<Integer> mInvokeWriteUserStateAsyncCallback;
@@ -5462,7 +5477,7 @@ public final class Settings implements Watchable, Snappable {
                 final long currentTimeMillis = SystemClock.uptimeMillis();
 
                 if (mWriteScheduled.get(userId)) {
-                    mHandler.removeMessages(userId);
+                    mAsyncHandler.removeMessages(userId);
 
                     // If enough time passed, write without holding off anymore.
                     final long lastNotWrittenMutationTimeMillis = mLastNotWrittenMutationTimesMillis
@@ -5471,7 +5486,7 @@ public final class Settings implements Watchable, Snappable {
                             - lastNotWrittenMutationTimeMillis;
                     if (timeSinceLastNotWrittenMutationMillis
                             >= MAX_WRITE_PERMISSIONS_DELAY_MILLIS) {
-                        mHandler.obtainMessage(userId).sendToTarget();
+                        mAsyncHandler.obtainMessage(userId).sendToTarget();
                         return;
                     }
 
@@ -5481,67 +5496,110 @@ public final class Settings implements Watchable, Snappable {
                     final long writeDelayMillis = Math.min(WRITE_PERMISSIONS_DELAY_MILLIS,
                             maxDelayMillis);
 
-                    Message message = mHandler.obtainMessage(userId);
+                    Message message = mAsyncHandler.obtainMessage(userId);
-                    mHandler.sendMessageDelayed(message, writeDelayMillis);
+                    mAsyncHandler.sendMessageDelayed(message, writeDelayMillis);
                 } else {
                     mLastNotWrittenMutationTimesMillis.put(userId, currentTimeMillis);
-                    Message message = mHandler.obtainMessage(userId);
+                    Message message = mAsyncHandler.obtainMessage(userId);
-                    mHandler.sendMessageDelayed(message, WRITE_PERMISSIONS_DELAY_MILLIS);
+                    mAsyncHandler.sendMessageDelayed(message, WRITE_PERMISSIONS_DELAY_MILLIS);
                     mWriteScheduled.put(userId, true);
                 }
             }
         }
 
-        public void writeStateForUserSync(int userId, @NonNull LegacyPermissionDataProvider
+        public void writeStateForUser(int userId, @NonNull LegacyPermissionDataProvider
                 legacyPermissionDataProvider,
                 @NonNull WatchedArrayMap<String, ? extends PackageStateInternal> packageStates,
-                @NonNull WatchedArrayMap<String, SharedUserSetting> sharedUsers) {
+                @NonNull WatchedArrayMap<String, SharedUserSetting> sharedUsers,
+                @Nullable Handler pmHandler, @NonNull Object pmLock) {
+            final int version;
+            final String fingerprint;
             synchronized (mLock) {
-                mHandler.removeMessages(userId);
+                mAsyncHandler.removeMessages(userId);
                 mWriteScheduled.delete(userId);
 
-                legacyPermissionDataProvider.writeLegacyPermissionStateTEMP();
+                version = mVersions.get(userId, INITIAL_VERSION);
+                fingerprint = mFingerprints.get(userId);
+            }
 
-                int version = mVersions.get(userId, INITIAL_VERSION);
+            Runnable writer = () -> {
+                final RuntimePermissionsState runtimePermissions;
+                synchronized (pmLock) {
+                    legacyPermissionDataProvider.writeLegacyPermissionStateTEMP();
 
-                String fingerprint = mFingerprints.get(userId);
+                    Map<String, List<RuntimePermissionsState.PermissionState>> packagePermissions =
+                            new ArrayMap<>();
+                    int packagesSize = packageStates.size();
+                    for (int i = 0; i < packagesSize; i++) {
+                        String packageName = packageStates.keyAt(i);
+                        PackageStateInternal packageState = packageStates.valueAt(i);
+                        if (!packageState.hasSharedUser()) {
+                            List<RuntimePermissionsState.PermissionState> permissions =
+                                    getPermissionsFromPermissionsState(
+                                            packageState.getLegacyPermissionState(), userId);
+                            if (permissions.isEmpty()
+                                    && !packageState.isInstallPermissionsFixed()) {
+                                // Storing an empty state means the package is known to the
+                                // system and its install permissions have been granted and fixed.
+                                // If this is not the case, we should not store anything.
+                                continue;
+                            }
+                            packagePermissions.put(packageName, permissions);
+                        }
+                    }
 
-                Map<String, List<RuntimePermissionsState.PermissionState>> packagePermissions =
+                    Map<String, List<RuntimePermissionsState.PermissionState>>
-                        new ArrayMap<>();
+                            sharedUserPermissions =
-                int packagesSize = packageStates.size();
+                            new ArrayMap<>();
-                for (int i = 0; i < packagesSize; i++) {
+                    final int sharedUsersSize = sharedUsers.size();
-                    String packageName = packageStates.keyAt(i);
+                    for (int i = 0; i < sharedUsersSize; i++) {
-                    PackageStateInternal packageState = packageStates.valueAt(i);
+                        String sharedUserName = sharedUsers.keyAt(i);
-                    if (!packageState.hasSharedUser()) {
+                        SharedUserSetting sharedUserSetting = sharedUsers.valueAt(i);
                         List<RuntimePermissionsState.PermissionState> permissions =
                                 getPermissionsFromPermissionsState(
-                                        packageState.getLegacyPermissionState(), userId);
+                                        sharedUserSetting.getLegacyPermissionState(), userId);
-                        if (permissions.isEmpty() && !packageState.isInstallPermissionsFixed()) {
+                        sharedUserPermissions.put(sharedUserName, permissions);
-                            // Storing an empty state means the package is known to the system and
-                            // its install permissions have been granted and fixed. If this is not
-                            // the case, we should not store anything.
-                            continue;
-                        }
-                        packagePermissions.put(packageName, permissions);
                     }
+
+                    runtimePermissions = new RuntimePermissionsState(version,
+                            fingerprint, packagePermissions, sharedUserPermissions);
                 }
-
+                synchronized (mLock) {
-                Map<String, List<RuntimePermissionsState.PermissionState>> sharedUserPermissions =
+                    mPendingStatesToWrite.put(userId, runtimePermissions);
-                        new ArrayMap<>();
-                final int sharedUsersSize = sharedUsers.size();
-                for (int i = 0; i < sharedUsersSize; i++) {
-                    String sharedUserName = sharedUsers.keyAt(i);
-                    SharedUserSetting sharedUserSetting = sharedUsers.valueAt(i);
-                    List<RuntimePermissionsState.PermissionState> permissions =
-                            getPermissionsFromPermissionsState(
-                                    sharedUserSetting.getLegacyPermissionState(), userId);
-                    sharedUserPermissions.put(sharedUserName, permissions);
                 }
+                if (pmHandler != null) {
+                    // Async version.
+                    mPersistenceHandler.post(() -> writePendingStates());
+                } else {
+                    // Sync version.
+                    writePendingStates();
+                }
+            };
 
-                RuntimePermissionsState runtimePermissions = new RuntimePermissionsState(version,
+            if (pmHandler != null) {
-                        fingerprint, packagePermissions, sharedUserPermissions);
+                // Async version, use pmHandler.
+                pmHandler.post(writer);
+            } else {
+                // Sync version, use caller's thread.
+                writer.run();
+            }
+        }
 
-                mPersistence.writeForUser(runtimePermissions, UserHandle.of(userId));
+        private void writePendingStates() {
+            while (true) {
+                final RuntimePermissionsState runtimePermissions;
+                final int userId;
+                synchronized (mLock) {
+                    if (mPendingStatesToWrite.size() == 0) {
+                        break;
+                    }
+                    userId = mPendingStatesToWrite.keyAt(0);
+                    runtimePermissions = mPendingStatesToWrite.valueAt(0);
+                    mPendingStatesToWrite.removeAt(0);
+                }
+                synchronized (mPersistenceLock) {
+                    mPersistence.writeForUser(runtimePermissions, UserHandle.of(userId));
+                }
             }
         }
 
@@ -5563,7 +5621,7 @@ public final class Settings implements Watchable, Snappable {
         private void onUserRemoved(int userId) {
             synchronized (mLock) {
                 // Make sure we do not
-                mHandler.removeMessages(userId);
+                mAsyncHandler.removeMessages(userId);
 
                 mPermissionUpgradeNeeded.delete(userId);
                 mVersions.delete(userId);
@@ -5572,7 +5630,7 @@ public final class Settings implements Watchable, Snappable {
         }
 
         public void deleteUserRuntimePermissionsFile(int userId) {
-            synchronized (mLock) {
+            synchronized (mPersistenceLock) {
                 mPersistence.deleteForUser(UserHandle.of(userId));
             }
         }
@@ -5581,16 +5639,17 @@ public final class Settings implements Watchable, Snappable {
                 @NonNull WatchedArrayMap<String, PackageSetting> packageSettings,
                 @NonNull WatchedArrayMap<String, SharedUserSetting> sharedUsers,
                 @NonNull File userRuntimePermissionsFile) {
+            final RuntimePermissionsState runtimePermissions;
+            synchronized (mPersistenceLock) {
+                runtimePermissions = mPersistence.readForUser(UserHandle.of(userId));
+            }
+            if (runtimePermissions == null) {
+                readLegacyStateForUserSync(userId, userRuntimePermissionsFile, packageSettings,
+                        sharedUsers);
+                writeStateForUserAsync(userId);
+                return;
+            }
             synchronized (mLock) {
-                RuntimePermissionsState runtimePermissions = mPersistence.readForUser(UserHandle.of(
-                        userId));
-                if (runtimePermissions == null) {
-                    readLegacyStateForUserSync(userId, userRuntimePermissionsFile, packageSettings,
-                            sharedUsers);
-                    writeStateForUserAsync(userId);
-                    return;
-                }
-
                 // If the runtime permissions file exists but the version is not set this is
                 // an upgrade from P->Q. Hence mark it with the special UPGRADE_VERSION.
                 int version = runtimePermissions.getVersion();

services/tests/servicestests/src/com/android/server/pm/PackageManagerSettingsTests.java

@@ -1516,7 +1516,7 @@ public class PackageManagerSettingsTests {
     private Settings makeSettings() {
         return new Settings(InstrumentationRegistry.getContext().getFilesDir(),
                 mRuntimePermissionsPersistence, mPermissionDataProvider,
-                mDomainVerificationManager, new PackageManagerTracedLock());
+                mDomainVerificationManager, null, new PackageManagerTracedLock());
     }
 
     private void verifyKeySetMetaData(Settings settings)