From 888d691d61d1f3596fd67ce151eb72e62fb6cf5e Mon Sep 17 00:00:00 2001 From: Jiakai Zhang Date: Wed, 18 Sep 2024 13:51:03 +0100 Subject: Offload `PreRebootDexoptJob.onUpdateReady` to a separate thread. Before this change, `PreRebootDexoptJob.onUpdateReady` is called on the package manager thread during an apex installation and triggered a pre-watchdog (1/4 of the watchdog timeout). This change fixes it. Bug: 366387132 Test: atest ArtServiceTests Change-Id: Ie50c04c1bfa81d38178ed1a2aa37a766f2da8a4a --- .../com/android/server/art/ArtShellCommand.java | 2 +- .../com/android/server/art/PreRebootDexoptJob.java | 14 +++++- .../android/server/art/PreRebootDexoptJobTest.java | 54 +++++++++++----------- 3 files changed, 40 insertions(+), 30 deletions(-) (limited to 'libartservice') diff --git a/libartservice/service/java/com/android/server/art/ArtShellCommand.java b/libartservice/service/java/com/android/server/art/ArtShellCommand.java index a5ad18dbbc..f7d26cb36f 100644 --- a/libartservice/service/java/com/android/server/art/ArtShellCommand.java +++ b/libartservice/service/java/com/android/server/art/ArtShellCommand.java @@ -817,7 +817,7 @@ public final class ArtShellCommand extends BasicShellCommandHandler { @RequiresApi(Build.VERSION_CODES.VANILLA_ICE_CREAM) private int handleSchedulePrDexoptJob(@NonNull PrintWriter pw, @Nullable String otaSlot) { - int code = mArtManagerLocal.getPreRebootDexoptJob().onUpdateReady(otaSlot); + int code = mArtManagerLocal.getPreRebootDexoptJob().onUpdateReadyImpl(otaSlot); switch (code) { case ArtFlags.SCHEDULE_SUCCESS: pw.println("Pre-reboot Dexopt job scheduled"); diff --git a/libartservice/service/java/com/android/server/art/PreRebootDexoptJob.java b/libartservice/service/java/com/android/server/art/PreRebootDexoptJob.java index c560300b48..65062612a3 100644 --- a/libartservice/service/java/com/android/server/art/PreRebootDexoptJob.java +++ b/libartservice/service/java/com/android/server/art/PreRebootDexoptJob.java @@ -90,6 +90,8 @@ public class PreRebootDexoptJob implements ArtServiceJobInterface { /** * Offloads `onStartJob` and `onStopJob` calls from the main thread while keeping the execution * order as the main thread does. + * Also offloads `onUpdateReady` calls from the package manager thread. We reuse this executor + * just for simplicity. The execution order does not matter. */ @NonNull private final ThreadPoolExecutor mSerializedExecutor = @@ -181,7 +183,14 @@ public class PreRebootDexoptJob implements ArtServiceJobInterface { * @param otaSlot The slot that contains the OTA update, "_a" or "_b", or null for a Mainline * update. */ - public synchronized @ScheduleStatus int onUpdateReady(@Nullable String otaSlot) { + public synchronized void onUpdateReady(@Nullable String otaSlot) { + // `onUpdateReadyImpl` can take time, especially on `resetLocked` when there are staged + // files from a previous run to be cleaned up, so we put it on a separate thread. + mSerializedExecutor.execute(() -> onUpdateReadyImpl(otaSlot)); + } + + /** For internal and testing use only. */ + public synchronized @ScheduleStatus int onUpdateReadyImpl(@Nullable String otaSlot) { cancelAnyLocked(); resetLocked(); updateOtaSlotLocked(otaSlot); @@ -190,7 +199,8 @@ public class PreRebootDexoptJob implements ArtServiceJobInterface { } /** - * Same as above, but starts the job immediately, instead of going through the job scheduler. + * Same as {@link #onUpdateReady}, but starts the job immediately, instead of going through the + * job scheduler. * * @param mapSnapshotsForOta whether to map/unmap snapshots. Only applicable to an OTA update. * @return The future of the job, or null if Pre-reboot Dexopt is not enabled. diff --git a/libartservice/service/javatests/com/android/server/art/PreRebootDexoptJobTest.java b/libartservice/service/javatests/com/android/server/art/PreRebootDexoptJobTest.java index fc83a32c9a..4aaf1d5be9 100644 --- a/libartservice/service/javatests/com/android/server/art/PreRebootDexoptJobTest.java +++ b/libartservice/service/javatests/com/android/server/art/PreRebootDexoptJobTest.java @@ -134,7 +134,7 @@ public class PreRebootDexoptJobTest { @Test public void testSchedule() throws Exception { - assertThat(mPreRebootDexoptJob.onUpdateReady(null /* otaSlot */)) + assertThat(mPreRebootDexoptJob.onUpdateReadyImpl(null /* otaSlot */)) .isEqualTo(ArtFlags.SCHEDULE_SUCCESS); assertThat(mJobInfo.isPeriodic()).isFalse(); @@ -148,7 +148,7 @@ public class PreRebootDexoptJobTest { when(SystemProperties.getBoolean(eq("pm.dexopt.disable_bg_dexopt"), anyBoolean())) .thenReturn(true); - assertThat(mPreRebootDexoptJob.onUpdateReady(null /* otaSlot */)) + assertThat(mPreRebootDexoptJob.onUpdateReadyImpl(null /* otaSlot */)) .isEqualTo(ArtFlags.SCHEDULE_DISABLED_BY_SYSPROP); verify(mJobScheduler, never()).schedule(any()); @@ -171,7 +171,7 @@ public class PreRebootDexoptJobTest { when(SystemProperties.getBoolean(eq("dalvik.vm.enable_pr_dexopt"), anyBoolean())) .thenReturn(false); - assertThat(mPreRebootDexoptJob.onUpdateReady(null /* otaSlot */)) + assertThat(mPreRebootDexoptJob.onUpdateReadyImpl(null /* otaSlot */)) .isEqualTo(ArtFlags.SCHEDULE_DISABLED_BY_SYSPROP); verify(mJobScheduler, never()).schedule(any()); @@ -199,7 +199,7 @@ public class PreRebootDexoptJobTest { eq(DeviceConfig.NAMESPACE_RUNTIME), eq("enable_pr_dexopt"), anyBoolean())) .thenReturn(true); - assertThat(mPreRebootDexoptJob.onUpdateReady(null /* otaSlot */)) + assertThat(mPreRebootDexoptJob.onUpdateReadyImpl(null /* otaSlot */)) .isEqualTo(ArtFlags.SCHEDULE_SUCCESS); verify(mJobScheduler).schedule(any()); @@ -218,7 +218,7 @@ public class PreRebootDexoptJobTest { eq("force_disable_pr_dexopt"), anyBoolean())) .thenReturn(true); - assertThat(mPreRebootDexoptJob.onUpdateReady(null /* otaSlot */)) + assertThat(mPreRebootDexoptJob.onUpdateReadyImpl(null /* otaSlot */)) .isEqualTo(ArtFlags.SCHEDULE_DISABLED_BY_SYSPROP); verify(mJobScheduler, never()).schedule(any()); @@ -226,7 +226,7 @@ public class PreRebootDexoptJobTest { @Test public void testUnschedule() { - mPreRebootDexoptJob.onUpdateReady(null /* otaSlot */); + mPreRebootDexoptJob.onUpdateReadyImpl(null /* otaSlot */); verify(mJobScheduler).cancel(JOB_ID); } @@ -248,7 +248,7 @@ public class PreRebootDexoptJobTest { }); assertThat(mPreRebootDexoptJob.hasStarted()).isFalse(); - mPreRebootDexoptJob.onUpdateReady(null /* otaSlot */); + mPreRebootDexoptJob.onUpdateReadyImpl(null /* otaSlot */); mPreRebootDexoptJob.onStartJobImpl(mJobService, mJobParameters); assertThat(jobStarted.tryAcquire(TIMEOUT_SEC, TimeUnit.SECONDS)).isTrue(); assertThat(mPreRebootDexoptJob.hasStarted()).isTrue(); @@ -279,7 +279,7 @@ public class PreRebootDexoptJobTest { return true; }); - mPreRebootDexoptJob.onUpdateReady(null /* otaSlot */); + mPreRebootDexoptJob.onUpdateReadyImpl(null /* otaSlot */); mPreRebootDexoptJob.onStartJobImpl(mJobService, mJobParameters); mPreRebootDexoptJob.onStopJobImpl(mJobParameters); @@ -311,8 +311,8 @@ public class PreRebootDexoptJobTest { @Test public void testUpdateOtaSlotOtaThenMainline() { - mPreRebootDexoptJob.onUpdateReady("_b" /* otaSlot */); - mPreRebootDexoptJob.onUpdateReady(null /* otaSlot */); + mPreRebootDexoptJob.onUpdateReadyImpl("_b" /* otaSlot */); + mPreRebootDexoptJob.onUpdateReadyImpl(null /* otaSlot */); when(mPreRebootDriver.run(eq("_b"), anyBoolean(), any())).thenReturn(true); @@ -322,8 +322,8 @@ public class PreRebootDexoptJobTest { @Test public void testUpdateOtaSlotMainlineThenOta() { - mPreRebootDexoptJob.onUpdateReady(null /* otaSlot */); - mPreRebootDexoptJob.onUpdateReady("_a" /* otaSlot */); + mPreRebootDexoptJob.onUpdateReadyImpl(null /* otaSlot */); + mPreRebootDexoptJob.onUpdateReadyImpl("_a" /* otaSlot */); when(mPreRebootDriver.run(eq("_a"), anyBoolean(), any())).thenReturn(true); @@ -333,8 +333,8 @@ public class PreRebootDexoptJobTest { @Test public void testUpdateOtaSlotMainlineThenMainline() { - mPreRebootDexoptJob.onUpdateReady(null /* otaSlot */); - mPreRebootDexoptJob.onUpdateReady(null /* otaSlot */); + mPreRebootDexoptJob.onUpdateReadyImpl(null /* otaSlot */); + mPreRebootDexoptJob.onUpdateReadyImpl(null /* otaSlot */); when(mPreRebootDriver.run(isNull(), anyBoolean(), any())).thenReturn(true); @@ -344,8 +344,8 @@ public class PreRebootDexoptJobTest { @Test public void testUpdateOtaSlotOtaThenOta() { - mPreRebootDexoptJob.onUpdateReady("_b" /* otaSlot */); - mPreRebootDexoptJob.onUpdateReady("_b" /* otaSlot */); + mPreRebootDexoptJob.onUpdateReadyImpl("_b" /* otaSlot */); + mPreRebootDexoptJob.onUpdateReadyImpl("_b" /* otaSlot */); when(mPreRebootDriver.run(eq("_b"), anyBoolean(), any())).thenReturn(true); @@ -355,13 +355,13 @@ public class PreRebootDexoptJobTest { @Test(expected = IllegalStateException.class) public void testUpdateOtaSlotOtaThenOtaDifferentSlots() { - mPreRebootDexoptJob.onUpdateReady("_b" /* otaSlot */); - mPreRebootDexoptJob.onUpdateReady("_a" /* otaSlot */); + mPreRebootDexoptJob.onUpdateReadyImpl("_b" /* otaSlot */); + mPreRebootDexoptJob.onUpdateReadyImpl("_a" /* otaSlot */); } @Test(expected = IllegalStateException.class) public void testUpdateOtaSlotOtaBogusSlot() { - mPreRebootDexoptJob.onUpdateReady("_bogus" /* otaSlot */); + mPreRebootDexoptJob.onUpdateReadyImpl("_bogus" /* otaSlot */); } /** @@ -379,7 +379,7 @@ public class PreRebootDexoptJobTest { }); // An update arrives. A job is scheduled. - mPreRebootDexoptJob.onUpdateReady(null /* otaSlot */); + mPreRebootDexoptJob.onUpdateReadyImpl(null /* otaSlot */); // The job scheduler starts the job. mPreRebootDexoptJob.onStartJobImpl(mJobService, mJobParameters); @@ -390,11 +390,11 @@ public class PreRebootDexoptJobTest { // Another update arrives. A new job is scheduled, replacing the old job. The old job // doesn't exit immediately, so this call is blocked. JobParameters oldParameters = mJobParameters; - mPreRebootDexoptJob.onUpdateReady(null /* otaSlot */); + mPreRebootDexoptJob.onUpdateReadyImpl(null /* otaSlot */); // The job scheduler tries to cancel the old job because of the new update. This call // doesn't matter because the job has already been cancelled by ourselves during the - // `onUpdateReady` call above. + // `onUpdateReadyImpl` call above. mPreRebootDexoptJob.onStopJobImpl(oldParameters); // The job scheduler starts the new job. @@ -432,15 +432,15 @@ public class PreRebootDexoptJobTest { @Test public void testRace2() throws Exception { // An update arrives. A job is scheduled. - mPreRebootDexoptJob.onUpdateReady(null /* otaSlot */); + mPreRebootDexoptJob.onUpdateReadyImpl(null /* otaSlot */); JobParameters oldParameters = mJobParameters; // The job scheduler starts the job. In the meantime, another update arrives. It's not - // possible that `onStartJob` is called for the old job after `onUpdateReady` is called - // because `onUpdateReady` unschedules the old job. However, since both calls acquire a + // possible that `onStartJob` is called for the old job after `onUpdateReadyImpl` is called + // because `onUpdateReadyImpl` unschedules the old job. However, since both calls acquire a // lock, the order of execution may be reversed. When this happens, the `onStartJob` request // should not succeed. - mPreRebootDexoptJob.onUpdateReady(null /* otaSlot */); + mPreRebootDexoptJob.onUpdateReadyImpl(null /* otaSlot */); mPreRebootDexoptJob.onStartJobImpl(mJobService, oldParameters); assertThat(mPreRebootDexoptJob.hasRunningJob()).isFalse(); @@ -465,7 +465,7 @@ public class PreRebootDexoptJobTest { }); // An update arrives. A job is scheduled. - mPreRebootDexoptJob.onUpdateReady(null /* otaSlot */); + mPreRebootDexoptJob.onUpdateReadyImpl(null /* otaSlot */); // The job scheduler starts the job. mPreRebootDexoptJob.onStartJobImpl(mJobService, mJobParameters); -- cgit v1.2.3-59-g8ed1b