diff options
author | Android Build Coastguard Worker <android-build-coastguard-worker@google.com> | 2023-12-05 11:48:29 +0000 |
---|---|---|
committer | Android Build Coastguard Worker <android-build-coastguard-worker@google.com> | 2023-12-05 12:30:08 +0000 |
commit | 3b072364164657d70436cbec60d1892a64ea73ad (patch) | |
tree | 08a5dca6632bc5a4a108566c4474b1c4c67082a6 | |
parent | fed9fd2e5d6165ed8e490d59913f06e02c11a53c (diff) | |
parent | 3414d01c7f3606bd700a9bff22df47d6d53c8b6a (diff) | |
download | art-3b072364164657d70436cbec60d1892a64ea73ad.tar.gz |
Make change and version bump to aml_art_341411000 for mainline module file: build/apex/manifest-art.json
Snap for 11178562 from 3414d01c7f3606bd700a9bff22df47d6d53c8b6a to mainline-art-release
Change-Id: I8e02668551c11753d9e6f34fb49f2bc6c0e0927e
34 files changed, 779 insertions, 210 deletions
diff --git a/TEST_MAPPING b/TEST_MAPPING index d9d5431e3e..084f3d7386 100644 --- a/TEST_MAPPING +++ b/TEST_MAPPING @@ -455,9 +455,6 @@ "name": "art-run-test-2244-checker-remove-try-boundary[com.google.android.art.apex]" }, { - "name": "art-run-test-2247-checker-write-barrier-elimination[com.google.android.art.apex]" - }, - { "name": "art-run-test-2249-checker-return-try-boundary-exit-in-loop[com.google.android.art.apex]" }, { @@ -1880,9 +1877,6 @@ "name": "art-run-test-2244-checker-remove-try-boundary" }, { - "name": "art-run-test-2247-checker-write-barrier-elimination" - }, - { "name": "art-run-test-2249-checker-return-try-boundary-exit-in-loop" }, { @@ -3289,9 +3283,6 @@ "name": "art-run-test-2244-checker-remove-try-boundary" }, { - "name": "art-run-test-2247-checker-write-barrier-elimination" - }, - { "name": "art-run-test-2249-checker-return-try-boundary-exit-in-loop" }, { diff --git a/artd/artd.cc b/artd/artd.cc index 6132a22eab..04a01a9ca4 100644 --- a/artd/artd.cc +++ b/artd/artd.cc @@ -719,10 +719,9 @@ ndk::ScopedAStatus Artd::mergeProfiles(const std::vector<ProfilePath>& in_profil OR_RETURN_NON_FATAL(NewFile::Create(output_profile_path, in_outputProfile->fsPermission)); if (in_referenceProfile.has_value()) { - if (in_options.forceMerge || in_options.dumpOnly || in_options.dumpClassesAndMethods) { + if (in_options.dumpOnly || in_options.dumpClassesAndMethods) { return Fatal( - "Reference profile must not be set when 'forceMerge', 'dumpOnly', or " - "'dumpClassesAndMethods' is set"); + "Reference profile must not be set when 'dumpOnly' or 'dumpClassesAndMethods' is set"); } std::string reference_profile_path = OR_RETURN_FATAL(BuildProfileOrDmPath(*in_referenceProfile)); @@ -755,7 +754,7 @@ ndk::ScopedAStatus Artd::mergeProfiles(const std::vector<ProfilePath>& in_profil props_->GetOrEmpty("dalvik.vm.bgdexopt.new-classes-percent")) .AddIfNonEmpty("--min-new-methods-percent-change=%s", props_->GetOrEmpty("dalvik.vm.bgdexopt.new-methods-percent")) - .AddIf(in_options.forceMerge, "--force-merge") + .AddIf(in_options.forceMerge, "--force-merge-and-analyze") .AddIf(in_options.forBootImage, "--boot-image-merge"); } @@ -778,9 +777,8 @@ ndk::ScopedAStatus Artd::mergeProfiles(const std::vector<ProfilePath>& in_profil } ProfmanResult::ProcessingResult expected_result = - (in_options.forceMerge || in_options.dumpOnly || in_options.dumpClassesAndMethods) ? - ProfmanResult::kSuccess : - ProfmanResult::kCompile; + (in_options.dumpOnly || in_options.dumpClassesAndMethods) ? ProfmanResult::kSuccess : + ProfmanResult::kCompile; if (result.value() != expected_result) { return NonFatal(ART_FORMAT("profman returned an unexpected code: {}", result.value())); } diff --git a/artd/artd_test.cc b/artd/artd_test.cc index 5b6c5b2767..f1806e1c2d 100644 --- a/artd/artd_test.cc +++ b/artd/artd_test.cc @@ -1756,7 +1756,7 @@ TEST_F(ArtdTest, mergeProfiles) { Contains(Flag("--reference-profile-file-fd=", FdHasContent("abc"))), Contains(Flag("--apk-fd=", FdOf(dex_file_1))), Contains(Flag("--apk-fd=", FdOf(dex_file_2))), - Not(Contains("--force-merge")), + Not(Contains("--force-merge-and-analyze")), Not(Contains("--boot-image-merge")))), HasKeepFdsFor("--profile-file-fd=", "--reference-profile-file-fd=", "--apk-fd=")), _, @@ -1872,13 +1872,14 @@ TEST_F(ArtdTest, mergeProfilesWithOptionsForceMerge) { CreateFile(dex_file_); - EXPECT_CALL( - *mock_exec_utils_, - DoExecAndReturnCode( - WhenSplitBy("--", _, AllOf(Contains("--force-merge"), Contains("--boot-image-merge"))), - _, - _)) - .WillOnce(Return(ProfmanResult::kSuccess)); + EXPECT_CALL(*mock_exec_utils_, + DoExecAndReturnCode(WhenSplitBy("--", + _, + AllOf(Contains("--force-merge-and-analyze"), + Contains("--boot-image-merge"))), + _, + _)) + .WillOnce(Return(ProfmanResult::kCompile)); bool result; EXPECT_TRUE(artd_ diff --git a/artd/binder/com/android/server/art/MergeProfileOptions.aidl b/artd/binder/com/android/server/art/MergeProfileOptions.aidl index 2d007f9406..5d3591ccf2 100644 --- a/artd/binder/com/android/server/art/MergeProfileOptions.aidl +++ b/artd/binder/com/android/server/art/MergeProfileOptions.aidl @@ -28,7 +28,7 @@ package com.android.server.art; * @hide */ parcelable MergeProfileOptions { - /** --force-merge */ + /** --force-merge-and-analyze */ boolean forceMerge; /** --boot-image-merge */ boolean forBootImage; diff --git a/build/apex/manifest-art.json b/build/apex/manifest-art.json index e4cd2310b7..6e37a9c302 100644 --- a/build/apex/manifest-art.json +++ b/build/apex/manifest-art.json @@ -3,7 +3,7 @@ // Placeholder module version to be replaced during build. // Do not change! - "version": 341312000, + "version": 341411000, "provideNativeLibs": [ "libjdwp.so" diff --git a/compiler/optimizing/write_barrier_elimination.cc b/compiler/optimizing/write_barrier_elimination.cc index eb70b670fe..6182125b74 100644 --- a/compiler/optimizing/write_barrier_elimination.cc +++ b/compiler/optimizing/write_barrier_elimination.cc @@ -21,6 +21,9 @@ #include "base/scoped_arena_containers.h" #include "optimizing/nodes.h" +// TODO(b/310755375, solanes): Disable WBE while we investigate crashes. +constexpr bool kWBEEnabled = false; + namespace art HIDDEN { class WBEVisitor final : public HGraphVisitor { @@ -153,8 +156,10 @@ class WBEVisitor final : public HGraphVisitor { }; bool WriteBarrierElimination::Run() { - WBEVisitor wbe_visitor(graph_, stats_); - wbe_visitor.VisitReversePostOrder(); + if (kWBEEnabled) { + WBEVisitor wbe_visitor(graph_, stats_); + wbe_visitor.VisitReversePostOrder(); + } return true; } diff --git a/libartservice/service/java/com/android/server/art/ArtManagerLocal.java b/libartservice/service/java/com/android/server/art/ArtManagerLocal.java index d04962c58f..f8d6b5643e 100644 --- a/libartservice/service/java/com/android/server/art/ArtManagerLocal.java +++ b/libartservice/service/java/com/android/server/art/ArtManagerLocal.java @@ -84,8 +84,10 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.Comparator; +import java.util.HashMap; import java.util.HashSet; import java.util.List; +import java.util.Map; import java.util.Objects; import java.util.Set; import java.util.concurrent.CompletableFuture; @@ -456,7 +458,9 @@ public final class ArtManagerLocal { * @param reason determines the default list of packages and options * @param cancellationSignal provides the ability to cancel this operation * @param processCallbackExecutor the executor to call {@code progressCallback} - * @param progressCallback called repeatedly whenever there is an update on the progress + * @param progressCallbacks a mapping from an integer, in {@link ArtFlags.BatchDexoptPass}, to + * the callback that is called repeatedly whenever there is an update on the progress + * @return a mapping from an integer, in {@link ArtFlags.BatchDexoptPass}, to the dexopt result. * @throws IllegalStateException if the operation encounters an error that should never happen * (e.g., an internal logic error), or the callback set by {@link * #setBatchDexoptStartCallback(Executor, BatchDexoptStartCallback)} provides invalid @@ -466,11 +470,12 @@ public final class ArtManagerLocal { */ @RequiresApi(Build.VERSION_CODES.UPSIDE_DOWN_CAKE) @NonNull - public DexoptResult dexoptPackages(@NonNull PackageManagerLocal.FilteredSnapshot snapshot, + public Map<Integer, DexoptResult> dexoptPackages( + @NonNull PackageManagerLocal.FilteredSnapshot snapshot, @NonNull @BatchDexoptReason String reason, @NonNull CancellationSignal cancellationSignal, @Nullable @CallbackExecutor Executor progressCallbackExecutor, - @Nullable Consumer<OperationProgress> progressCallback) { + @Nullable Map<Integer, Consumer<OperationProgress>> progressCallbacks) { List<String> defaultPackages = Collections.unmodifiableList(getDefaultPackages(snapshot, reason)); DexoptParams defaultDexoptParams = new DexoptParams.Builder(reason).build(); @@ -488,16 +493,37 @@ public final class ArtManagerLocal { ExecutorService dexoptExecutor = Executors.newFixedThreadPool(ReasonMapping.getConcurrencyForReason(reason)); + Map<Integer, DexoptResult> dexoptResults = new HashMap<>(); try { if (reason.equals(ReasonMapping.REASON_BG_DEXOPT)) { - maybeDowngradePackages(snapshot, + DexoptResult downgradeResult = maybeDowngradePackages(snapshot, new HashSet<>(params.getPackages()) /* excludedPackages */, - cancellationSignal, dexoptExecutor); + cancellationSignal, dexoptExecutor, progressCallbackExecutor, + progressCallbacks != null ? progressCallbacks.get(ArtFlags.PASS_DOWNGRADE) + : null); + if (downgradeResult != null) { + dexoptResults.put(ArtFlags.PASS_DOWNGRADE, downgradeResult); + } + } + Log.i(TAG, + "Dexopting " + params.getPackages().size() + " packages with reason=" + reason); + DexoptResult mainResult = mInjector.getDexoptHelper().dexopt(snapshot, + params.getPackages(), params.getDexoptParams(), cancellationSignal, + dexoptExecutor, progressCallbackExecutor, + progressCallbacks != null ? progressCallbacks.get(ArtFlags.PASS_MAIN) : null); + dexoptResults.put(ArtFlags.PASS_MAIN, mainResult); + if (reason.equals(ReasonMapping.REASON_BG_DEXOPT)) { + DexoptResult supplementaryResult = maybeDexoptPackagesSupplementaryPass(snapshot, + mainResult, params.getDexoptParams(), cancellationSignal, dexoptExecutor, + progressCallbackExecutor, + progressCallbacks != null + ? progressCallbacks.get(ArtFlags.PASS_SUPPLEMENTARY) + : null); + if (supplementaryResult != null) { + dexoptResults.put(ArtFlags.PASS_SUPPLEMENTARY, supplementaryResult); + } } - Log.i(TAG, "Dexopting packages"); - return mInjector.getDexoptHelper().dexopt(snapshot, params.getPackages(), - params.getDexoptParams(), cancellationSignal, dexoptExecutor, - progressCallbackExecutor, progressCallback); + return dexoptResults; } finally { dexoptExecutor.shutdown(); } @@ -857,7 +883,7 @@ public final class ArtManagerLocal { @Nullable Consumer<OperationProgress> progressCallback) { try (var snapshot = mInjector.getPackageManagerLocal().withFilteredSnapshot()) { dexoptPackages(snapshot, bootReason, new CancellationSignal(), progressCallbackExecutor, - progressCallback); + progressCallback != null ? Map.of(ArtFlags.PASS_MAIN, progressCallback) : null); } } @@ -1009,26 +1035,31 @@ public final class ArtManagerLocal { } @RequiresApi(Build.VERSION_CODES.UPSIDE_DOWN_CAKE) - private void maybeDowngradePackages(@NonNull PackageManagerLocal.FilteredSnapshot snapshot, + @Nullable + private DexoptResult maybeDowngradePackages( + @NonNull PackageManagerLocal.FilteredSnapshot snapshot, @NonNull Set<String> excludedPackages, @NonNull CancellationSignal cancellationSignal, - @NonNull Executor executor) { + @NonNull Executor executor, + @Nullable @CallbackExecutor Executor progressCallbackExecutor, + @Nullable Consumer<OperationProgress> progressCallback) { if (shouldDowngrade()) { List<String> packages = getDefaultPackages(snapshot, ReasonMapping.REASON_INACTIVE) .stream() .filter(pkg -> !excludedPackages.contains(pkg)) .collect(Collectors.toList()); if (!packages.isEmpty()) { - Log.i(TAG, "Storage is low. Downgrading inactive packages"); + Log.i(TAG, "Storage is low. Downgrading " + packages.size() + " inactive packages"); DexoptParams params = new DexoptParams.Builder(ReasonMapping.REASON_INACTIVE).build(); - mInjector.getDexoptHelper().dexopt(snapshot, packages, params, cancellationSignal, - executor, null /* processCallbackExecutor */, null /* progressCallback */); + return mInjector.getDexoptHelper().dexopt(snapshot, packages, params, + cancellationSignal, executor, progressCallbackExecutor, progressCallback); } else { Log.i(TAG, "Storage is low, but downgrading is disabled or there's nothing to " + "downgrade"); } } + return null; } @RequiresApi(Build.VERSION_CODES.UPSIDE_DOWN_CAKE) @@ -1042,6 +1073,48 @@ public final class ArtManagerLocal { } } + @RequiresApi(Build.VERSION_CODES.UPSIDE_DOWN_CAKE) + @Nullable + private DexoptResult maybeDexoptPackagesSupplementaryPass( + @NonNull PackageManagerLocal.FilteredSnapshot snapshot, + @NonNull DexoptResult mainResult, @NonNull DexoptParams mainParams, + @NonNull CancellationSignal cancellationSignal, @NonNull Executor dexoptExecutor, + @Nullable @CallbackExecutor Executor progressCallbackExecutor, + @Nullable Consumer<OperationProgress> progressCallback) { + if ((mainParams.getFlags() & ArtFlags.FLAG_FORCE_MERGE_PROFILE) != 0) { + return null; + } + + // Only pick packages that used a profile-guided filter and were skipped in the main pass. + // This is a very coarse filter to reduce unnecessary iterations on a best-effort basis. + // Packages included in the list may still be skipped by dexopter if the profiles don't have + // any change. + List<String> packageNames = + mainResult.getPackageDexoptResults() + .stream() + .filter(packageResult + -> packageResult.getDexContainerFileDexoptResults() + .stream() + .anyMatch(fileResult + -> DexFile.isProfileGuidedCompilerFilter( + fileResult.getActualCompilerFilter()) + && fileResult.getStatus() + == DexoptResult.DEXOPT_SKIPPED)) + .map(packageResult -> packageResult.getPackageName()) + .collect(Collectors.toList()); + + DexoptParams dexoptParams = mainParams.toBuilder() + .setFlags(ArtFlags.FLAG_FORCE_MERGE_PROFILE, + ArtFlags.FLAG_FORCE_MERGE_PROFILE) + .build(); + + Log.i(TAG, + "Dexopting " + packageNames.size() + " packages with reason=" + + dexoptParams.getReason() + " (supplementary pass)"); + return mInjector.getDexoptHelper().dexopt(snapshot, packageNames, dexoptParams, + cancellationSignal, dexoptExecutor, progressCallbackExecutor, progressCallback); + } + /** Returns the list of packages to process for the given reason. */ @RequiresApi(Build.VERSION_CODES.UPSIDE_DOWN_CAKE) @NonNull diff --git a/libartservice/service/java/com/android/server/art/ArtShellCommand.java b/libartservice/service/java/com/android/server/art/ArtShellCommand.java index b83f4ab25d..f9e610c124 100644 --- a/libartservice/service/java/com/android/server/art/ArtShellCommand.java +++ b/libartservice/service/java/com/android/server/art/ArtShellCommand.java @@ -20,6 +20,8 @@ import static android.os.ParcelFileDescriptor.AutoCloseInputStream; import static com.android.server.art.ArtManagerLocal.SnapshotProfileException; import static com.android.server.art.PrimaryDexUtils.PrimaryDexInfo; +import static com.android.server.art.ReasonMapping.BatchDexoptReason; +import static com.android.server.art.model.ArtFlags.BatchDexoptPass; import static com.android.server.art.model.ArtFlags.DexoptFlags; import static com.android.server.art.model.ArtFlags.PriorityClassApi; import static com.android.server.art.model.DexoptResult.DexContainerFileDexoptResult; @@ -66,11 +68,14 @@ import java.nio.file.Paths; import java.util.ArrayList; import java.util.HashMap; import java.util.List; +import java.util.Locale; import java.util.Map; import java.util.UUID; import java.util.concurrent.CompletableFuture; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; +import java.util.function.Consumer; +import java.util.function.Function; import java.util.stream.Collectors; /** @@ -195,6 +200,7 @@ public final class ArtShellCommand extends BasicShellCommandHandler { boolean forAllPackages = false; boolean legacyClearProfile = false; boolean verbose = false; + boolean forceMergeProfile = false; String opt; while ((opt = getNextOption()) != null) { @@ -259,6 +265,9 @@ public final class ArtShellCommand extends BasicShellCommandHandler { case "-v": verbose = true; break; + case "--force-merge-profile": + forceMergeProfile = true; + break; default: pw.println("Error: Unknown option: " + opt); return 1; @@ -290,6 +299,10 @@ public final class ArtShellCommand extends BasicShellCommandHandler { if (force) { paramsBuilder.setFlags(ArtFlags.FLAG_FORCE, ArtFlags.FLAG_FORCE); } + if (forceMergeProfile) { + paramsBuilder.setFlags( + ArtFlags.FLAG_FORCE_MERGE_PROFILE, ArtFlags.FLAG_FORCE_MERGE_PROFILE); + } if (splitArg != null) { if (scopeFlags != 0) { pw.println("Error: '--primary-dex', '--secondary-dex', " @@ -382,8 +395,7 @@ public final class ArtShellCommand extends BasicShellCommandHandler { BackgroundDexoptJob.Result result = Utils.getFuture(future); if (result instanceof BackgroundDexoptJob.CompletedResult) { var completedResult = (BackgroundDexoptJob.CompletedResult) result; - if (completedResult.dexoptResult().getFinalStatus() - == DexoptResult.DEXOPT_CANCELLED) { + if (completedResult.isCancelled()) { pw.println("Job cancelled. See logs for details"); } else { pw.println("Job finished. See logs for details"); @@ -584,20 +596,41 @@ public final class ArtShellCommand extends BasicShellCommandHandler { ReasonMapping.BATCH_DEXOPT_REASONS); return 1; } - DexoptResult result; + + final String finalReason = reason; + + // Create callbacks to print the progress. + Map<Integer, Consumer<OperationProgress>> progressCallbacks = new HashMap<>(); + for (@BatchDexoptPass int pass : ArtFlags.BATCH_DEXOPT_PASSES) { + progressCallbacks.put(pass, progress -> { + pw.println(String.format(Locale.US, "%s: %d%%", + getProgressMessageForBatchDexoptPass(pass, finalReason), + progress.getPercentage())); + pw.flush(); + }); + } + ExecutorService progressCallbackExecutor = Executors.newSingleThreadExecutor(); try (var signal = new WithCancellationSignal(pw, true /* verbose */)) { - result = mArtManagerLocal.dexoptPackages( - snapshot, reason, signal.get(), progressCallbackExecutor, progress -> { - pw.println(String.format("Dexopting apps: %d%%", progress.getPercentage())); - pw.flush(); - }); + Map<Integer, DexoptResult> results = mArtManagerLocal.dexoptPackages(snapshot, + finalReason, signal.get(), progressCallbackExecutor, progressCallbacks); + Utils.executeAndWait(progressCallbackExecutor, () -> { - printDexoptResult(pw, result, true /* verbose */, true /* multiPackage */); + for (@BatchDexoptPass int pass : ArtFlags.BATCH_DEXOPT_PASSES) { + if (results.containsKey(pass)) { + pw.println("Result of " + + getProgressMessageForBatchDexoptPass(pass, finalReason) + .toLowerCase(Locale.US) + + ":"); + printDexoptResult( + pw, results.get(pass), true /* verbose */, true /* multiPackage */); + } + } }); } finally { progressCallbackExecutor.shutdown(); } + return 0; } @@ -642,6 +675,8 @@ public final class ArtShellCommand extends BasicShellCommandHandler { pw.println(" For secondary dex files, it also clears all dexopt artifacts."); pw.println(" When this flag is set, all the other flags are ignored."); pw.println(" -v Verbose mode. This mode prints detailed results."); + pw.println(" --force-merge-profile Force merge profiles even if the difference between"); + pw.println(" before and after the merge is not significant."); pw.println(" Scope options:"); pw.println(" --primary-dex Dexopt primary dex files only (all APKs that are installed"); pw.println(" as part of the package, including the base APK and all other split"); @@ -906,6 +941,21 @@ public final class ArtShellCommand extends BasicShellCommandHandler { } } + @NonNull + private String getProgressMessageForBatchDexoptPass( + @BatchDexoptPass int pass, @NonNull @BatchDexoptReason String reason) { + switch (pass) { + case ArtFlags.PASS_DOWNGRADE: + return "Downgrading apps"; + case ArtFlags.PASS_MAIN: + return reason.equals(ReasonMapping.REASON_BG_DEXOPT) ? "Dexopting apps (main pass)" + : "Dexopting apps"; + case ArtFlags.PASS_SUPPLEMENTARY: + return "Dexopting apps (supplementary pass)"; + } + throw new IllegalArgumentException("Unknown batch dexopt pass " + pass); + } + private static class WithCancellationSignal implements AutoCloseable { @NonNull private final CancellationSignal mSignal = new CancellationSignal(); @NonNull private final String mJobId; diff --git a/libartservice/service/java/com/android/server/art/BackgroundDexoptJob.java b/libartservice/service/java/com/android/server/art/BackgroundDexoptJob.java index 9944e3fb1f..46e3a9f77c 100644 --- a/libartservice/service/java/com/android/server/art/BackgroundDexoptJob.java +++ b/libartservice/service/java/com/android/server/art/BackgroundDexoptJob.java @@ -17,6 +17,7 @@ package com.android.server.art; import static com.android.server.art.ArtManagerLocal.ScheduleBackgroundDexoptJobCallback; +import static com.android.server.art.model.ArtFlags.BatchDexoptPass; import static com.android.server.art.model.ArtFlags.ScheduleStatus; import static com.android.server.art.model.Config.Callback; @@ -32,6 +33,7 @@ import android.os.CancellationSignal; import android.os.SystemClock; import android.os.SystemProperties; import android.util.Log; +import android.util.Slog; import androidx.annotation.RequiresApi; @@ -41,14 +43,19 @@ import com.android.server.LocalManagerRegistry; import com.android.server.art.model.ArtFlags; import com.android.server.art.model.Config; import com.android.server.art.model.DexoptResult; +import com.android.server.art.model.OperationProgress; import com.android.server.pm.PackageManagerLocal; import com.google.auto.value.AutoValue; +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; import java.util.Objects; import java.util.Optional; import java.util.concurrent.CompletableFuture; import java.util.concurrent.TimeUnit; +import java.util.function.Consumer; /** @hide */ @RequiresApi(Build.VERSION_CODES.UPSIDE_DOWN_CAKE) @@ -85,15 +92,20 @@ public class BackgroundDexoptJob { public boolean onStartJob( @NonNull BackgroundDexoptJobService jobService, @NonNull JobParameters params) { start().thenAcceptAsync(result -> { - writeStats(result); + try { + writeStats(result); + } catch (RuntimeException e) { + // Not expected. Log wtf to surface it. + Slog.wtf(TAG, "Failed to write stats", e); + } + // This is a periodic job, where the interval is specified in the `JobInfo`. "true" // means to execute again during a future idle maintenance window in the same // interval, while "false" means not to execute again during a future idle maintenance // window in the same interval but to execute again in the next interval. // This call will be ignored if `onStopJob` is called. - boolean wantsReschedule = result instanceof CompletedResult - && ((CompletedResult) result).dexoptResult().getFinalStatus() - == DexoptResult.DEXOPT_CANCELLED; + boolean wantsReschedule = + result instanceof CompletedResult && ((CompletedResult) result).isCancelled(); jobService.jobFinished(params, wantsReschedule); }); // "true" means the job will continue running until `jobFinished` is called. @@ -202,12 +214,28 @@ public class BackgroundDexoptJob { @NonNull private CompletedResult run(@NonNull CancellationSignal cancellationSignal) { - long startTimeMs = SystemClock.uptimeMillis(); - DexoptResult dexoptResult; + // Create callbacks to time each pass. + Map<Integer, Long> startTimeMsByPass = new HashMap<>(); + Map<Integer, Long> durationMsByPass = new HashMap<>(); + Map<Integer, Consumer<OperationProgress>> progressCallbacks = new HashMap<>(); + for (@BatchDexoptPass int pass : ArtFlags.BATCH_DEXOPT_PASSES) { + progressCallbacks.put(pass, progress -> { + if (progress.getTotal() == 0) { + durationMsByPass.put(pass, 0l); + } else if (progress.getCurrent() == 0) { + startTimeMsByPass.put(pass, SystemClock.uptimeMillis()); + } else if (progress.getCurrent() == progress.getTotal()) { + durationMsByPass.put( + pass, SystemClock.uptimeMillis() - startTimeMsByPass.get(pass)); + } + }); + } + + Map<Integer, DexoptResult> dexoptResultByPass; try (var snapshot = mInjector.getPackageManagerLocal().withFilteredSnapshot()) { - dexoptResult = mInjector.getArtManagerLocal().dexoptPackages(snapshot, - ReasonMapping.REASON_BG_DEXOPT, cancellationSignal, - null /* processCallbackExecutor */, null /* processCallback */); + dexoptResultByPass = mInjector.getArtManagerLocal().dexoptPackages(snapshot, + ReasonMapping.REASON_BG_DEXOPT, cancellationSignal, Runnable::run, + progressCallbacks); // For simplicity, we don't support cancelling the following operation in the middle. // This is fine because it typically takes only a few seconds. @@ -220,7 +248,7 @@ public class BackgroundDexoptJob { Log.i(TAG, String.format("Freed %d bytes", freedBytes)); } } - return CompletedResult.create(dexoptResult, SystemClock.uptimeMillis() - startTimeMs); + return CompletedResult.create(dexoptResultByPass, durationMsByPass); } private void writeStats(@NonNull Result result) { @@ -239,13 +267,22 @@ public class BackgroundDexoptJob { static class FatalErrorResult extends Result {} @AutoValue + @SuppressWarnings("AutoValueImmutableFields") // Can't use ImmutableMap because it's in Guava. static abstract class CompletedResult extends Result { - abstract @NonNull DexoptResult dexoptResult(); - abstract long durationMs(); + abstract @NonNull Map<Integer, DexoptResult> dexoptResultByPass(); + abstract @NonNull Map<Integer, Long> durationMsByPass(); @NonNull - static CompletedResult create(@NonNull DexoptResult dexoptResult, long durationMs) { - return new AutoValue_BackgroundDexoptJob_CompletedResult(dexoptResult, durationMs); + static CompletedResult create(@NonNull Map<Integer, DexoptResult> dexoptResultByPass, + @NonNull Map<Integer, Long> durationMsByPass) { + return new AutoValue_BackgroundDexoptJob_CompletedResult( + Collections.unmodifiableMap(dexoptResultByPass), + Collections.unmodifiableMap(durationMsByPass)); + } + + public boolean isCancelled() { + return dexoptResultByPass().values().stream().anyMatch( + result -> result.getFinalStatus() == DexoptResult.DEXOPT_CANCELLED); } } diff --git a/libartservice/service/java/com/android/server/art/BackgroundDexoptJobStatsReporter.java b/libartservice/service/java/com/android/server/art/BackgroundDexoptJobStatsReporter.java index c6cc73af19..dc98914292 100644 --- a/libartservice/service/java/com/android/server/art/BackgroundDexoptJobStatsReporter.java +++ b/libartservice/service/java/com/android/server/art/BackgroundDexoptJobStatsReporter.java @@ -1,11 +1,14 @@ package com.android.server.art; +import static com.android.server.art.model.ArtFlags.BatchDexoptPass; + import android.annotation.NonNull; import android.app.job.JobParameters; import android.os.Build; import androidx.annotation.RequiresApi; +import com.android.server.art.model.ArtFlags; import com.android.server.art.model.DexoptResult; import dalvik.system.DexFile; @@ -22,31 +25,48 @@ import java.util.stream.Collectors; @RequiresApi(Build.VERSION_CODES.UPSIDE_DOWN_CAKE) public class BackgroundDexoptJobStatsReporter { public static void reportFailure() { + // The fatal error can occur during any pass, but we attribute it to the main pass for + // simplicity. ArtStatsLog.write(ArtStatsLog.BACKGROUND_DEXOPT_JOB_ENDED, ArtStatsLog.BACKGROUND_DEXOPT_JOB_ENDED__STATUS__STATUS_FATAL_ERROR, JobParameters.STOP_REASON_UNDEFINED, 0 /* durationMs */, 0 /* deprecated */, 0 /* optimizedPackagesCount */, 0 /* packagesDependingOnBootClasspathCount */, - 0 /* totalPackagesCount */); + 0 /* totalPackagesCount */, + ArtStatsLog.BACKGROUND_DEXOPT_JOB_ENDED__PASS__PASS_MAIN); } public static void reportSuccess(@NonNull BackgroundDexoptJob.CompletedResult completedResult, Optional<Integer> stopReason) { + for (var entry : completedResult.dexoptResultByPass().entrySet()) { + reportPass(entry.getKey(), entry.getValue(), + completedResult.durationMsByPass().getOrDefault(entry.getKey(), 0l), + stopReason); + } + } + + public static void reportPass(@BatchDexoptPass int pass, @NonNull DexoptResult dexoptResult, + long durationMs, Optional<Integer> stopReason) { + // The job contains multiple passes, so the stop reason may not be for the current pass. We + // shouldn't report the stop reason if the current pass finished before the job was + // cancelled. + int reportedStopReason = dexoptResult.getFinalStatus() == DexoptResult.DEXOPT_CANCELLED + ? stopReason.orElse(JobParameters.STOP_REASON_UNDEFINED) + : JobParameters.STOP_REASON_UNDEFINED; + List<DexoptResult.PackageDexoptResult> packageDexoptResults = - getFilteredPackageResults(completedResult); + getFilteredPackageResults(dexoptResult); + ArtStatsLog.write(ArtStatsLog.BACKGROUND_DEXOPT_JOB_ENDED, - getStatusForStats(completedResult, stopReason), - stopReason.orElse(JobParameters.STOP_REASON_UNDEFINED), - completedResult.durationMs(), 0 /* deprecated */, - getDexoptedPackagesCount(packageDexoptResults), + getStatusForStats(dexoptResult, stopReason), reportedStopReason, durationMs, + 0 /* deprecated */, getDexoptedPackagesCount(packageDexoptResults), getPackagesDependingOnBootClasspathCount(packageDexoptResults), - packageDexoptResults.size()); + packageDexoptResults.size(), toStatsdPassEnum(pass)); } @NonNull private static List<DexoptResult.PackageDexoptResult> getFilteredPackageResults( - @NonNull BackgroundDexoptJob.CompletedResult completedResult) { - return completedResult.dexoptResult() - .getPackageDexoptResults() + @NonNull DexoptResult dexoptResult) { + return dexoptResult.getPackageDexoptResults() .stream() .filter(packageResult -> packageResult.getDexContainerFileDexoptResults().stream().anyMatch( @@ -58,8 +78,8 @@ public class BackgroundDexoptJobStatsReporter { } private static int getStatusForStats( - @NonNull BackgroundDexoptJob.CompletedResult result, Optional<Integer> stopReason) { - if (result.dexoptResult().getFinalStatus() == DexoptResult.DEXOPT_CANCELLED) { + @NonNull DexoptResult dexoptResult, Optional<Integer> stopReason) { + if (dexoptResult.getFinalStatus() == DexoptResult.DEXOPT_CANCELLED) { if (stopReason.isPresent()) { return ArtStatsLog .BACKGROUND_DEXOPT_JOB_ENDED__STATUS__STATUS_ABORT_BY_CANCELLATION; @@ -69,8 +89,7 @@ public class BackgroundDexoptJobStatsReporter { } boolean isSkippedDueToStorageLow = - result.dexoptResult() - .getPackageDexoptResults() + dexoptResult.getPackageDexoptResults() .stream() .flatMap(packageResult -> packageResult.getDexContainerFileDexoptResults().stream()) @@ -106,4 +125,16 @@ public class BackgroundDexoptJobStatsReporter { .map(DexoptResult.DexContainerFileDexoptResult::getActualCompilerFilter) .anyMatch(DexFile::isOptimizedCompilerFilter); } + + private static int toStatsdPassEnum(@BatchDexoptPass int pass) { + switch (pass) { + case ArtFlags.PASS_DOWNGRADE: + return ArtStatsLog.BACKGROUND_DEXOPT_JOB_ENDED__PASS__PASS_DOWNGRADE; + case ArtFlags.PASS_MAIN: + return ArtStatsLog.BACKGROUND_DEXOPT_JOB_ENDED__PASS__PASS_MAIN; + case ArtFlags.PASS_SUPPLEMENTARY: + return ArtStatsLog.BACKGROUND_DEXOPT_JOB_ENDED__PASS__PASS_SUPPLEMENTARY; + } + throw new IllegalArgumentException("Unknown batch dexopt pass " + pass); + } } diff --git a/libartservice/service/java/com/android/server/art/Dexopter.java b/libartservice/service/java/com/android/server/art/Dexopter.java index 5420c44689..2f15166ce7 100644 --- a/libartservice/service/java/com/android/server/art/Dexopter.java +++ b/libartservice/service/java/com/android/server/art/Dexopter.java @@ -541,9 +541,12 @@ public abstract class Dexopter<DexInfoType extends DetailedDexInfo> { @Nullable ProfilePath referenceProfile) throws RemoteException { OutputProfile output = buildOutputProfile(dexInfo, false /* isPublic */); + var options = new MergeProfileOptions(); + options.forceMerge = (mParams.getFlags() & ArtFlags.FLAG_FORCE_MERGE_PROFILE) != 0; + try { if (mInjector.getArtd().mergeProfiles(getCurProfiles(dexInfo), referenceProfile, output, - List.of(dexInfo.dexPath()), new MergeProfileOptions())) { + List.of(dexInfo.dexPath()), options)) { return ProfilePath.tmpProfilePath(output.profilePath); } } catch (ServiceSpecificException e) { diff --git a/libartservice/service/java/com/android/server/art/model/ArtFlags.java b/libartservice/service/java/com/android/server/art/model/ArtFlags.java index cfb021fc08..6de14f0f11 100644 --- a/libartservice/service/java/com/android/server/art/model/ArtFlags.java +++ b/libartservice/service/java/com/android/server/art/model/ArtFlags.java @@ -29,6 +29,7 @@ import com.android.server.pm.PackageManagerLocal; import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; +import java.util.List; /** @hide */ @SystemApi(client = SystemApi.Client.SYSTEM_SERVER) @@ -89,6 +90,13 @@ public class ArtFlags { */ @SuppressLint("UnflaggedApi") // Flag support for mainline is not available. public static final int FLAG_IGNORE_PROFILE = 1 << 7; + /** + * Whether to force merge profiles even if the difference between before and after the merge + * is not significant. + * + * @hide + */ + public static final int FLAG_FORCE_MERGE_PROFILE = 1 << 8; /** * Flags for {@link @@ -130,6 +138,7 @@ public class ArtFlags { FLAG_FOR_SINGLE_SPLIT, FLAG_SKIP_IF_STORAGE_LOW, FLAG_IGNORE_PROFILE, + FLAG_FORCE_MERGE_PROFILE, }) // clang-format on @Retention(RetentionPolicy.SOURCE) @@ -230,5 +239,51 @@ public class ArtFlags { @Retention(RetentionPolicy.SOURCE) public @interface ScheduleStatus {} + /** + * The downgrade pass, run before the main pass. Only applicable to bg-dexopt. + * + * @hide + */ + public static final int PASS_DOWNGRADE = 0; + + /** + * The main pass. + * + * @hide + */ + public static final int PASS_MAIN = 1; + + /** + * The supplementary pass, run after the main pass, to take the opportunity to dexopt more + * packages. Compared to the main pass, it uses different criteria to determine whether dexopt + * is needed or not, but iterates over the same packages in the same order as the main pass (so + * the logic in {@link ArtManagerLocal#getDefaultPackages} and {@link + * ArtManagerLocal.BatchDexoptStartCallback} controls the packages here too.) + * + * Only applicable to bg-dexopt. + * + * @hide + */ + public static final int PASS_SUPPLEMENTARY = 2; + + /** + * Indicates the pass of a batch dexopt run. + * + * @hide + */ + // clang-format off + @IntDef(prefix = "PASS_", value = { + PASS_DOWNGRADE, + PASS_MAIN, + PASS_SUPPLEMENTARY, + }) + // clang-format on + @Retention(RetentionPolicy.SOURCE) + public @interface BatchDexoptPass {} + + /** @hide */ + public static final List<Integer> BATCH_DEXOPT_PASSES = + List.of(PASS_DOWNGRADE, PASS_MAIN, PASS_SUPPLEMENTARY); + private ArtFlags() {} } diff --git a/libartservice/service/java/com/android/server/art/model/DexoptParams.java b/libartservice/service/java/com/android/server/art/model/DexoptParams.java index 45616d536d..455d4cdead 100644 --- a/libartservice/service/java/com/android/server/art/model/DexoptParams.java +++ b/libartservice/service/java/com/android/server/art/model/DexoptParams.java @@ -223,4 +223,12 @@ public class DexoptParams { public @Nullable String getSplitName() { return mSplitName; } + + /** @hide */ + public @NonNull Builder toBuilder() { + return new Builder(mReason, mFlags) + .setCompilerFilter(mCompilerFilter) + .setPriorityClass(mPriorityClass) + .setSplitName(mSplitName); + } } diff --git a/libartservice/service/java/com/android/server/art/model/DexoptResult.java b/libartservice/service/java/com/android/server/art/model/DexoptResult.java index 901eef9076..64cdb1ca78 100644 --- a/libartservice/service/java/com/android/server/art/model/DexoptResult.java +++ b/libartservice/service/java/com/android/server/art/model/DexoptResult.java @@ -113,6 +113,13 @@ public abstract class DexoptResult { return new AutoValue_DexoptResult(requestedCompilerFilter, reason, packageDexoptResult); } + /** @hide */ + @VisibleForTesting + public static @NonNull DexoptResult create() { + return new AutoValue_DexoptResult( + "compiler-filter", "reason", List.of() /* packageDexoptResult */); + } + /** * The requested compiler filter. Note that the compiler filter might be adjusted before the * execution based on factors like dexopt flags, whether the profile is available, or whether diff --git a/libartservice/service/java/com/android/server/art/model/OperationProgress.java b/libartservice/service/java/com/android/server/art/model/OperationProgress.java index a47a556bdc..a0a7ddc4fc 100644 --- a/libartservice/service/java/com/android/server/art/model/OperationProgress.java +++ b/libartservice/service/java/com/android/server/art/model/OperationProgress.java @@ -38,7 +38,7 @@ public abstract class OperationProgress { /** The overall progress, in the range of [0, 100]. */ public int getPercentage() { - return 100 * getCurrent() / getTotal(); + return getTotal() == 0 ? 100 : 100 * getCurrent() / getTotal(); } /** diff --git a/libartservice/service/javatests/com/android/server/art/ArtManagerLocalTest.java b/libartservice/service/javatests/com/android/server/art/ArtManagerLocalTest.java index 5a153e4564..5e3e7b9cd7 100644 --- a/libartservice/service/javatests/com/android/server/art/ArtManagerLocalTest.java +++ b/libartservice/service/javatests/com/android/server/art/ArtManagerLocalTest.java @@ -19,6 +19,8 @@ package com.android.server.art; import static android.os.ParcelFileDescriptor.AutoCloseInputStream; import static com.android.server.art.DexUseManagerLocal.DetailedSecondaryDexInfo; +import static com.android.server.art.model.DexoptResult.DexContainerFileDexoptResult; +import static com.android.server.art.model.DexoptResult.PackageDexoptResult; import static com.android.server.art.model.DexoptStatus.DexContainerFileDexoptStatus; import static com.android.server.art.testing.TestingUtils.deepEq; import static com.android.server.art.testing.TestingUtils.inAnyOrder; @@ -58,6 +60,7 @@ import android.os.storage.StorageManager; import androidx.test.filters.SmallTest; import com.android.modules.utils.pm.PackageStateModulesUtils; +import com.android.server.art.model.ArtFlags; import com.android.server.art.model.Config; import com.android.server.art.model.DeleteResult; import com.android.server.art.model.DexoptParams; @@ -90,6 +93,7 @@ import java.nio.file.Path; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.concurrent.Executor; import java.util.concurrent.ForkJoinPool; import java.util.concurrent.TimeUnit; import java.util.function.Consumer; @@ -408,7 +412,7 @@ public class ArtManagerLocalTest { @Test public void testDexoptPackage() throws Exception { var params = new DexoptParams.Builder("install").build(); - var result = mock(DexoptResult.class); + var result = DexoptResult.create(); var cancellationSignal = new CancellationSignal(); when(mDexoptHelper.dexopt(any(), deepEq(List.of(PKG_NAME_1)), same(params), @@ -422,7 +426,7 @@ public class ArtManagerLocalTest { @Test public void testResetDexoptStatus() throws Exception { - var result = mock(DexoptResult.class); + var result = DexoptResult.create(); var cancellationSignal = new CancellationSignal(); when(mDexoptHelper.dexopt( @@ -459,7 +463,7 @@ public class ArtManagerLocalTest { @Test public void testDexoptPackages() throws Exception { - var dexoptResult = mock(DexoptResult.class); + var dexoptResult = DexoptResult.create(); var cancellationSignal = new CancellationSignal(); when(mDexUseManager.getPackageLastUsedAtMs(PKG_NAME_2)).thenReturn(CURRENT_TIME_MS); simulateStorageLow(); @@ -474,7 +478,7 @@ public class ArtManagerLocalTest { assertThat(mArtManagerLocal.dexoptPackages(mSnapshot, "bg-dexopt", cancellationSignal, null /* processCallbackExecutor */, null /* processCallback */)) - .isSameInstanceAs(dexoptResult); + .isEqualTo(Map.of(ArtFlags.PASS_MAIN, dexoptResult)); // Nothing to downgrade. verify(mDexoptHelper, never()) @@ -490,7 +494,7 @@ public class ArtManagerLocalTest { when(mDexUseManager.getPackageLastUsedAtMs(PKG_NAME_1)).thenReturn(0l); simulateStorageLow(); - var result = mock(DexoptResult.class); + var result = DexoptResult.create(); var cancellationSignal = new CancellationSignal(); // PKG_NAME_1 should be dexopted. @@ -517,25 +521,28 @@ public class ArtManagerLocalTest { when(mDexUseManager.getPackageLastUsedAtMs(PKG_NAME_1)).thenReturn(NOT_RECENT_TIME_MS); simulateStorageLow(); - var result = mock(DexoptResult.class); + var mainResult = DexoptResult.create(); + var downgradeResult = DexoptResult.create(); var cancellationSignal = new CancellationSignal(); // PKG_NAME_1 should not be dexopted. - doReturn(result) + doReturn(mainResult) .when(mDexoptHelper) .dexopt(any(), deepEq(List.of(PKG_NAME_2)), argThat(params -> params.getReason().equals("bg-dexopt")), any(), any(), any(), any()); // PKG_NAME_1 should be downgraded. - doReturn(result) + doReturn(downgradeResult) .when(mDexoptHelper) .dexopt(any(), deepEq(List.of(PKG_NAME_1)), argThat(params -> params.getReason().equals("inactive")), any(), any(), any(), any()); - mArtManagerLocal.dexoptPackages(mSnapshot, "bg-dexopt", cancellationSignal, - null /* processCallbackExecutor */, null /* processCallback */); + assertThat(mArtManagerLocal.dexoptPackages(mSnapshot, "bg-dexopt", cancellationSignal, + null /* processCallbackExecutor */, null /* processCallback */)) + .isEqualTo(Map.of( + ArtFlags.PASS_DOWNGRADE, downgradeResult, ArtFlags.PASS_MAIN, mainResult)); } @Test @@ -545,7 +552,7 @@ public class ArtManagerLocalTest { when(userState.getFirstInstallTimeMillis()).thenReturn(NOT_RECENT_TIME_MS); when(mDexUseManager.getPackageLastUsedAtMs(PKG_NAME_1)).thenReturn(NOT_RECENT_TIME_MS); - var result = mock(DexoptResult.class); + var result = DexoptResult.create(); var cancellationSignal = new CancellationSignal(); // PKG_NAME_1 should not be dexopted. @@ -566,7 +573,7 @@ public class ArtManagerLocalTest { @Test public void testDexoptPackagesBootAfterMainlineUpdate() throws Exception { - var result = mock(DexoptResult.class); + var result = DexoptResult.create(); var cancellationSignal = new CancellationSignal(); lenient().when(mInjector.isSystemUiPackage(PKG_NAME_1)).thenReturn(true); @@ -583,7 +590,7 @@ public class ArtManagerLocalTest { @Test public void testDexoptPackagesBootAfterMainlineUpdatePackagesNotFound() throws Exception { - var result = mock(DexoptResult.class); + var result = DexoptResult.create(); var cancellationSignal = new CancellationSignal(); // PKG_NAME_1 is neither recently installed nor recently used. PackageUserState userState = mPkgState1.getStateForUser(UserHandle.of(1)); @@ -615,7 +622,7 @@ public class ArtManagerLocalTest { simulateStorageLow(); var params = new DexoptParams.Builder("bg-dexopt").build(); - var result = mock(DexoptResult.class); + var result = DexoptResult.create(); var cancellationSignal = new CancellationSignal(); mArtManagerLocal.setBatchDexoptStartCallback(ForkJoinPool.commonPool(), @@ -645,7 +652,7 @@ public class ArtManagerLocalTest { @Test public void testDexoptPackagesOverrideCleared() throws Exception { var params = new DexoptParams.Builder("bg-dexopt").build(); - var result = mock(DexoptResult.class); + var result = DexoptResult.create(); var cancellationSignal = new CancellationSignal(); mArtManagerLocal.setBatchDexoptStartCallback(ForkJoinPool.commonPool(), @@ -661,7 +668,56 @@ public class ArtManagerLocalTest { assertThat(mArtManagerLocal.dexoptPackages(mSnapshot, "bg-dexopt", cancellationSignal, null /* processCallbackExecutor */, null /* processCallback */)) - .isSameInstanceAs(result); + .isEqualTo(Map.of(ArtFlags.PASS_MAIN, result)); + } + + @Test + public void testDexoptPackagesSupplementaryPass() throws Exception { + // The supplementary pass should only try dexopting PKG_NAME_2. + var mainResult = DexoptResult.create("speed-profile", "bg-dexopt", + List.of(PackageDexoptResult.create(PKG_NAME_1, + List.of(DexContainerFileDexoptResult.create("dex-file-1", + true /* isPrimaryAbi */, "arm64", "speed-profile", + DexoptResult.DEXOPT_PERFORMED), + DexContainerFileDexoptResult.create("dex-file-2", + true /* isPrimaryAbi */, "arm64", "speed", + DexoptResult.DEXOPT_SKIPPED)), + null /* packageLevelStatus */), + PackageDexoptResult.create(PKG_NAME_2, + List.of(DexContainerFileDexoptResult.create("dex-file-1", + true /* isPrimaryAbi */, "arm64", "speed-profile", + DexoptResult.DEXOPT_PERFORMED), + DexContainerFileDexoptResult.create("dex-file-2", + true /* isPrimaryAbi */, "arm64", "speed-profile", + DexoptResult.DEXOPT_SKIPPED)), + null /* packageLevelStatus */))); + var supplementaryResult = DexoptResult.create(); + var cancellationSignal = new CancellationSignal(); + + // The main pass. + doReturn(mainResult) + .when(mDexoptHelper) + .dexopt(any(), inAnyOrder(PKG_NAME_1, PKG_NAME_2), + argThat(params + -> params.getReason().equals("bg-dexopt") + && (params.getFlags() & ArtFlags.FLAG_FORCE_MERGE_PROFILE) + == 0), + any(), any(), any(), any()); + + // The supplementary pass. + doReturn(supplementaryResult) + .when(mDexoptHelper) + .dexopt(any(), inAnyOrder(PKG_NAME_2), + argThat(params + -> params.getReason().equals("bg-dexopt") + && (params.getFlags() & ArtFlags.FLAG_FORCE_MERGE_PROFILE) + != 0), + any(), any(), any(), any()); + + assertThat(mArtManagerLocal.dexoptPackages(mSnapshot, "bg-dexopt", cancellationSignal, + null /* processCallbackExecutor */, null /* processCallback */)) + .isEqualTo(Map.of(ArtFlags.PASS_MAIN, mainResult, ArtFlags.PASS_SUPPLEMENTARY, + supplementaryResult)); } @Test(expected = IllegalStateException.class) @@ -899,6 +955,31 @@ public class ArtManagerLocalTest { } @Test + public void testOnBoot() throws Exception { + var progressCallbackExecutor = mock(Executor.class); + var progressCallback = mock(Consumer.class); + + when(mDexoptHelper.dexopt(any(), any(), + argThat(params -> params.getReason().equals(ReasonMapping.REASON_FIRST_BOOT)), + any(), any(), same(progressCallbackExecutor), same(progressCallback))) + .thenReturn(DexoptResult.create()); + + mArtManagerLocal.onBoot( + ReasonMapping.REASON_FIRST_BOOT, progressCallbackExecutor, progressCallback); + } + + @Test + public void testOnBootNoProgressCallback() throws Exception { + when(mDexoptHelper.dexopt(any(), any(), + argThat(params -> params.getReason().equals(ReasonMapping.REASON_FIRST_BOOT)), + any(), any(), isNull(), isNull())) + .thenReturn(DexoptResult.create()); + + mArtManagerLocal.onBoot(ReasonMapping.REASON_FIRST_BOOT, + null /* progressCallbackExecutor */, null /* progressCallback */); + } + + @Test public void testCleanup() throws Exception { // It should keep all artifacts. doReturn(createGetDexoptStatusResult("speed-profile", "bg-dexopt", "location")) diff --git a/libartservice/service/javatests/com/android/server/art/BackgroundDexoptJobTest.java b/libartservice/service/javatests/com/android/server/art/BackgroundDexoptJobTest.java index 3528caf3c3..c61461d0c9 100644 --- a/libartservice/service/javatests/com/android/server/art/BackgroundDexoptJobTest.java +++ b/libartservice/service/javatests/com/android/server/art/BackgroundDexoptJobTest.java @@ -17,6 +17,8 @@ package com.android.server.art; import static com.android.server.art.model.Config.Callback; +import static com.android.server.art.model.DexoptResult.DexoptResultStatus; +import static com.android.server.art.model.DexoptResult.PackageDexoptResult; import static com.google.common.truth.Truth.assertThat; @@ -25,6 +27,7 @@ import static org.mockito.Mockito.anyBoolean; import static org.mockito.Mockito.anyInt; import static org.mockito.Mockito.eq; import static org.mockito.Mockito.lenient; +import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.same; import static org.mockito.Mockito.times; @@ -56,6 +59,9 @@ import org.junit.runner.RunWith; import org.mockito.ArgumentCaptor; import org.mockito.Mock; +import java.util.HashMap; +import java.util.List; +import java.util.Map; import java.util.concurrent.Future; import java.util.concurrent.Semaphore; import java.util.concurrent.TimeUnit; @@ -74,12 +80,12 @@ public class BackgroundDexoptJobTest { @Mock private PackageManagerLocal mPackageManagerLocal; @Mock private PackageManagerLocal.FilteredSnapshot mSnapshot; @Mock private JobScheduler mJobScheduler; - @Mock private DexoptResult mDexoptResult; @Mock private BackgroundDexoptJobService mJobService; @Mock private JobParameters mJobParameters; private Config mConfig; private BackgroundDexoptJob mBackgroundDexoptJob; private Semaphore mJobFinishedCalled = new Semaphore(0); + private Map<Integer, DexoptResult> mDexoptResultByPass; @Before public void setUp() throws Exception { @@ -110,17 +116,19 @@ public class BackgroundDexoptJobTest { lenient() .when(mJobParameters.getStopReason()) .thenReturn(JobParameters.STOP_REASON_UNDEFINED); + + mDexoptResultByPass = new HashMap<>(); } @Test public void testStart() { when(mArtManagerLocal.dexoptPackages( same(mSnapshot), eq(ReasonMapping.REASON_BG_DEXOPT), any(), any(), any())) - .thenReturn(mDexoptResult); + .thenReturn(mDexoptResultByPass); Result result = Utils.getFuture(mBackgroundDexoptJob.start()); assertThat(result).isInstanceOf(CompletedResult.class); - assertThat(((CompletedResult) result).dexoptResult()).isSameInstanceAs(mDexoptResult); + assertThat(((CompletedResult) result).dexoptResultByPass()).isEqualTo(mDexoptResultByPass); verify(mArtManagerLocal).cleanup(same(mSnapshot)); } @@ -131,7 +139,7 @@ public class BackgroundDexoptJobTest { when(mArtManagerLocal.dexoptPackages(any(), any(), any(), any(), any())) .thenAnswer(invocation -> { assertThat(dexoptDone.tryAcquire(TIMEOUT_SEC, TimeUnit.SECONDS)).isTrue(); - return mDexoptResult; + return mDexoptResultByPass; }); Future<Result> future1 = mBackgroundDexoptJob.start(); @@ -147,7 +155,7 @@ public class BackgroundDexoptJobTest { @Test public void testStartAnother() { when(mArtManagerLocal.dexoptPackages(any(), any(), any(), any(), any())) - .thenReturn(mDexoptResult); + .thenReturn(mDexoptResultByPass); Future<Result> future1 = mBackgroundDexoptJob.start(); Utils.getFuture(future1); @@ -172,7 +180,7 @@ public class BackgroundDexoptJobTest { .thenReturn(true); when(mArtManagerLocal.dexoptPackages(any(), any(), any(), any(), any())) - .thenReturn(mDexoptResult); + .thenReturn(mDexoptResultByPass); // The `start` method should ignore the system property. The system property is for // `schedule`. @@ -187,7 +195,7 @@ public class BackgroundDexoptJobTest { assertThat(dexoptCancelled.tryAcquire(TIMEOUT_SEC, TimeUnit.SECONDS)).isTrue(); var cancellationSignal = invocation.<CancellationSignal>getArgument(2); assertThat(cancellationSignal.isCanceled()).isTrue(); - return mDexoptResult; + return mDexoptResultByPass; }); Future<Result> future = mBackgroundDexoptJob.start(); @@ -273,9 +281,12 @@ public class BackgroundDexoptJobTest { @Test public void testWantsRescheduleFalsePerformed() throws Exception { - when(mDexoptResult.getFinalStatus()).thenReturn(DexoptResult.DEXOPT_PERFORMED); + DexoptResult downgradeResult = createDexoptResultWithStatus(DexoptResult.DEXOPT_PERFORMED); + mDexoptResultByPass.put(ArtFlags.PASS_DOWNGRADE, downgradeResult); + DexoptResult mainResult = createDexoptResultWithStatus(DexoptResult.DEXOPT_PERFORMED); + mDexoptResultByPass.put(ArtFlags.PASS_MAIN, mainResult); when(mArtManagerLocal.dexoptPackages(any(), any(), any(), any(), any())) - .thenReturn(mDexoptResult); + .thenReturn(mDexoptResultByPass); mBackgroundDexoptJob.onStartJob(mJobService, mJobParameters); assertThat(mJobFinishedCalled.tryAcquire(TIMEOUT_SEC, TimeUnit.SECONDS)).isTrue(); @@ -296,13 +307,22 @@ public class BackgroundDexoptJobTest { @Test public void testWantsRescheduleTrue() throws Exception { - when(mDexoptResult.getFinalStatus()).thenReturn(DexoptResult.DEXOPT_CANCELLED); + DexoptResult downgradeResult = createDexoptResultWithStatus(DexoptResult.DEXOPT_PERFORMED); + mDexoptResultByPass.put(ArtFlags.PASS_DOWNGRADE, downgradeResult); + DexoptResult mainResult = createDexoptResultWithStatus(DexoptResult.DEXOPT_CANCELLED); + mDexoptResultByPass.put(ArtFlags.PASS_MAIN, mainResult); when(mArtManagerLocal.dexoptPackages(any(), any(), any(), any(), any())) - .thenReturn(mDexoptResult); + .thenReturn(mDexoptResultByPass); mBackgroundDexoptJob.onStartJob(mJobService, mJobParameters); assertThat(mJobFinishedCalled.tryAcquire(TIMEOUT_SEC, TimeUnit.SECONDS)).isTrue(); verify(mJobService).jobFinished(any(), eq(true) /* wantsReschedule */); } + + private DexoptResult createDexoptResultWithStatus(@DexoptResultStatus int status) { + return DexoptResult.create("compiler-filter", "reason", + List.of(PackageDexoptResult.create( + "package-name", List.of() /* dexContainerFileDexoptResults */, status))); + } } diff --git a/libartservice/service/javatests/com/android/server/art/PrimaryDexopterTest.java b/libartservice/service/javatests/com/android/server/art/PrimaryDexopterTest.java index 2d4cdc0cf3..cdabcf341c 100644 --- a/libartservice/service/javatests/com/android/server/art/PrimaryDexopterTest.java +++ b/libartservice/service/javatests/com/android/server/art/PrimaryDexopterTest.java @@ -291,8 +291,7 @@ public class PrimaryDexopterTest extends PrimaryDexopterTestBase { @Test public void testDexoptMergesProfiles() throws Exception { - when(mPkgState.getStateForUser(eq(UserHandle.of(0)))).thenReturn(mPkgUserStateInstalled); - when(mPkgState.getStateForUser(eq(UserHandle.of(2)))).thenReturn(mPkgUserStateInstalled); + setPackageInstalledForUserIds(0, 2); when(mArtd.mergeProfiles(any(), any(), any(), any(), any())).thenReturn(true); @@ -336,8 +335,7 @@ public class PrimaryDexopterTest extends PrimaryDexopterTestBase { @Test public void testDexoptMergesProfilesMergeFailed() throws Exception { - when(mPkgState.getStateForUser(eq(UserHandle.of(0)))).thenReturn(mPkgUserStateInstalled); - when(mPkgState.getStateForUser(eq(UserHandle.of(2)))).thenReturn(mPkgUserStateInstalled); + setPackageInstalledForUserIds(0, 2); when(mArtd.mergeProfiles(any(), any(), any(), any(), any())).thenReturn(false); @@ -364,6 +362,28 @@ public class PrimaryDexopterTest extends PrimaryDexopterTestBase { } @Test + public void testDexoptMergesProfilesForceMerge() throws Exception { + mDexoptParams = mDexoptParams.toBuilder() + .setFlags(ArtFlags.FLAG_FORCE_MERGE_PROFILE, + ArtFlags.FLAG_FORCE_MERGE_PROFILE) + .build(); + mPrimaryDexopter = + new PrimaryDexopter(mInjector, mPkgState, mPkg, mDexoptParams, mCancellationSignal); + + setPackageInstalledForUserIds(0, 2); + + mMergeProfileOptions.forceMerge = true; + when(mArtd.mergeProfiles(any(), any(), any(), any(), deepEq(mMergeProfileOptions))) + .thenReturn(true); + + makeProfileUsable(mRefProfile); + when(mArtd.getProfileVisibility(deepEq(mRefProfile))) + .thenReturn(FileVisibility.OTHER_READABLE); + + mPrimaryDexopter.dexopt(); + } + + @Test public void testDexoptUsesDmProfile() throws Exception { makeProfileNotUsable(mRefProfile); makeProfileNotUsable(mPrebuiltProfile); @@ -758,4 +778,12 @@ public class PrimaryDexopterTest extends PrimaryDexopterTestBase { assertThat(result.getExternalProfileErrors()).isEmpty(); } } + + /** Dexopter relies on this information to determine which current profiles to check. */ + private void setPackageInstalledForUserIds(int... userIds) { + for (int userId : userIds) { + when(mPkgState.getStateForUser(eq(UserHandle.of(userId)))) + .thenReturn(mPkgUserStateInstalled); + } + } } diff --git a/libartservice/service/javatests/com/android/server/art/model/DexoptParamsTest.java b/libartservice/service/javatests/com/android/server/art/model/DexoptParamsTest.java index 60986414fa..d98340ee17 100644 --- a/libartservice/service/javatests/com/android/server/art/model/DexoptParamsTest.java +++ b/libartservice/service/javatests/com/android/server/art/model/DexoptParamsTest.java @@ -16,12 +16,19 @@ package com.android.server.art.model; +import static com.google.common.truth.Truth.assertThat; + import androidx.test.filters.SmallTest; import androidx.test.runner.AndroidJUnit4; import org.junit.Test; import org.junit.runner.RunWith; +import java.lang.reflect.Field; +import java.lang.reflect.Modifier; +import java.util.Arrays; +import java.util.stream.Collectors; + @SmallTest @RunWith(AndroidJUnit4.class) public class DexoptParamsTest { @@ -106,4 +113,31 @@ public class DexoptParamsTest { .setSplitName("split_0") .build(); } + + @Test + public void testToBuilder() { + // Update this test with new fields if this assertion fails. + assertThat(Arrays.stream(DexoptParams.class.getDeclaredFields()) + .filter(field -> !Modifier.isStatic(field.getModifiers())) + .map(Field::getName) + .collect(Collectors.toList())) + .containsExactly( + "mFlags", "mCompilerFilter", "mPriorityClass", "mReason", "mSplitName"); + + DexoptParams params1 = + new DexoptParams.Builder("install") + .setFlags(ArtFlags.FLAG_FOR_PRIMARY_DEX | ArtFlags.FLAG_FOR_SINGLE_SPLIT) + .setCompilerFilter("speed") + .setPriorityClass(90) + .setSplitName("split_0") + .build(); + + DexoptParams params2 = params1.toBuilder().build(); + + assertThat(params1.getFlags()).isEqualTo(params2.getFlags()); + assertThat(params1.getCompilerFilter()).isEqualTo(params2.getCompilerFilter()); + assertThat(params1.getPriorityClass()).isEqualTo(params2.getPriorityClass()); + assertThat(params1.getReason()).isEqualTo(params2.getReason()); + assertThat(params1.getSplitName()).isEqualTo(params2.getSplitName()); + } } diff --git a/profman/include/profman/profman_result.h b/profman/include/profman/profman_result.h index 9c9aca9e05..c4ca98828d 100644 --- a/profman/include/profman/profman_result.h +++ b/profman/include/profman/profman_result.h @@ -40,9 +40,13 @@ class ProfmanResult { kSuccess = 0, // A merge has been performed, meaning the reference profile has been changed. kCompile = 1, - // `--profile-file(-fd)` is not specified, or the specified profiles are outdated (i.e., APK - // filename or checksum mismatch), empty, or don't contain enough number of new classes and - // methods that meets the threshold to trigger a merge. + // One of the following conditions is met: + // - `--profile-file(-fd)` is not specified. + // - The specified profiles are outdated (i.e., APK filename or checksum mismatch). + // - The specified profiles are empty. + // - The specified profiles don't contain any new class or method. + // - The specified profiles don't contain enough number of new classes and methods that meets + // the threshold to trigger a merge, and `--force-merge-and-analyze` is not set. kSkipCompilationSmallDelta = 2, // All the input profiles (including the reference profile) are either outdated (i.e., APK // filename or checksum mismatch) or empty. diff --git a/profman/profile_assistant.cc b/profman/profile_assistant.cc index abbde2d527..4d98e4927d 100644 --- a/profman/profile_assistant.cc +++ b/profman/profile_assistant.cc @@ -54,7 +54,7 @@ ProfmanResult::ProcessingResult ProfileAssistant::ProcessProfilesInternal( ProfileCompilationInfo cur_info(options.IsBootImageMerge()); if (!cur_info.Load(profile_files[i]->Fd(), /*merge_classes=*/ true, filter_fn)) { LOG(WARNING) << "Could not load profile file at index " << i; - if (options.IsForceMerge()) { + if (options.IsForceMerge() || options.IsForceMergeAndAnalyze()) { // If we have to merge forcefully, ignore load failures. // This is useful for boot image profiles to ignore stale profiles which are // cleared lazily. @@ -79,17 +79,30 @@ ProfmanResult::ProcessingResult ProfileAssistant::ProcessProfilesInternal( if (info.IsEmpty()) { return ProfmanResult::kSkipCompilationEmptyProfiles; } - uint32_t min_change_in_methods_for_compilation = std::max( - (options.GetMinNewMethodsPercentChangeForCompilation() * number_of_methods) / 100, - kMinNewMethodsForCompilation); - uint32_t min_change_in_classes_for_compilation = std::max( - (options.GetMinNewClassesPercentChangeForCompilation() * number_of_classes) / 100, - kMinNewClassesForCompilation); - // Check if there is enough new information added by the current profiles. - if (((info.GetNumberOfMethods() - number_of_methods) < min_change_in_methods_for_compilation) && - ((info.GetNumberOfResolvedClasses() - number_of_classes) - < min_change_in_classes_for_compilation)) { - return ProfmanResult::kSkipCompilationSmallDelta; + + if (options.IsForceMergeAndAnalyze()) { + // When we force merge and analyze, we want to always recompile unless there is absolutely no + // difference between before and after the merge (i.e., the classes and methods in the + // reference profile were already a superset of those in all current profiles before the + // merge.) + if (info.GetNumberOfMethods() == number_of_methods && + info.GetNumberOfResolvedClasses() == number_of_classes) { + return ProfmanResult::kSkipCompilationSmallDelta; + } + } else { + uint32_t min_change_in_methods_for_compilation = std::max( + (options.GetMinNewMethodsPercentChangeForCompilation() * number_of_methods) / 100, + kMinNewMethodsForCompilation); + uint32_t min_change_in_classes_for_compilation = std::max( + (options.GetMinNewClassesPercentChangeForCompilation() * number_of_classes) / 100, + kMinNewClassesForCompilation); + // Check if there is enough new information added by the current profiles. + if (((info.GetNumberOfMethods() - number_of_methods) < + min_change_in_methods_for_compilation) && + ((info.GetNumberOfResolvedClasses() - number_of_classes) < + min_change_in_classes_for_compilation)) { + return ProfmanResult::kSkipCompilationSmallDelta; + } } } diff --git a/profman/profile_assistant.h b/profman/profile_assistant.h index 6b7a7a67f5..4caf86b3e2 100644 --- a/profman/profile_assistant.h +++ b/profman/profile_assistant.h @@ -32,11 +32,12 @@ class ProfileAssistant { public: static constexpr bool kForceMergeDefault = false; static constexpr bool kBootImageMergeDefault = false; - static constexpr uint32_t kMinNewMethodsPercentChangeForCompilation = 20; - static constexpr uint32_t kMinNewClassesPercentChangeForCompilation = 20; + static constexpr uint32_t kMinNewMethodsPercentChangeForCompilation = 2; + static constexpr uint32_t kMinNewClassesPercentChangeForCompilation = 2; Options() : force_merge_(kForceMergeDefault), + force_merge_and_analyze_(kForceMergeDefault), boot_image_merge_(kBootImageMergeDefault), min_new_methods_percent_change_for_compilation_( kMinNewMethodsPercentChangeForCompilation), @@ -44,7 +45,9 @@ class ProfileAssistant { kMinNewClassesPercentChangeForCompilation) { } + // Only for S and T uses. U+ should use `IsForceMergeAndAnalyze`. bool IsForceMerge() const { return force_merge_; } + bool IsForceMergeAndAnalyze() const { return force_merge_and_analyze_; } bool IsBootImageMerge() const { return boot_image_merge_; } uint32_t GetMinNewMethodsPercentChangeForCompilation() const { return min_new_methods_percent_change_for_compilation_; @@ -54,6 +57,7 @@ class ProfileAssistant { } void SetForceMerge(bool value) { force_merge_ = value; } + void SetForceMergeAndAnalyze(bool value) { force_merge_and_analyze_ = value; } void SetBootImageMerge(bool value) { boot_image_merge_ = value; } void SetMinNewMethodsPercentChangeForCompilation(uint32_t value) { min_new_methods_percent_change_for_compilation_ = value; @@ -63,10 +67,15 @@ class ProfileAssistant { } private: - // If true, performs a forced merge, without analyzing if there is a - // significant difference between the current profile and the reference profile. + // If true, performs a forced merge, without analyzing if there is a significant difference + // between before and after the merge. // See ProfileAssistant#ProcessProfile. + // Only for S and T uses. U+ should use `force_merge_and_analyze_`. bool force_merge_; + // If true, performs a forced merge and analyzes if there is any difference between before and + // after the merge. + // See ProfileAssistant#ProcessProfile. + bool force_merge_and_analyze_; // Signals that the merge is for boot image profiles. It will ignore differences // in profile versions (instead of aborting). bool boot_image_merge_; diff --git a/profman/profile_assistant_test.cc b/profman/profile_assistant_test.cc index f7c4255071..93766e5d52 100644 --- a/profman/profile_assistant_test.cc +++ b/profman/profile_assistant_test.cc @@ -684,16 +684,6 @@ TEST_F(ProfileAssistantTest, ShouldAdviseCompilationMethodPercentage) { kNumberOfMethodsInCurProfile, kNumberOfMethodsInRefProfile, extra_args)); } -TEST_F(ProfileAssistantTest, DoNotAdviseCompilationMethodPercentageWithNewMin) { - const uint16_t kNumberOfMethodsInRefProfile = 6000; - const uint16_t kNumberOfMethodsInCurProfile = 6200; // Threshold is 20%. - - // We should not advise compilation. - ASSERT_EQ(ProfmanResult::kSkipCompilationSmallDelta, - CheckCompilationMethodPercentChange(kNumberOfMethodsInCurProfile, - kNumberOfMethodsInRefProfile)); -} - TEST_F(ProfileAssistantTest, DoNotAdviseCompilationClassPercentage) { const uint16_t kNumberOfClassesInRefProfile = 6000; const uint16_t kNumberOfClassesInCurProfile = 6110; // Threshold is 2%. @@ -716,16 +706,6 @@ TEST_F(ProfileAssistantTest, ShouldAdviseCompilationClassPercentage) { kNumberOfClassesInCurProfile, kNumberOfClassesInRefProfile, extra_args)); } -TEST_F(ProfileAssistantTest, DoNotAdviseCompilationClassPercentageWithNewMin) { - const uint16_t kNumberOfClassesInRefProfile = 6000; - const uint16_t kNumberOfClassesInCurProfile = 6200; // Threshold is 20%. - - // We should not advise compilation. - ASSERT_EQ(ProfmanResult::kSkipCompilationSmallDelta, - CheckCompilationClassPercentChange(kNumberOfClassesInCurProfile, - kNumberOfClassesInRefProfile)); -} - TEST_F(ProfileAssistantTest, FailProcessingBecauseOfProfiles) { ScratchFile profile1; ScratchFile profile2; @@ -2078,38 +2058,60 @@ TEST_F(ProfileAssistantTest, CopyAndUpdateProfileKeyNoUpdate) { } TEST_F(ProfileAssistantTest, BootImageMerge) { - ScratchFile profile; - ScratchFile reference_profile; - std::vector<int> profile_fds({GetFd(profile)}); - int reference_profile_fd = GetFd(reference_profile); - std::vector<uint32_t> hot_methods_cur; - std::vector<uint32_t> hot_methods_ref; - std::vector<uint32_t> empty_vector; - size_t num_methods = 100; - for (size_t i = 0; i < num_methods; ++i) { - hot_methods_cur.push_back(i); + ScratchFile profile1; + ScratchFile profile2; + ScratchFile profile3; + ScratchFile output_profile; + std::vector<uint32_t> hot_methods_1; + std::vector<uint32_t> hot_methods_2; + std::vector<uint32_t> hot_methods_3; + for (size_t i = 0; i < 100; ++i) { + hot_methods_1.push_back(i); + } + for (size_t i = 50; i < 150; ++i) { + hot_methods_2.push_back(i); } - for (size_t i = 0; i < num_methods; ++i) { - hot_methods_ref.push_back(i); + for (size_t i = 100; i < 200; ++i) { + hot_methods_3.push_back(i); } - ProfileCompilationInfo info1(/*for_boot_image=*/ true); - SetupBasicProfile(dex1, hot_methods_cur, empty_vector, empty_vector, - profile, &info1); + ProfileCompilationInfo info1(/*for_boot_image=*/false); + SetupBasicProfile( + dex1, hot_methods_1, /*startup_methods=*/{}, /*post_startup_methods=*/{}, profile1, &info1); ProfileCompilationInfo info2(/*for_boot_image=*/true); - SetupBasicProfile(dex1, hot_methods_ref, empty_vector, empty_vector, - reference_profile, &info2); + SetupBasicProfile( + dex1, hot_methods_2, /*startup_methods=*/{}, /*post_startup_methods=*/{}, profile2, &info2); + ProfileCompilationInfo info3(/*for_boot_image=*/true); + SetupBasicProfile( + dex1, hot_methods_3, /*startup_methods=*/{}, /*post_startup_methods=*/{}, profile3, &info3); - std::vector<const std::string> extra_args({"--force-merge", "--boot-image-merge"}); - - int return_code = ProcessProfiles(profile_fds, reference_profile_fd, extra_args); - - ASSERT_EQ(return_code, ProfmanResult::kSuccess); + { + int return_code = ProcessProfiles({profile1.GetFd(), profile2.GetFd(), profile3.GetFd()}, + output_profile.GetFd(), + {"--force-merge-and-analyze", "--boot-image-merge"}); + ASSERT_EQ(return_code, ProfmanResult::kCompile); + + // Verify the result: it should be equal to info2 union info3 since info1 is a regular profile + // and should be ignored. + ProfileCompilationInfo result(/*for_boot_image=*/true); + ASSERT_TRUE(result.Load(output_profile.GetFd())); + ASSERT_TRUE(info2.MergeWith(info3)); + ASSERT_TRUE(result.Equals(info2)); + } - // Verify the result: it should be equal to info2 since info1 is a regular profile - // and should be ignored. - ProfileCompilationInfo result(/*for_boot_image=*/ true); - ASSERT_TRUE(result.Load(reference_profile.GetFd())); - ASSERT_TRUE(result.Equals(info2)); + // Same for the legacy force merge mode. + { + int return_code = ProcessProfiles({profile1.GetFd(), profile2.GetFd(), profile3.GetFd()}, + output_profile.GetFd(), + {"--force-merge", "--boot-image-merge"}); + ASSERT_EQ(return_code, ProfmanResult::kSuccess); + + // Verify the result: it should be equal to info2 union info3 since info1 is a regular profile + // and should be ignored. + ProfileCompilationInfo result(/*for_boot_image=*/true); + ASSERT_TRUE(result.Load(output_profile.GetFd())); + ASSERT_TRUE(info2.MergeWith(info3)); + ASSERT_TRUE(result.Equals(info2)); + } } // Under default behaviour we should not advice compilation @@ -2154,6 +2156,82 @@ TEST_F(ProfileAssistantTest, ForceMerge) { ASSERT_TRUE(result.Equals(info1)); } +TEST_F(ProfileAssistantTest, ForceMergeAndAnalyze) { + const uint16_t kNumberOfMethodsInRefProfile = 600; + const uint16_t kNumberOfMethodsInCurProfile = 601; + + ScratchFile ref_profile; + ScratchFile cur_profile; + + ProfileCompilationInfo ref_info; + SetupProfile( + dex1, dex2, kNumberOfMethodsInRefProfile, /*number_of_classes=*/0, ref_profile, &ref_info); + ProfileCompilationInfo cur_info; + SetupProfile( + dex1, dex2, kNumberOfMethodsInCurProfile, /*number_of_classes=*/0, cur_profile, &cur_info); + + std::vector<const std::string> extra_args({"--force-merge-and-analyze"}); + int return_code = ProcessProfiles({cur_profile.GetFd()}, ref_profile.GetFd(), extra_args); + + ASSERT_EQ(return_code, ProfmanResult::kCompile); + + // Check that the result is the aggregation. + ProfileCompilationInfo result; + ASSERT_TRUE(result.Load(ref_profile.GetFd())); + ASSERT_TRUE(ref_info.MergeWith(cur_info)); + ASSERT_TRUE(result.Equals(ref_info)); +} + +TEST_F(ProfileAssistantTest, ForceMergeAndAnalyzeNoDelta) { + const uint16_t kNumberOfMethodsInRefProfile = 600; + const uint16_t kNumberOfMethodsInCurProfile = 600; + + ScratchFile ref_profile; + ScratchFile cur_profile; + + ProfileCompilationInfo ref_info; + SetupProfile( + dex1, dex2, kNumberOfMethodsInRefProfile, /*number_of_classes=*/0, ref_profile, &ref_info); + ProfileCompilationInfo cur_info; + SetupProfile( + dex1, dex2, kNumberOfMethodsInCurProfile, /*number_of_classes=*/0, cur_profile, &cur_info); + + std::vector<const std::string> extra_args({"--force-merge-and-analyze"}); + int return_code = ProcessProfiles({cur_profile.GetFd()}, ref_profile.GetFd(), extra_args); + + ASSERT_EQ(return_code, ProfmanResult::kSkipCompilationSmallDelta); + + // Check that the reference profile is unchanged. + ProfileCompilationInfo result; + ASSERT_TRUE(result.Load(ref_profile.GetFd())); + ASSERT_TRUE(result.Equals(ref_info)); +} + +TEST_F(ProfileAssistantTest, ForceMergeAndAnalyzeEmptyProfiles) { + const uint16_t kNumberOfMethodsInRefProfile = 0; + const uint16_t kNumberOfMethodsInCurProfile = 0; + + ScratchFile ref_profile; + ScratchFile cur_profile; + + ProfileCompilationInfo ref_info; + SetupProfile( + dex1, dex2, kNumberOfMethodsInRefProfile, /*number_of_classes=*/0, ref_profile, &ref_info); + ProfileCompilationInfo cur_info; + SetupProfile( + dex1, dex2, kNumberOfMethodsInCurProfile, /*number_of_classes=*/0, cur_profile, &cur_info); + + std::vector<const std::string> extra_args({"--force-merge-and-analyze"}); + int return_code = ProcessProfiles({cur_profile.GetFd()}, ref_profile.GetFd(), extra_args); + + ASSERT_EQ(return_code, ProfmanResult::kSkipCompilationEmptyProfiles); + + // Check that the reference profile is unchanged. + ProfileCompilationInfo result; + ASSERT_TRUE(result.Load(ref_profile.GetFd())); + ASSERT_TRUE(result.Equals(ref_info)); +} + // Test that we consider the annations when we merge boot image profiles. TEST_F(ProfileAssistantTest, BootImageMergeWithAnnotations) { ScratchFile profile; @@ -2236,25 +2314,40 @@ TEST_F(ProfileAssistantTest, ForceMergeIgnoreProfilesItCannotLoad) { std::string content = "giberish"; ASSERT_TRUE(profile1.GetFile()->WriteFully(content.c_str(), content.length())); - ProfileCompilationInfo info2(/*for_boot_image=*/ true); + ProfileCompilationInfo info2(/*for_boot_image=*/true); info2.Save(profile2.GetFd()); - std::vector<int> profile_fds({ GetFd(profile1)}); + std::vector<int> profile_fds({GetFd(profile1)}); int reference_profile_fd = GetFd(profile2); // With force-merge we should merge successfully. - std::vector<const std::string> extra_args({"--force-merge", "--boot-image-merge"}); - ASSERT_EQ(ProcessProfiles(profile_fds, reference_profile_fd, extra_args), - ProfmanResult::kSuccess); + { + ASSERT_EQ( + ProcessProfiles( + profile_fds, reference_profile_fd, {"--force-merge-and-analyze", "--boot-image-merge"}), + ProfmanResult::kSkipCompilationEmptyProfiles); + + ProfileCompilationInfo result(/*for_boot_image=*/true); + ASSERT_TRUE(result.Load(reference_profile_fd)); + ASSERT_TRUE(info2.Equals(result)); + } - ProfileCompilationInfo result(/*for_boot_image=*/ true); - ASSERT_TRUE(result.Load(reference_profile_fd)); - ASSERT_TRUE(info2.Equals(result)); + // Same for the legacy force merge mode. + { + ASSERT_EQ( + ProcessProfiles(profile_fds, reference_profile_fd, {"--force-merge", "--boot-image-merge"}), + ProfmanResult::kSuccess); + + ProfileCompilationInfo result(/*for_boot_image=*/true); + ASSERT_TRUE(result.Load(reference_profile_fd)); + ASSERT_TRUE(info2.Equals(result)); + } // Without force-merge we should fail. - std::vector<const std::string> extra_args2({"--boot-image-merge"}); - ASSERT_EQ(ProcessProfiles(profile_fds, reference_profile_fd, extra_args2), - ProfmanResult::kErrorBadProfiles); + { + ASSERT_EQ(ProcessProfiles(profile_fds, reference_profile_fd, {"--boot-image-merge"}), + ProfmanResult::kErrorBadProfiles); + } } } // namespace art diff --git a/profman/profman.cc b/profman/profman.cc index 375a489821..0958f4b01c 100644 --- a/profman/profman.cc +++ b/profman/profman.cc @@ -190,10 +190,13 @@ NO_RETURN static void Usage(const char *fmt, ...) { UsageError(" --boot-image-merge: indicates that this merge is for a boot image profile."); UsageError(" In this case, the reference profile must have a boot profile version."); UsageError(" --force-merge: performs a forced merge, without analyzing if there is a"); - UsageError(" significant difference between the current profile and the reference profile."); - UsageError(" --min-new-methods-percent-change=percentage between 0 and 100 (default 20)"); + UsageError(" significant difference between before and after the merge."); + UsageError(" Deprecated. Use --force-merge-and-analyze instead."); + UsageError(" --force-merge-and-analyze: performs a forced merge and analyzes if there is any"); + UsageError(" difference between before and after the merge."); + UsageError(" --min-new-methods-percent-change=percentage between 0 and 100 (default 2)"); UsageError(" the min percent of new methods to trigger a compilation."); - UsageError(" --min-new-classes-percent-change=percentage between 0 and 100 (default 20)"); + UsageError(" --min-new-classes-percent-change=percentage between 0 and 100 (default 2)"); UsageError(" the min percent of new classes to trigger a compilation."); UsageError(""); @@ -469,7 +472,11 @@ class ProfMan final { } else if (option == "--boot-image-merge") { profile_assistant_options_.SetBootImageMerge(true); } else if (option == "--force-merge") { + // For backward compatibility only. + // TODO(jiakaiz): Remove this when S and T are no longer supported. profile_assistant_options_.SetForceMerge(true); + } else if (option == "--force-merge-and-analyze") { + profile_assistant_options_.SetForceMergeAndAnalyze(true); } else { Usage("Unknown argument '%s'", raw_option); } diff --git a/runtime/gc/space/image_space.cc b/runtime/gc/space/image_space.cc index 56efcabb7c..f4606018b2 100644 --- a/runtime/gc/space/image_space.cc +++ b/runtime/gc/space/image_space.cc @@ -1052,6 +1052,7 @@ class ImageSpace::Loader { Thread* const self = Thread::Current(); static constexpr size_t kMinBlocks = 2u; const bool use_parallel = pool != nullptr && image_header.GetBlockCount() >= kMinBlocks; + bool failed_decompression = false; for (const ImageHeader::Block& block : image_header.GetBlocks(temp_map.Begin())) { auto function = [&](Thread*) { const uint64_t start2 = NanoTime(); @@ -1059,8 +1060,11 @@ class ImageSpace::Loader { bool result = block.Decompress(/*out_ptr=*/map.Begin(), /*in_ptr=*/temp_map.Begin(), error_msg); - if (!result && error_msg != nullptr) { - *error_msg = "Failed to decompress image block " + *error_msg; + if (!result) { + failed_decompression = true; + if (error_msg != nullptr) { + *error_msg = "Failed to decompress image block " + *error_msg; + } } VLOG(image) << "Decompress block " << block.GetDataSize() << " -> " << block.GetImageSize() << " in " << PrettyDuration(NanoTime() - start2); @@ -1082,6 +1086,10 @@ class ImageSpace::Loader { VLOG(image) << "Decompressing image took " << PrettyDuration(time) << " (" << PrettySize(static_cast<uint64_t>(map.Size()) * MsToNs(1000) / (time + 1)) << "/s)"; + if (failed_decompression) { + DCHECK(error_msg == nullptr || !error_msg->empty()); + return MemMap::Invalid(); + } } else { DCHECK(!allow_direct_mapping); // We do not allow direct mapping for boot image extensions compiled to a memfd. diff --git a/runtime/image.cc b/runtime/image.cc index c8021f82e2..37437b4b86 100644 --- a/runtime/image.cc +++ b/runtime/image.cc @@ -219,7 +219,14 @@ bool ImageHeader::Block::Decompress(uint8_t* out_ptr, if (!ok) { return false; } - CHECK_EQ(decompressed_size, image_size_); + if (decompressed_size != image_size_) { + if (error_msg != nullptr) { + // Maybe some disk / memory corruption, just bail. + *error_msg = (std::ostringstream() << "Decompressed size different than image size: " + << decompressed_size << ", and " << image_size_).str(); + } + return false; + } break; } default: { diff --git a/runtime/javaheapprof/javaheapsampler.cc b/runtime/javaheapprof/javaheapsampler.cc index a73ed0b719..74671347c7 100644 --- a/runtime/javaheapprof/javaheapsampler.cc +++ b/runtime/javaheapprof/javaheapsampler.cc @@ -131,10 +131,6 @@ void HeapSampler::AdjustSampleOffset(size_t adjustment) { << " next_bytes_until_sample = " << next_bytes_until_sample; } -bool HeapSampler::IsEnabled() { - return enabled_.load(std::memory_order_acquire); -} - int HeapSampler::GetSamplingInterval() { return p_sampling_interval_.load(std::memory_order_acquire); } diff --git a/runtime/javaheapprof/javaheapsampler.h b/runtime/javaheapprof/javaheapsampler.h index 618893cad0..41514726cd 100644 --- a/runtime/javaheapprof/javaheapsampler.h +++ b/runtime/javaheapprof/javaheapsampler.h @@ -68,7 +68,7 @@ class HeapSampler { // of new Tlab after Reset. void AdjustSampleOffset(size_t adjustment); // Is heap sampler enabled? - bool IsEnabled(); + bool IsEnabled() { return enabled_.load(std::memory_order_acquire); } // Set the sampling interval. void SetSamplingInterval(int sampling_interval) REQUIRES(!geo_dist_rng_lock_); // Return the sampling interval. @@ -80,7 +80,7 @@ class HeapSampler { // possibly decreasing sample intervals by sample_adj_bytes. size_t PickAndAdjustNextSample(size_t sample_adj_bytes = 0) REQUIRES(!geo_dist_rng_lock_); - std::atomic<bool> enabled_; + std::atomic<bool> enabled_{false}; // Default sampling interval is 4kb. // Writes guarded by geo_dist_rng_lock_. std::atomic<int> p_sampling_interval_{4 * 1024}; diff --git a/runtime/jit/jit.cc b/runtime/jit/jit.cc index b231cce0bc..3472c788ab 100644 --- a/runtime/jit/jit.cc +++ b/runtime/jit/jit.cc @@ -71,7 +71,7 @@ static constexpr uint32_t kJitStressDefaultOptimizeThreshold = kJitDefaultOptimi static constexpr uint32_t kJitSlowStressDefaultOptimizeThreshold = kJitStressDefaultOptimizeThreshold / 2; -static constexpr uint32_t kJitDefaultWarmupThreshold = 0xffff; +static constexpr uint32_t kJitDefaultWarmupThreshold = 0x3fff; // Different warm-up threshold constants. These default to the equivalent warmup thresholds divided // by 2, but can be overridden at the command-line. static constexpr uint32_t kJitStressDefaultWarmupThreshold = kJitDefaultWarmupThreshold / 2; diff --git a/runtime/native/dalvik_system_ZygoteHooks.cc b/runtime/native/dalvik_system_ZygoteHooks.cc index 3c73cc569e..af0ee53854 100644 --- a/runtime/native/dalvik_system_ZygoteHooks.cc +++ b/runtime/native/dalvik_system_ZygoteHooks.cc @@ -347,10 +347,12 @@ static void ZygoteHooks_nativePostForkChild(JNIEnv* env, runtime->GetHeap()->PostForkChildAction(thread); - // Setup an app startup complete task in case the app doesn't notify it - // through VMRuntime::notifyStartupCompleted. - static constexpr uint64_t kMaxAppStartupTimeNs = MsToNs(5000); // 5 seconds - runtime->GetHeap()->AddHeapTask(new StartupCompletedTask(NanoTime() + kMaxAppStartupTimeNs)); + if (!is_zygote) { + // Setup an app startup complete task in case the app doesn't notify it + // through VMRuntime::notifyStartupCompleted. + static constexpr uint64_t kMaxAppStartupTimeNs = MsToNs(5000); // 5 seconds + runtime->GetHeap()->AddHeapTask(new StartupCompletedTask(NanoTime() + kMaxAppStartupTimeNs)); + } if (runtime->GetJit() != nullptr) { if (!is_system_server) { diff --git a/runtime/runtime.cc b/runtime/runtime.cc index 93a8bd0f91..7eff246d12 100644 --- a/runtime/runtime.cc +++ b/runtime/runtime.cc @@ -956,7 +956,8 @@ static jobject CreateSystemClassLoader(Runtime* runtime) { CHECK(getSystemClassLoader->IsStatic()); ObjPtr<mirror::Object> system_class_loader = getSystemClassLoader->InvokeStatic<'L'>(soa.Self()); - CHECK(system_class_loader != nullptr); + CHECK(system_class_loader != nullptr) + << (soa.Self()->IsExceptionPending() ? soa.Self()->GetException()->Dump() : "<null>"); ScopedAssertNoThreadSuspension sants(__FUNCTION__); jobject g_system_class_loader = diff --git a/test/2247-checker-write-barrier-elimination/Android.bp b/test/2247-checker-write-barrier-elimination/Android.bp index c9744e9b00..5848cb496e 100644 --- a/test/2247-checker-write-barrier-elimination/Android.bp +++ b/test/2247-checker-write-barrier-elimination/Android.bp @@ -15,7 +15,7 @@ package { java_test { name: "art-run-test-2247-checker-write-barrier-elimination", defaults: ["art-run-test-defaults"], - test_config_template: ":art-run-test-target-template", + test_config_template: ":art-run-test-target-no-test-suite-tag-template", srcs: ["src/**/*.java"], data: [ ":art-run-test-2247-checker-write-barrier-elimination-expected-stdout", diff --git a/test/knownfailures.json b/test/knownfailures.json index f6c0e25ebe..91cf968413 100644 --- a/test/knownfailures.json +++ b/test/knownfailures.json @@ -1,5 +1,10 @@ [ { + "tests": "2247-checker-write-barrier-elimination", + "description": ["Disable 2247- until we fix the WBE issue."], + "bug": "http://b/310755375" + }, + { "tests": "153-reference-stress", "description": ["Disable 153-reference-stress temporarily until a fix", "arrives."], diff --git a/test/utils/regen-test-files b/test/utils/regen-test-files index d047c2354a..64846a7ef9 100755 --- a/test/utils/regen-test-files +++ b/test/utils/regen-test-files @@ -215,7 +215,9 @@ known_failing_tests = frozenset([ "993-breakpoints-non-debuggable", "2243-single-step-default", "2262-miranda-methods", - "2262-default-conflict-methods" + "2262-default-conflict-methods", + # 2247-checker-write-barrier-elimination: Disabled while we investigate failures + "2247-checker-write-barrier-elimination" ]) known_failing_on_hwasan_tests = frozenset([ |