From a578a8db22b41dc96fbdf64651a58320e257f878 Mon Sep 17 00:00:00 2001 From: Martin Stjernholm Date: Fri, 13 Jan 2023 22:41:06 +0000 Subject: [PATCH] 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;