From 9cb1e9a7c3c8b3d84a1be783ed55d2bebe4c3dce Mon Sep 17 00:00:00 2001 From: Pinyao Ting Date: Thu, 14 Apr 2022 22:08:19 +0000 Subject: [PATCH] Use more fine-grained lock when persisting shortcuts to disk Currently we are holding service-level lock whenever a process talks to ShortcutService via any of the public apis, which unnecessarily decreases the overall throughput. At the time ShortcutService was implemented, all shortcuts from various packages were persisted into a single xml file on a write-through basis. This was the main reason a single synchoronization lock was required since two parallel updates might corrupt the xml file. Since that shortcuts from different packages are moved into separate xml files, in this CL we: 1. Only write the shortcuts from individual packages into corresponding xml file after there's a change to the shortcuts in that package, as opposed to writing every shortcuts from every packages under that user into corresponding xml files, which was rather inefficient. 2. The write operation is still scheduled as a background job, but it no longer holds the service-level lock. Instead it would only hold package level lock since it is only writing into the xml from that specific package. Given that file I/O is the most time-consuming task in ShortcutService, refrain from holding service-level lock when performing these type of tasks should help overall system throughput. Bug: 186011943 Test: atest CtsShortcutManagerTestCases Change-Id: I773f4c1d7a39e26030a78ee57f29bf0f33848614 --- .../android/server/pm/ShortcutLauncher.java | 10 ++++ .../android/server/pm/ShortcutPackage.java | 38 +++++++------- .../server/pm/ShortcutPackageItem.java | 49 +++++++++++++++++-- .../android/server/pm/ShortcutService.java | 32 +++++------- .../com/android/server/pm/ShortcutUser.java | 27 +--------- .../server/pm/BaseShortcutManagerTest.java | 6 +-- .../server/pm/ShortcutManagerTest1.java | 20 ++++---- 7 files changed, 102 insertions(+), 80 deletions(-) diff --git a/services/core/java/com/android/server/pm/ShortcutLauncher.java b/services/core/java/com/android/server/pm/ShortcutLauncher.java index 2960bc9a3790c..c0c2349532978 100644 --- a/services/core/java/com/android/server/pm/ShortcutLauncher.java +++ b/services/core/java/com/android/server/pm/ShortcutLauncher.java @@ -420,4 +420,14 @@ class ShortcutLauncher extends ShortcutPackageItem { ArraySet getAllPinnedShortcutsForTest(String packageName, int packageUserId) { return new ArraySet<>(mPinnedShortcuts.get(PackageWithUser.of(packageUserId, packageName))); } + + @Override + protected File getShortcutPackageItemFile() { + final File path = new File(mShortcutUser.mService.injectUserDataPath( + mShortcutUser.getUserId()), ShortcutUser.DIRECTORY_LUANCHERS); + // Package user id and owner id can have different values for ShortcutLaunchers. Adding + // user Id to the file name to create a unique path. Owner id is used in the root path. + final String fileName = getPackageName() + getPackageUserId() + ".xml"; + return new File(path, fileName); + } } diff --git a/services/core/java/com/android/server/pm/ShortcutPackage.java b/services/core/java/com/android/server/pm/ShortcutPackage.java index b3723fb61f5a3..0200d954bef16 100644 --- a/services/core/java/com/android/server/pm/ShortcutPackage.java +++ b/services/core/java/com/android/server/pm/ShortcutPackage.java @@ -160,8 +160,6 @@ class ShortcutPackage extends ShortcutPackageItem { private static final String KEY_BITMAPS = "bitmaps"; private static final String KEY_BITMAP_BYTES = "bitmapBytes"; - private final Object mLock = new Object(); - private final Executor mExecutor; /** @@ -779,7 +777,7 @@ class ShortcutPackage extends ShortcutPackageItem { return false; } mApiCallCount++; - s.scheduleSaveUser(getOwnerUserId()); + scheduleSave(); return true; } @@ -789,7 +787,7 @@ class ShortcutPackage extends ShortcutPackageItem { } if (mApiCallCount > 0) { mApiCallCount = 0; - mShortcutUser.mService.scheduleSaveUser(getOwnerUserId()); + scheduleSave(); } } @@ -1886,15 +1884,12 @@ class ShortcutPackage extends ShortcutPackageItem { final ShortcutPackage ret = new ShortcutPackage(shortcutUser, shortcutUser.getUserId(), packageName); - synchronized (ret.mLock) { ret.mIsAppSearchSchemaUpToDate = ShortcutService.parseIntAttribute( parser, ATTR_SCHEMA_VERSON, 0) == AppSearchShortcutInfo.SCHEMA_VERSION; } - ret.mApiCallCount = - ShortcutService.parseIntAttribute(parser, ATTR_CALL_COUNT); - ret.mLastResetTime = - ShortcutService.parseLongAttribute(parser, ATTR_LAST_RESET); + ret.mApiCallCount = ShortcutService.parseIntAttribute(parser, ATTR_CALL_COUNT); + ret.mLastResetTime = ShortcutService.parseLongAttribute(parser, ATTR_LAST_RESET); final int outerDepth = parser.getDepth(); @@ -2436,16 +2431,15 @@ class ShortcutPackage extends ShortcutPackageItem { }))); } - void persistsAllShortcutsAsync() { - synchronized (mLock) { - final Map copy = mShortcuts; - if (!mTransientShortcuts.isEmpty()) { - copy.putAll(mTransientShortcuts); - mTransientShortcuts.clear(); - } - saveShortcutsAsync(copy.values().stream().filter(ShortcutInfo::usesQuota).collect( - Collectors.toList())); + @Override + void scheduleSaveToAppSearchLocked() { + final Map copy = new ArrayMap<>(mShortcuts); + if (!mTransientShortcuts.isEmpty()) { + copy.putAll(mTransientShortcuts); + mTransientShortcuts.clear(); } + saveShortcutsAsync(copy.values().stream().filter(ShortcutInfo::usesQuota).collect( + Collectors.toList())); } private void saveShortcutsAsync( @@ -2544,4 +2538,12 @@ class ShortcutPackage extends ShortcutPackageItem { Binder.restoreCallingIdentity(callingIdentity); } } + + @Override + protected File getShortcutPackageItemFile() { + final File path = new File(mShortcutUser.mService.injectUserDataPath( + mShortcutUser.getUserId()), ShortcutUser.DIRECTORY_PACKAGES); + final String fileName = getPackageName() + ".xml"; + return new File(path, fileName); + } } diff --git a/services/core/java/com/android/server/pm/ShortcutPackageItem.java b/services/core/java/com/android/server/pm/ShortcutPackageItem.java index 829133c9854ae..6e0436f208e33 100644 --- a/services/core/java/com/android/server/pm/ShortcutPackageItem.java +++ b/services/core/java/com/android/server/pm/ShortcutPackageItem.java @@ -23,6 +23,7 @@ import android.util.Slog; import android.util.TypedXmlSerializer; import android.util.Xml; +import com.android.internal.annotations.GuardedBy; import com.android.internal.util.Preconditions; import org.json.JSONException; @@ -36,7 +37,7 @@ import java.nio.charset.StandardCharsets; import java.util.Objects; /** - * All methods should be guarded by {@code #mShortcutUser.mService.mLock}. + * All methods should be either guarded by {@code #mShortcutUser.mService.mLock} or {@code #mLock}. */ abstract class ShortcutPackageItem { private static final String TAG = ShortcutService.TAG; @@ -49,6 +50,8 @@ abstract class ShortcutPackageItem { protected ShortcutUser mShortcutUser; + protected final Object mLock = new Object(); + protected ShortcutPackageItem(@NonNull ShortcutUser shortcutUser, int packageUserId, @NonNull String packageName, @NonNull ShortcutPackageInfo packageInfo) { @@ -98,7 +101,7 @@ abstract class ShortcutPackageItem { } final ShortcutService s = mShortcutUser.mService; mPackageInfo.refreshSignature(s, this); - s.scheduleSaveUser(getOwnerUserId()); + scheduleSave(); } public void attemptToRestoreIfNeededAndSave() { @@ -138,7 +141,7 @@ abstract class ShortcutPackageItem { // Either way, it's no longer a shadow. mPackageInfo.setShadow(false); - s.scheduleSaveUser(mPackageUserId); + scheduleSave(); } protected abstract boolean canRestoreAnyVersion(); @@ -148,7 +151,8 @@ abstract class ShortcutPackageItem { public abstract void saveToXml(@NonNull TypedXmlSerializer out, boolean forBackup) throws IOException, XmlPullParserException; - public void saveToFile(File path, boolean forBackup) { + @GuardedBy("mLock") + public void saveToFileLocked(File path, boolean forBackup) { final AtomicFile file = new AtomicFile(path); FileOutputStream os = null; try { @@ -176,6 +180,11 @@ abstract class ShortcutPackageItem { } } + @GuardedBy("mLock") + void scheduleSaveToAppSearchLocked() { + + } + public JSONObject dumpCheckin(boolean clear) throws JSONException { final JSONObject result = new JSONObject(); result.put(KEY_NAME, mPackageName); @@ -187,4 +196,36 @@ abstract class ShortcutPackageItem { */ public void verifyStates() { } + + public void scheduleSave() { + mShortcutUser.mService.injectPostToHandlerDebounced( + mSaveShortcutPackageRunner, mSaveShortcutPackageRunner); + } + + private final Runnable mSaveShortcutPackageRunner = this::saveShortcutPackageItem; + + void saveShortcutPackageItem() { + // Wait for bitmap saves to conclude before proceeding to saving shortcuts. + mShortcutUser.mService.waitForBitmapSaves(); + // Save each ShortcutPackageItem in a separate Xml file. + final File path = getShortcutPackageItemFile(); + if (ShortcutService.DEBUG || ShortcutService.DEBUG_REBOOT) { + Slog.d(TAG, "Saving package item " + getPackageName() + " to " + path); + } + synchronized (mLock) { + path.getParentFile().mkdirs(); + // TODO: Since we are persisting shortcuts into AppSearch, we should read from/write to + // AppSearch as opposed to maintaining a separate XML file. + saveToFileLocked(path, false /*forBackup*/); + scheduleSaveToAppSearchLocked(); + } + } + + void removeShortcutPackageItem() { + synchronized (mLock) { + getShortcutPackageItemFile().delete(); + } + } + + protected abstract File getShortcutPackageItemFile(); } diff --git a/services/core/java/com/android/server/pm/ShortcutService.java b/services/core/java/com/android/server/pm/ShortcutService.java index 9627c4394db74..780f976d2a400 100644 --- a/services/core/java/com/android/server/pm/ShortcutService.java +++ b/services/core/java/com/android/server/pm/ShortcutService.java @@ -748,7 +748,7 @@ public class ShortcutService extends IShortcutService.Stub { getUserShortcutsLocked(userId).cancelAllInFlightTasks(); // Save all dirty information. - saveDirtyInfo(false); + saveDirtyInfo(); // Unload mUsers.delete(userId); @@ -1203,10 +1203,6 @@ public class ShortcutService extends IShortcutService.Stub { @VisibleForTesting void saveDirtyInfo() { - saveDirtyInfo(true); - } - - private void saveDirtyInfo(boolean saveShortcutsInAppSearch) { if (DEBUG || DEBUG_REBOOT) { Slog.d(TAG, "saveDirtyInfo"); } @@ -1221,10 +1217,6 @@ public class ShortcutService extends IShortcutService.Stub { if (userId == UserHandle.USER_NULL) { // USER_NULL for base state. saveBaseStateLocked(); } else { - if (saveShortcutsInAppSearch) { - getUserShortcutsLocked(userId).forAllPackages( - ShortcutPackage::persistsAllShortcutsAsync); - } saveUserLocked(userId); } } @@ -1816,7 +1808,7 @@ public class ShortcutService extends IShortcutService.Stub { } injectPostToHandlerDebounced(sp, notifyListenerRunnable(packageName, userId)); notifyShortcutChangeCallbacks(packageName, userId, changedShortcuts, removedShortcuts); - scheduleSaveUser(userId); + sp.scheduleSave(); } private void notifyListeners(@NonNull final String packageName, @UserIdInt final int userId) { @@ -2878,12 +2870,11 @@ public class ShortcutService extends IShortcutService.Stub { final ShortcutUser user = getUserShortcutsLocked(owningUserId); boolean doNotify = false; - // First, remove the package from the package list (if the package is a publisher). - if (packageUserId == owningUserId) { - if (user.removePackage(packageName) != null) { - doNotify = true; - } + final ShortcutPackage sp = (packageUserId == owningUserId) + ? user.removePackage(packageName) : null; + if (sp != null) { + doNotify = true; } // Also remove from the launcher list (if the package is a launcher). @@ -2906,6 +2897,10 @@ public class ShortcutService extends IShortcutService.Stub { // notifyListeners. user.rescanPackageIfNeeded(packageName, /* forceRescan=*/ true); } + if (!appStillExists && (packageUserId == owningUserId) && sp != null) { + // If the app is removed altogether, we can get rid of the xml as well + injectPostToHandler(() -> sp.removeShortcutPackageItem()); + } if (!wasUserLoaded) { // Note this will execute the scheduled save. @@ -3788,7 +3783,7 @@ public class ShortcutService extends IShortcutService.Stub { if (mHandler.hasCallbacks(mSaveDirtyInfoRunner)) { mHandler.removeCallbacks(mSaveDirtyInfoRunner); forEachLoadedUserLocked(ShortcutUser::cancelAllInFlightTasks); - saveDirtyInfo(false); + saveDirtyInfo(); } mShutdown.set(true); } @@ -4457,7 +4452,7 @@ public class ShortcutService extends IShortcutService.Stub { // Save to the filesystem. scheduleSaveUser(userId); - saveDirtyInfo(false); + saveDirtyInfo(); // Note, in case of backup, we don't have to wait on bitmap saving, because we don't // back up bitmaps anyway. @@ -5352,8 +5347,7 @@ public class ShortcutService extends IShortcutService.Stub { } } - @VisibleForTesting - void waitForBitmapSavesForTest() { + void waitForBitmapSaves() { synchronized (mLock) { mShortcutBitmapSaver.waitForAllSavesLocked(); } diff --git a/services/core/java/com/android/server/pm/ShortcutUser.java b/services/core/java/com/android/server/pm/ShortcutUser.java index 4bb5dcfa4b269..75e18b547c553 100644 --- a/services/core/java/com/android/server/pm/ShortcutUser.java +++ b/services/core/java/com/android/server/pm/ShortcutUser.java @@ -407,35 +407,10 @@ class ShortcutUser { } spi.saveToXml(out, forBackup); } else { - // Save each ShortcutPackageItem in a separate Xml file. - final File path = getShortcutPackageItemFile(spi); - if (ShortcutService.DEBUG || ShortcutService.DEBUG_REBOOT) { - Slog.d(TAG, "Saving package item " + spi.getPackageName() + " to " + path); - } - - path.getParentFile().mkdirs(); - spi.saveToFile(path, forBackup); + spi.saveShortcutPackageItem(); } } - private File getShortcutPackageItemFile(ShortcutPackageItem spi) { - boolean isShortcutLauncher = spi instanceof ShortcutLauncher; - - final File path = new File(mService.injectUserDataPath(mUserId), - isShortcutLauncher ? DIRECTORY_LUANCHERS : DIRECTORY_PACKAGES); - - final String fileName; - if (isShortcutLauncher) { - // Package user id and owner id can have different values for ShortcutLaunchers. Adding - // user Id to the file name to create a unique path. Owner id is used in the root path. - fileName = spi.getPackageName() + spi.getPackageUserId() + ".xml"; - } else { - fileName = spi.getPackageName() + ".xml"; - } - - return new File(path, fileName); - } - public static ShortcutUser loadFromXml(ShortcutService s, TypedXmlPullParser parser, int userId, boolean fromBackup) throws IOException, XmlPullParserException, InvalidFileFormatException { final ShortcutUser ret = new ShortcutUser(s, userId); diff --git a/services/tests/servicestests/src/com/android/server/pm/BaseShortcutManagerTest.java b/services/tests/servicestests/src/com/android/server/pm/BaseShortcutManagerTest.java index e4ee4d0647243..8961e642013cf 100644 --- a/services/tests/servicestests/src/com/android/server/pm/BaseShortcutManagerTest.java +++ b/services/tests/servicestests/src/com/android/server/pm/BaseShortcutManagerTest.java @@ -1969,7 +1969,7 @@ public abstract class BaseShortcutManagerTest extends InstrumentationTestCase { if (si == null) { return null; } - mService.waitForBitmapSavesForTest(); + mService.waitForBitmapSaves(); return new File(si.getBitmapPath()).getName(); } @@ -1978,7 +1978,7 @@ public abstract class BaseShortcutManagerTest extends InstrumentationTestCase { if (si == null) { return null; } - mService.waitForBitmapSavesForTest(); + mService.waitForBitmapSaves(); return new File(si.getBitmapPath()).getAbsolutePath(); } @@ -2133,7 +2133,7 @@ public abstract class BaseShortcutManagerTest extends InstrumentationTestCase { } protected boolean bitmapDirectoryExists(String packageName, int userId) { - mService.waitForBitmapSavesForTest(); + mService.waitForBitmapSaves(); final File path = new File(mService.getUserBitmapFilePath(userId), packageName); return path.isDirectory(); } diff --git a/services/tests/servicestests/src/com/android/server/pm/ShortcutManagerTest1.java b/services/tests/servicestests/src/com/android/server/pm/ShortcutManagerTest1.java index 867890f938ba6..411b52155abb2 100644 --- a/services/tests/servicestests/src/com/android/server/pm/ShortcutManagerTest1.java +++ b/services/tests/servicestests/src/com/android/server/pm/ShortcutManagerTest1.java @@ -1040,7 +1040,7 @@ public class ShortcutManagerTest1 extends BaseShortcutManagerTest { dumpsysOnLogcat(); - mService.waitForBitmapSavesForTest(); + mService.waitForBitmapSaves(); // Check files and directories. // Package 3 has no bitmaps, so we don't create a directory. assertBitmapDirectories(USER_0, CALLING_PACKAGE_1, CALLING_PACKAGE_2); @@ -1096,7 +1096,7 @@ public class ShortcutManagerTest1 extends BaseShortcutManagerTest { makeFile(mService.getUserBitmapFilePath(USER_10), CALLING_PACKAGE_2, "3").createNewFile(); makeFile(mService.getUserBitmapFilePath(USER_10), CALLING_PACKAGE_2, "4").createNewFile(); - mService.waitForBitmapSavesForTest(); + mService.waitForBitmapSaves(); assertBitmapDirectories(USER_0, CALLING_PACKAGE_1, CALLING_PACKAGE_2, CALLING_PACKAGE_3, "a.b.c", "d.e.f"); @@ -1111,7 +1111,7 @@ public class ShortcutManagerTest1 extends BaseShortcutManagerTest { // The below check is the same as above, except this time USER_0 use the CALLING_PACKAGE_3 // directory. - mService.waitForBitmapSavesForTest(); + mService.waitForBitmapSaves(); assertBitmapDirectories(USER_0, CALLING_PACKAGE_1, CALLING_PACKAGE_2, CALLING_PACKAGE_3); assertBitmapDirectories(USER_10, CALLING_PACKAGE_1, CALLING_PACKAGE_2); @@ -1390,7 +1390,7 @@ public class ShortcutManagerTest1 extends BaseShortcutManagerTest { .setIcon(Icon.createWithContentUri("test_uri")) .build() ))); - mService.waitForBitmapSavesForTest(); + mService.waitForBitmapSaves(); assertWith(getCallerShortcuts()) .forShortcutWithId("s1", si -> { assertTrue(si.hasIconUri()); @@ -1402,13 +1402,13 @@ public class ShortcutManagerTest1 extends BaseShortcutManagerTest { .setIcon(Icon.createWithResource(getTestContext(), R.drawable.black_32x32)) .build() ))); - mService.waitForBitmapSavesForTest(); + mService.waitForBitmapSaves(); assertWith(getCallerShortcuts()) .forShortcutWithId("s1", si -> { assertTrue(si.hasIconResource()); assertEquals(R.drawable.black_32x32, si.getIconResourceId()); }); - mService.waitForBitmapSavesForTest(); + mService.waitForBitmapSaves(); mInjectedCurrentTimeMillis += INTERVAL; // reset throttling @@ -1419,7 +1419,7 @@ public class ShortcutManagerTest1 extends BaseShortcutManagerTest { getTestContext().getResources(), R.drawable.black_64x64))) .build() ))); - mService.waitForBitmapSavesForTest(); + mService.waitForBitmapSaves(); assertWith(getCallerShortcuts()) .forShortcutWithId("s1", si -> { assertTrue(si.hasIconFile()); @@ -1437,7 +1437,7 @@ public class ShortcutManagerTest1 extends BaseShortcutManagerTest { getTestContext().getResources(), R.drawable.black_64x64))) .build() ))); - mService.waitForBitmapSavesForTest(); + mService.waitForBitmapSaves(); assertWith(getCallerShortcuts()) .forShortcutWithId("s1", si -> { assertTrue(si.hasIconFile()); @@ -1451,7 +1451,7 @@ public class ShortcutManagerTest1 extends BaseShortcutManagerTest { .setIcon(Icon.createWithResource(getTestContext(), R.drawable.black_32x32)) .build() ))); - mService.waitForBitmapSavesForTest(); + mService.waitForBitmapSaves(); assertWith(getCallerShortcuts()) .forShortcutWithId("s1", si -> { assertTrue(si.hasIconResource()); @@ -1463,7 +1463,7 @@ public class ShortcutManagerTest1 extends BaseShortcutManagerTest { .setIcon(Icon.createWithContentUri("test_uri")) .build() ))); - mService.waitForBitmapSavesForTest(); + mService.waitForBitmapSaves(); assertWith(getCallerShortcuts()) .forShortcutWithId("s1", si -> { assertTrue(si.hasIconUri());