From a578a8db22b41dc96fbdf64651a58320e257f878 Mon Sep 17 00:00:00 2001 From: Martin Stjernholm Date: Fri, 13 Jan 2023 22:41:06 +0000 Subject: [PATCH 1/2] Remove fallbacks from ART Service to legacy dexopt code. ART Service is now feature complete, so the fallbacks serve no purpose. Test: presubmits Bug: 251903639 Change-Id: I49abe7b6a7ea12ca6e5ab6953323515e46da3eae --- .../com/android/server/pm/DexOptHelper.java | 81 +++++-------------- .../android/server/pm/dex/DexoptOptions.java | 38 +++++---- 2 files changed, 44 insertions(+), 75 deletions(-) diff --git a/services/core/java/com/android/server/pm/DexOptHelper.java b/services/core/java/com/android/server/pm/DexOptHelper.java index 923c64dd73aab..4db180d9bb7b0 100644 --- a/services/core/java/com/android/server/pm/DexOptHelper.java +++ b/services/core/java/com/android/server/pm/DexOptHelper.java @@ -82,10 +82,8 @@ import java.util.Collections; import java.util.Comparator; import java.util.HashSet; import java.util.List; -import java.util.Optional; import java.util.Set; import java.util.concurrent.TimeUnit; -import java.util.concurrent.atomic.AtomicLong; import java.util.function.Predicate; /** @@ -413,9 +411,8 @@ public final class DexOptHelper { @DexOptResult int dexoptStatus; if (options.isDexoptOnlySecondaryDex()) { - Optional artSrvRes = performDexOptWithArtService(options, 0 /* extraFlags */); - if (artSrvRes.isPresent()) { - dexoptStatus = artSrvRes.get(); + if (useArtService()) { + dexoptStatus = performDexOptWithArtService(options, 0 /* extraFlags */); } else { try { return mPm.getDexManager().dexoptSecondaryDex(options); @@ -455,10 +452,8 @@ public final class DexOptHelper { // if the package can now be considered up to date for the given filter. @DexOptResult private int performDexOptInternal(DexoptOptions options) { - Optional artSrvRes = - performDexOptWithArtService(options, ArtFlags.FLAG_SHOULD_INCLUDE_DEPENDENCIES); - if (artSrvRes.isPresent()) { - return artSrvRes.get(); + if (useArtService()) { + return performDexOptWithArtService(options, ArtFlags.FLAG_SHOULD_INCLUDE_DEPENDENCIES); } AndroidPackage p; @@ -488,46 +483,26 @@ public final class DexOptHelper { } /** - * Performs dexopt on the given package using ART Service. - * - * @return a {@link DexOptResult}, or empty if the request isn't supported so that it is - * necessary to fall back to the legacy code paths. + * Performs dexopt on the given package using ART Service. May only be called when ART Service + * is enabled, i.e. when {@link useArtService} returns true. */ - private Optional performDexOptWithArtService(DexoptOptions options, + @DexOptResult + private int performDexOptWithArtService(DexoptOptions options, /*@DexoptFlags*/ int extraFlags) { - ArtManagerLocal artManager = getArtManagerLocal(); - if (artManager == null) { - return Optional.empty(); - } - try (PackageManagerLocal.FilteredSnapshot snapshot = getPackageManagerLocal().withFilteredSnapshot()) { PackageState ops = snapshot.getPackageState(options.getPackageName()); if (ops == null) { - return Optional.of(PackageDexOptimizer.DEX_OPT_FAILED); + return PackageDexOptimizer.DEX_OPT_FAILED; } AndroidPackage oap = ops.getAndroidPackage(); if (oap == null) { - return Optional.of(PackageDexOptimizer.DEX_OPT_FAILED); + return PackageDexOptimizer.DEX_OPT_FAILED; } - if (oap.isApex()) { - return Optional.of(PackageDexOptimizer.DEX_OPT_SKIPPED); - } - DexoptParams params = options.convertToDexoptParams(extraFlags); - if (params == null) { - return Optional.empty(); - } - - DexoptResult result; - try { - result = artManager.dexoptPackage(snapshot, options.getPackageName(), params); - } catch (UnsupportedOperationException e) { - reportArtManagerFallback(options.getPackageName(), e.toString()); - return Optional.empty(); - } - - return Optional.of(convertToDexOptResult(result)); + DexoptResult result = + getArtManagerLocal().dexoptPackage(snapshot, options.getPackageName(), params); + return convertToDexOptResult(result); } } @@ -605,14 +580,12 @@ public final class DexOptHelper { getDefaultCompilerFilter(), null /* splitName */, DexoptOptions.DEXOPT_FORCE | DexoptOptions.DEXOPT_BOOT_COMPLETE); - // performDexOptWithArtService ignores the snapshot and takes its own, so it can race with - // the package checks above, but at worst the effect is only a bit less friendly error - // below. - Optional artSrvRes = - performDexOptWithArtService(options, ArtFlags.FLAG_SHOULD_INCLUDE_DEPENDENCIES); - int res; - if (artSrvRes.isPresent()) { - res = artSrvRes.get(); + @DexOptResult int res; + if (useArtService()) { + // performDexOptWithArtService ignores the snapshot and takes its own, so it can race + // with the package checks above, but at worst the effect is only a bit less friendly + // error below. + res = performDexOptWithArtService(options, ArtFlags.FLAG_SHOULD_INCLUDE_DEPENDENCIES); } else { try { res = performDexOptInternalWithDependenciesLI(pkg, packageState, options); @@ -908,15 +881,6 @@ public final class DexOptHelper { return false; } - /** - * Called whenever we need to fall back from ART Service to the legacy dexopt code. - */ - public static void reportArtManagerFallback(String packageName, String reason) { - // STOPSHIP(b/251903639): Minimize these calls to avoid platform getting shipped with code - // paths that will always bypass ART Service. - Slog.i(TAG, "Falling back to old PackageManager dexopt for " + packageName + ": " + reason); - } - /** * Returns true if ART Service should be used for package optimization. */ @@ -1006,12 +970,9 @@ public final class DexOptHelper { } /** - * Returns {@link ArtManagerLocal} if ART Service should be used for package dexopt. + * Returns the registered {@link ArtManagerLocal} instance, or else throws an unchecked error. */ - public static @Nullable ArtManagerLocal getArtManagerLocal() { - if (!useArtService()) { - return null; - } + public static @NonNull ArtManagerLocal getArtManagerLocal() { try { return LocalManagerRegistry.getManagerOrThrow(ArtManagerLocal.class); } catch (ManagerNotFoundException e) { diff --git a/services/core/java/com/android/server/pm/dex/DexoptOptions.java b/services/core/java/com/android/server/pm/dex/DexoptOptions.java index 4fce5e87d1c3f..310c0e8c68fe8 100644 --- a/services/core/java/com/android/server/pm/dex/DexoptOptions.java +++ b/services/core/java/com/android/server/pm/dex/DexoptOptions.java @@ -21,18 +21,19 @@ import static com.android.server.pm.PackageManagerServiceCompilerMapping.getComp import static dalvik.system.DexFile.isProfileGuidedCompilerFilter; import android.annotation.NonNull; -import android.annotation.Nullable; +import android.util.Log; import com.android.server.art.ReasonMapping; import com.android.server.art.model.ArtFlags; import com.android.server.art.model.DexoptParams; -import com.android.server.pm.DexOptHelper; import com.android.server.pm.PackageManagerService; /** * Options used for dexopt invocations. */ public final class DexoptOptions { + private static final String TAG = "DexoptOptions"; + // When set, the profiles will be checked for updates before calling dexopt. If // the apps profiles didn't update in a meaningful way (decided by the compiler), dexopt // will be skipped. @@ -88,8 +89,9 @@ public final class DexoptOptions { // The set of flags for the dexopt options. It's a mix of the DEXOPT_* flags. private final int mFlags; - // When not null, dexopt will optimize only the split identified by this name. - // It only applies for primary apk and it's always null if mOnlySecondaryDex is true. + // When not null, dexopt will optimize only the split identified by this APK file name (not + // split name). It only applies for primary apk and it's always null if mOnlySecondaryDex is + // true. private final String mSplitName; // The reason for invoking dexopt (see PackageManagerService.REASON_* constants). @@ -250,14 +252,20 @@ public final class DexoptOptions { * * @param extraFlags extra {@link ArtFlags#DexoptFlags} to set in the returned * {@code DexoptParams} beyond those converted from this object - * @return null if the settings cannot be accurately represented, and hence the old - * PackageManager/installd code paths need to be used. + * @throws UnsupportedOperationException if the settings cannot be accurately represented. */ - public @Nullable DexoptParams convertToDexoptParams(/*@DexoptFlags*/ int extraFlags) { + public @NonNull DexoptParams convertToDexoptParams(/*@DexoptFlags*/ int extraFlags) { if (mSplitName != null) { - DexOptHelper.reportArtManagerFallback( - mPackageName, "Request to optimize only split " + mSplitName); - return null; + // ART Service supports dexopting a single split - see ArtFlags.FLAG_FOR_SINGLE_SPLIT. + // However using it here requires searching through the splits to find the one matching + // the APK file name in mSplitName, and we don't have the AndroidPackage available for + // that. + // + // Hence we throw here instead, under the assumption that no code paths that dexopt + // splits need this conversion (e.g. shell commands with the --split argument are + // handled by ART Service directly). + throw new UnsupportedOperationException( + "Request to optimize only split " + mSplitName + " for " + mPackageName); } /*@DexoptFlags*/ int flags = extraFlags; @@ -280,11 +288,11 @@ public final class DexoptOptions { flags |= ArtFlags.FLAG_SHOULD_DOWNGRADE; } if ((mFlags & DEXOPT_INSTALL_WITH_DEX_METADATA_FILE) == 0) { - // ART Service cannot be instructed to ignore a DM file if present, so not setting this - // flag is not supported. - DexOptHelper.reportArtManagerFallback( - mPackageName, "DEXOPT_INSTALL_WITH_DEX_METADATA_FILE not set"); - return null; + // ART Service cannot be instructed to ignore a DM file if present. + Log.w(TAG, + "DEXOPT_INSTALL_WITH_DEX_METADATA_FILE not set in request to optimise " + + mPackageName + + " - ART Service will unconditionally use a DM file if present."); } /*@PriorityClassApi*/ int priority; From ff7a8d6b694deedc22272ae9ab35ffd8291caa2f Mon Sep 17 00:00:00 2001 From: Martin Stjernholm Date: Mon, 16 Jan 2023 22:31:25 +0000 Subject: [PATCH 2/2] Call into ART Service for to delete OAT artifacts when it is enabled. PackageManagerService.deleteOatArtifactsOfPackage is used e.g. from AppHibernationService via the deprecated PackageManagerInternalBase.deleteOatArtifactsOfPackage. Test: adb shell cmd app_hibernation set-state --global com.android.egg true with dalvik.vm.useartservice=true and check that ART Service gets called and returns a freed byte count. Bug: 251903639 Change-Id: I7bc01274067abaf1f8de8c0d82020550d23a22f9 --- .../android/server/pm/OtaDexoptService.java | 8 +--- .../server/pm/PackageManagerInternalBase.java | 8 +--- .../server/pm/PackageManagerService.java | 37 +++++++++++++++---- 3 files changed, 32 insertions(+), 21 deletions(-) diff --git a/services/core/java/com/android/server/pm/OtaDexoptService.java b/services/core/java/com/android/server/pm/OtaDexoptService.java index 490b2a954418b..767c0a73bc543 100644 --- a/services/core/java/com/android/server/pm/OtaDexoptService.java +++ b/services/core/java/com/android/server/pm/OtaDexoptService.java @@ -168,13 +168,7 @@ public class OtaDexoptService extends IOtaDexopt.Stub { Log.i(TAG, "Low on space, deleting oat files in an attempt to free up space: " + DexOptHelper.packagesToString(others)); for (PackageStateInternal pkg : others) { - // TODO(b/251903639): Call into ART Service. - try { - mPackageManagerService.deleteOatArtifactsOfPackage( - snapshot, pkg.getPackageName()); - } catch (LegacyDexoptDisabledException e) { - throw new RuntimeException(e); - } + mPackageManagerService.deleteOatArtifactsOfPackage(snapshot, pkg.getPackageName()); } } long spaceAvailableNow = getAvailableSpace(); diff --git a/services/core/java/com/android/server/pm/PackageManagerInternalBase.java b/services/core/java/com/android/server/pm/PackageManagerInternalBase.java index 99fff720221e5..4e7521070132b 100644 --- a/services/core/java/com/android/server/pm/PackageManagerInternalBase.java +++ b/services/core/java/com/android/server/pm/PackageManagerInternalBase.java @@ -47,7 +47,6 @@ import android.util.ArrayMap; import android.util.ArraySet; import android.util.SparseArray; -import com.android.server.pm.Installer.LegacyDexoptDisabledException; import com.android.server.pm.dex.DexManager; import com.android.server.pm.permission.PermissionManagerServiceInternal; import com.android.server.pm.pkg.AndroidPackage; @@ -708,12 +707,7 @@ abstract class PackageManagerInternalBase extends PackageManagerInternal { @Override @Deprecated public final long deleteOatArtifactsOfPackage(String packageName) { - // TODO(b/251903639): Call into ART Service. - try { - return mService.deleteOatArtifactsOfPackage(snapshot(), packageName); - } catch (LegacyDexoptDisabledException e) { - throw new RuntimeException(e); - } + return mService.deleteOatArtifactsOfPackage(snapshot(), packageName); } @Override diff --git a/services/core/java/com/android/server/pm/PackageManagerService.java b/services/core/java/com/android/server/pm/PackageManagerService.java index f316d21ba2a44..0a4e5f18bf341 100644 --- a/services/core/java/com/android/server/pm/PackageManagerService.java +++ b/services/core/java/com/android/server/pm/PackageManagerService.java @@ -196,6 +196,7 @@ import com.android.server.SystemConfig; import com.android.server.Watchdog; import com.android.server.apphibernation.AppHibernationManagerInternal; import com.android.server.art.DexUseManagerLocal; +import com.android.server.art.model.DeleteResult; import com.android.server.compat.CompatChange; import com.android.server.compat.PlatformCompat; import com.android.server.pm.Installer.InstallerException; @@ -7106,17 +7107,39 @@ public class PackageManagerService implements PackageSender, TestUtilityService return AndroidPackageUtils.canHaveOatDir(packageState, packageState.getPkg()); } - long deleteOatArtifactsOfPackage(@NonNull Computer snapshot, String packageName) - throws LegacyDexoptDisabledException { + long deleteOatArtifactsOfPackage(@NonNull Computer snapshot, String packageName) { PackageManagerServiceUtils.enforceSystemOrRootOrShell( "Only the system or shell can delete oat artifacts"); - PackageStateInternal packageState = snapshot.getPackageStateInternal(packageName); - if (packageState == null || packageState.getPkg() == null) { - return -1; // error code of deleteOptimizedFiles + if (DexOptHelper.useArtService()) { + // TODO(chiuwinson): Retrieve filtered snapshot from Computer instance instead. + try (PackageManagerLocal.FilteredSnapshot filteredSnapshot = + PackageManagerServiceUtils.getPackageManagerLocal() + .withFilteredSnapshot()) { + try { + DeleteResult res = DexOptHelper.getArtManagerLocal().deleteDexoptArtifacts( + filteredSnapshot, packageName); + return res.getFreedBytes(); + } catch (IllegalArgumentException e) { + Log.e(TAG, e.toString()); + return -1; + } catch (IllegalStateException e) { + Slog.wtfStack(TAG, e.toString()); + return -1; + } + } + } else { + PackageStateInternal packageState = snapshot.getPackageStateInternal(packageName); + if (packageState == null || packageState.getPkg() == null) { + return -1; // error code of deleteOptimizedFiles + } + try { + return mDexManager.deleteOptimizedFiles( + ArtUtils.createArtPackageInfo(packageState.getPkg(), packageState)); + } catch (LegacyDexoptDisabledException e) { + throw new RuntimeException(e); + } } - return mDexManager.deleteOptimizedFiles( - ArtUtils.createArtPackageInfo(packageState.getPkg(), packageState)); } List getMimeGroupInternal(@NonNull Computer snapshot, String packageName,