Merge changes I78cd6a32,I99656d57 into tm-dev am: c16c6a2a78

Original change: https://googleplex-android-review.googlesource.com/c/platform/frameworks/base/+/18351504

Change-Id: If47fd705a81f3cc3b5d98df3ca28780c0ac2d2eb
Signed-off-by: Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com>

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

@@ -94,12 +94,13 @@ import java.util.Set;
 @VisibleForTesting(visibility = VisibleForTesting.Visibility.PRIVATE)
 public interface Computer extends PackageDataSnapshot {
 
+    int getVersion();
+
     /**
      * Administrative statistics: record that the snapshot has been used.  Every call
      * to use() increments the usage counter.
      */
-    default void use() {
+    Computer use();
-    }
     /**
      * Fetch the snapshot usage counter.
      * @return The number of times this snapshot was used.

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

@@ -367,6 +367,8 @@ public class ComputerEngine implements Computer {
         return (v1 > v2) ? -1 : ((v1 < v2) ? 1 : 0);
     };
 
+    private final int mVersion;
+
     // The administrative use counter.
     private int mUsed = 0;
 
@@ -424,7 +426,8 @@ public class ComputerEngine implements Computer {
         return mLocalAndroidApplication;
     }
 
-    ComputerEngine(PackageManagerService.Snapshot args) {
+    ComputerEngine(PackageManagerService.Snapshot args, int version) {
+        mVersion = version;
         mSettings = new Settings(args.settings);
         mIsolatedOwners = args.isolatedOwners;
         mPackages = args.packages;
@@ -464,11 +467,17 @@ public class ComputerEngine implements Computer {
         mService = args.service;
     }
 
+    @Override
+    public int getVersion() {
+        return mVersion;
+    }
+
     /**
      * Record that the snapshot was used.
      */
-    public final void use() {
+    public final Computer use() {
         mUsed++;
+        return this;
     }
 
     /**

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

@@ -30,7 +30,7 @@ import com.android.internal.annotations.VisibleForTesting;
 public final class ComputerLocked extends ComputerEngine {
 
     ComputerLocked(PackageManagerService.Snapshot args) {
-        super(args);
+        super(args, -1);
     }
 
     protected ComponentName resolveComponentName() {

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

@@ -271,8 +271,8 @@ import java.util.Set;
 import java.util.concurrent.Executor;
 import java.util.concurrent.Future;
 import java.util.concurrent.TimeUnit;
-import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.atomic.AtomicReference;
 import java.util.function.Consumer;
 
 /**
@@ -1038,22 +1038,15 @@ public class PackageManagerService implements PackageSender, TestUtilityService
     // times during the PackageManagerService constructor but it should not be modified thereafter.
     private ComputerLocked mLiveComputer;
 
-    // A lock-free cache for frequently called functions.
+    private static final AtomicReference<Computer> sSnapshot = new AtomicReference<>();
-    private volatile Computer mSnapshotComputer;
 
-    // If true, the snapshot is invalid (stale).  The attribute is static since it may be
+    // If this differs from Computer#getVersion, the snapshot is invalid (stale).
-    // set from outside classes.  The attribute may be set to true anywhere, although it
+    private static final AtomicInteger sSnapshotPendingVersion = new AtomicInteger(1);
-    // should only be set true while holding mLock.  However, the attribute id guaranteed
-    // to be set false only while mLock and mSnapshotLock are both held.
-    private static final AtomicBoolean sSnapshotInvalid = new AtomicBoolean(true);
-
-    static final ThreadLocal<ThreadComputer> sThreadComputer =
-            ThreadLocal.withInitial(ThreadComputer::new);
 
     /**
-     * This lock is used to make reads from {@link #sSnapshotInvalid} and
+     * This lock is used to make reads from {@link #sSnapshotPendingVersion} and
-     * {@link #mSnapshotComputer} atomic inside {@code snapshotComputer()}.  This lock is
+     * {@link #sSnapshot} atomic inside {@code snapshotComputer()} when the versions mismatch.
-     * not meant to be used outside that method.  This lock must be taken before
+     * This lock is not meant to be used outside that method. This lock must be taken before
      * {@link #mLock} is taken.
      */
     private final Object mSnapshotLock = new Object();
@@ -1077,48 +1070,53 @@ public class PackageManagerService implements PackageSender, TestUtilityService
             // yet invalidated the snapshot.  Always give the thread the live computer.
             return mLiveComputer;
         }
-        synchronized (mSnapshotLock) {
-            // This synchronization block serializes access to the snapshot computer and
-            // to the code that samples mSnapshotInvalid.
-            Computer c = mSnapshotComputer;
-            if (sSnapshotInvalid.getAndSet(false) || (c == null)) {
-                // The snapshot is invalid if it is marked as invalid or if it is null.  If it
-                // is null, then it is currently being rebuilt by rebuildSnapshot().
-                synchronized (mLock) {
-                    // Rebuild the snapshot if it is invalid.  Note that the snapshot might be
-                    // invalidated as it is rebuilt.  However, the snapshot is still
-                    // self-consistent (the lock is being held) and is current as of the time
-                    // this function is entered.
-                    rebuildSnapshot();
 
-                    // Guaranteed to be non-null.  mSnapshotComputer is only be set to null
+        var oldSnapshot = sSnapshot.get();
-                    // temporarily in rebuildSnapshot(), which is guarded by mLock().  Since
+        var pendingVersion = sSnapshotPendingVersion.get();
-                    // the mLock is held in this block and since rebuildSnapshot() is
+
-                    // complete, the attribute can not now be null.
+        if (oldSnapshot != null && oldSnapshot.getVersion() == pendingVersion) {
-                    c = mSnapshotComputer;
+            return oldSnapshot.use();
-                }
+        }
+
+        synchronized (mSnapshotLock) {
+            // Re-capture pending version in case a new invalidation occurred since last check
+            var rebuildSnapshot = sSnapshot.get();
+            var rebuildVersion = sSnapshotPendingVersion.get();
+
+            // Check the versions again while the lock is held, in case the rebuild time caused
+            // multiple threads to wait on the snapshot lock. When the first thread finishes
+            // a rebuild, the snapshot is now valid and the other waiting threads can use it
+            // without kicking off their own rebuilds.
+            if (rebuildSnapshot != null && rebuildSnapshot.getVersion() == rebuildVersion) {
+                return rebuildSnapshot.use();
+            }
+
+            synchronized (mLock) {
+                // Fetch version one last time to ensure that the rebuilt snapshot matches
+                // the latest invalidation, which could have come in between entering the
+                // SnapshotLock and mLock sync blocks.
+                rebuildVersion = sSnapshotPendingVersion.get();
+
+                // Build the snapshot for this version
+                var newSnapshot = rebuildSnapshot(rebuildSnapshot, rebuildVersion);
+                sSnapshot.set(newSnapshot);
+                return newSnapshot.use();
             }
-            c.use();
-            return c;
         }
     }
 
-    /**
-     * Rebuild the cached computer.  mSnapshotComputer is temporarily set to null to block other
-     * threads from using the invalid computer until it is rebuilt.
-     */
     @GuardedBy({ "mLock", "mSnapshotLock"})
-    private void rebuildSnapshot() {
+    private Computer rebuildSnapshot(@Nullable Computer oldSnapshot, int newVersion) {
-        final long now = SystemClock.currentTimeMicro();
+        var now = SystemClock.currentTimeMicro();
-        final int hits = mSnapshotComputer == null ? -1 : mSnapshotComputer.getUsed();
+        var hits = oldSnapshot == null ? -1 : oldSnapshot.getUsed();
-        mSnapshotComputer = null;
+        var args = new Snapshot(Snapshot.SNAPPED);
-        final Snapshot args = new Snapshot(Snapshot.SNAPPED);
+        var newSnapshot = new ComputerEngine(args, newVersion);
-        mSnapshotComputer = new ComputerEngine(args);
+        var done = SystemClock.currentTimeMicro();
-        final long done = SystemClock.currentTimeMicro();
 
         if (mSnapshotStatistics != null) {
             mSnapshotStatistics.rebuild(now, done, hits);
         }
+        return newSnapshot;
     }
 
     /**
@@ -1138,7 +1136,7 @@ public class PackageManagerService implements PackageSender, TestUtilityService
         if (TRACE_SNAPSHOTS) {
             Log.i(TAG, "snapshot: onChange(" + what + ")");
         }
-        sSnapshotInvalid.set(true);
+        sSnapshotPendingVersion.incrementAndGet();
     }
 
     /**
@@ -1665,7 +1663,6 @@ public class PackageManagerService implements PackageSender, TestUtilityService
         mRequiredSdkSandboxPackage = testParams.requiredSdkSandboxPackage;
 
         mLiveComputer = createLiveComputer();
-        mSnapshotComputer = null;
         mSnapshotStatistics = null;
 
         mPackages.putAll(testParams.packages);
@@ -1855,9 +1852,8 @@ public class PackageManagerService implements PackageSender, TestUtilityService
             // cached computer is the same as the live computer until the end of the
             // constructor, at which time the invalidation method updates it.
             mSnapshotStatistics = new SnapshotStatistics();
-            sSnapshotInvalid.set(true);
+            sSnapshotPendingVersion.incrementAndGet();
             mLiveComputer = createLiveComputer();
-            mSnapshotComputer = null;
             registerObservers(true);
         }
 
@@ -5388,10 +5384,8 @@ public class PackageManagerService implements PackageSender, TestUtilityService
         @Override
         public void setApplicationCategoryHint(String packageName, int categoryHint,
                 String callerPackageName) {
-            final PackageStateMutator.InitialState initialState = recordInitialState();
+            final FunctionalUtils.ThrowingBiFunction<PackageStateMutator.InitialState, Computer,
-
+                    PackageStateMutator.Result> implementation = (initialState, computer) -> {
-            final FunctionalUtils.ThrowingFunction<Computer, PackageStateMutator.Result>
-                    implementation = computer -> {
                 if (computer.getInstantAppPackageName(Binder.getCallingUid()) != null) {
                     throw new SecurityException(
                             "Instant applications don't have access to this method");
@@ -5419,12 +5413,13 @@ public class PackageManagerService implements PackageSender, TestUtilityService
                 }
             };
 
-            PackageStateMutator.Result result = implementation.apply(snapshotComputer());
+            PackageStateMutator.Result result =
+                    implementation.apply(recordInitialState(), snapshotComputer());
             if (result != null && result.isStateChanged() && !result.isSpecificPackageNull()) {
                 // TODO: Specific return value of what state changed?
                 // The installer on record might have changed, retry with lock
                 synchronized (mPackageStateWriteLock) {
-                    result = implementation.apply(snapshotComputer());
+                    result = implementation.apply(recordInitialState(), snapshotComputer());
                 }
             }
 
@@ -7156,9 +7151,19 @@ public class PackageManagerService implements PackageSender, TestUtilityService
     public PackageStateMutator.Result commitPackageStateMutation(
             @Nullable PackageStateMutator.InitialState initialState, @NonNull String packageName,
             @NonNull Consumer<PackageStateWrite> consumer) {
+        PackageStateMutator.Result result = null;
+        if (Thread.holdsLock(mPackageStateWriteLock)) {
+            // If the thread is already holding the lock, this is likely a retry based on a prior
+            // failure, and re-calculating whether a state change occurred can be skipped.
+            result = PackageStateMutator.Result.SUCCESS;
+        }
         synchronized (mPackageStateWriteLock) {
-            final PackageStateMutator.Result result = mPackageStateMutator.generateResult(
+            if (result == null) {
-                    initialState, mChangedPackagesTracker.getSequenceNumber());
+                // If the thread wasn't previously holding, this is a first-try commit and so a
+                // state change may have happened.
+                result = mPackageStateMutator.generateResult(
+                        initialState, mChangedPackagesTracker.getSequenceNumber());
+            }
             if (result != PackageStateMutator.Result.SUCCESS) {
                 return result;
             }

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

@@ -1,52 +0,0 @@
-/*
- * Copyright (C) 2021 The Android Open Source Project
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- *      http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-package com.android.server.pm;
-
-/**
- * This class records the Computer being used by a thread and the Computer's reference
- * count.  There is a thread-local copy of this class.
- */
-public final class ThreadComputer implements AutoCloseable {
-    Computer mComputer = null;
-    int mRefCount = 0;
-
-    void acquire(Computer c) {
-        if (mRefCount != 0 && mComputer != c) {
-            throw new RuntimeException("computer mismatch, count = " + mRefCount);
-        }
-        mComputer = c;
-        mRefCount++;
-    }
-
-    void acquire() {
-        if (mRefCount == 0 || mComputer == null) {
-            throw new RuntimeException("computer acquire on empty ref count");
-        }
-        mRefCount++;
-    }
-
-    void release() {
-        if (--mRefCount == 0) {
-            mComputer = null;
-        }
-    }
-
-    @Override
-    public void close() {
-        release();
-    }
-}