diff options
| -rw-r--r-- | libartservice/service/java/com/android/server/art/PreRebootDexoptJob.java | 45 | ||||
| -rw-r--r-- | libartservice/service/javatests/com/android/server/art/PreRebootDexoptJobTest.java | 30 |
2 files changed, 54 insertions, 21 deletions
diff --git a/libartservice/service/java/com/android/server/art/PreRebootDexoptJob.java b/libartservice/service/java/com/android/server/art/PreRebootDexoptJob.java index 5c4a9093ca..a7cc8331c1 100644 --- a/libartservice/service/java/com/android/server/art/PreRebootDexoptJob.java +++ b/libartservice/service/java/com/android/server/art/PreRebootDexoptJob.java @@ -48,6 +48,9 @@ import java.time.Duration; import java.util.Objects; import java.util.UUID; import java.util.concurrent.CompletableFuture; +import java.util.concurrent.LinkedBlockingQueue; +import java.util.concurrent.ThreadPoolExecutor; +import java.util.concurrent.TimeUnit; /** * The Pre-reboot Dexopt job. @@ -81,6 +84,15 @@ public class PreRebootDexoptJob implements ArtServiceJobInterface { /** Whether to map/unmap snapshots. Only applicable to an OTA update. */ @GuardedBy("this") private boolean mMapSnapshotsForOta = false; + /** + * Offloads `onStartJob` and `onStopJob` calls from the main thread while keeping the execution + * order as the main thread does. + */ + @NonNull + private final ThreadPoolExecutor mSerializedExecutor = + new ThreadPoolExecutor(1 /* corePoolSize */, 1 /* maximumPoolSize */, + 60 /* keepAliveTime */, TimeUnit.SECONDS, new LinkedBlockingQueue<Runnable>()); + // Mutations to the global state of Pre-reboot Dexopt, including mounts, staged files, and // stats, should only be done when there is no job running and the `this` lock is held, or by // the job itself. @@ -92,17 +104,28 @@ public class PreRebootDexoptJob implements ArtServiceJobInterface { @VisibleForTesting public PreRebootDexoptJob(@NonNull Injector injector) { mInjector = injector; + // Recycle the thread if it's not used for `keepAliveTime`. + mSerializedExecutor.allowsCoreThreadTimeOut(); } @Override - public synchronized boolean onStartJob( + public boolean onStartJob( + @NonNull BackgroundDexoptJobService jobService, @NonNull JobParameters params) { + mSerializedExecutor.execute(() -> onStartJobImpl(jobService, params)); + // "true" means the job will continue running until `jobFinished` is called. + return true; + } + + @VisibleForTesting + public synchronized void onStartJobImpl( @NonNull BackgroundDexoptJobService jobService, @NonNull JobParameters params) { JobInfo pendingJob = mInjector.getJobScheduler().getPendingJob(JOB_ID); if (pendingJob == null || !params.getExtras().getString("ticket").equals( pendingJob.getExtras().getString("ticket"))) { // Job expired. We can only get here due to a race, and this should be very rare. - return false; + Utils.check(!mIsRunningJobKnownByJobScheduler); + return; } mIsRunningJobKnownByJobScheduler = true; @@ -118,17 +141,20 @@ public class PreRebootDexoptJob implements ArtServiceJobInterface { // No need to handle exceptions thrown by the future because exceptions are handled inside // the future itself. startLocked(onJobFinishedLocked); - // "true" means the job will continue running until `jobFinished` is called. - return true; } @Override - public synchronized boolean onStopJob(@NonNull JobParameters params) { + public boolean onStopJob(@NonNull JobParameters params) { + mSerializedExecutor.execute(() -> onStopJobImpl(params)); + // "true" means to execute again with the default retry policy. + return true; + } + + @VisibleForTesting + public synchronized void onStopJobImpl(@NonNull JobParameters params) { if (mIsRunningJobKnownByJobScheduler) { cancelGivenLocked(mRunningJob, false /* expectInterrupt */); } - // "true" means to execute again with the default retry policy. - return true; } /** @@ -194,6 +220,11 @@ public class PreRebootDexoptJob implements ArtServiceJobInterface { } } + @VisibleForTesting + public synchronized boolean hasRunningJob() { + return mRunningJob != null; + } + @GuardedBy("this") private @ScheduleStatus int scheduleLocked() { if (this != BackgroundDexoptJobService.getJob(JOB_ID)) { diff --git a/libartservice/service/javatests/com/android/server/art/PreRebootDexoptJobTest.java b/libartservice/service/javatests/com/android/server/art/PreRebootDexoptJobTest.java index 7103d4023b..fc83a32c9a 100644 --- a/libartservice/service/javatests/com/android/server/art/PreRebootDexoptJobTest.java +++ b/libartservice/service/javatests/com/android/server/art/PreRebootDexoptJobTest.java @@ -249,7 +249,7 @@ public class PreRebootDexoptJobTest { assertThat(mPreRebootDexoptJob.hasStarted()).isFalse(); mPreRebootDexoptJob.onUpdateReady(null /* otaSlot */); - mPreRebootDexoptJob.onStartJob(mJobService, mJobParameters); + mPreRebootDexoptJob.onStartJobImpl(mJobService, mJobParameters); assertThat(jobStarted.tryAcquire(TIMEOUT_SEC, TimeUnit.SECONDS)).isTrue(); assertThat(mPreRebootDexoptJob.hasStarted()).isTrue(); @@ -280,8 +280,8 @@ public class PreRebootDexoptJobTest { }); mPreRebootDexoptJob.onUpdateReady(null /* otaSlot */); - mPreRebootDexoptJob.onStartJob(mJobService, mJobParameters); - mPreRebootDexoptJob.onStopJob(mJobParameters); + mPreRebootDexoptJob.onStartJobImpl(mJobService, mJobParameters); + mPreRebootDexoptJob.onStopJobImpl(mJobParameters); // Check that `onStopJob` is really blocking. If it wasn't, the check below might still pass // due to a race, but we would have a flaky test. @@ -316,7 +316,7 @@ public class PreRebootDexoptJobTest { when(mPreRebootDriver.run(eq("_b"), anyBoolean(), any())).thenReturn(true); - mPreRebootDexoptJob.onStartJob(mJobService, mJobParameters); + mPreRebootDexoptJob.onStartJobImpl(mJobService, mJobParameters); mPreRebootDexoptJob.waitForRunningJob(); } @@ -327,7 +327,7 @@ public class PreRebootDexoptJobTest { when(mPreRebootDriver.run(eq("_a"), anyBoolean(), any())).thenReturn(true); - mPreRebootDexoptJob.onStartJob(mJobService, mJobParameters); + mPreRebootDexoptJob.onStartJobImpl(mJobService, mJobParameters); mPreRebootDexoptJob.waitForRunningJob(); } @@ -338,7 +338,7 @@ public class PreRebootDexoptJobTest { when(mPreRebootDriver.run(isNull(), anyBoolean(), any())).thenReturn(true); - mPreRebootDexoptJob.onStartJob(mJobService, mJobParameters); + mPreRebootDexoptJob.onStartJobImpl(mJobService, mJobParameters); mPreRebootDexoptJob.waitForRunningJob(); } @@ -349,7 +349,7 @@ public class PreRebootDexoptJobTest { when(mPreRebootDriver.run(eq("_b"), anyBoolean(), any())).thenReturn(true); - mPreRebootDexoptJob.onStartJob(mJobService, mJobParameters); + mPreRebootDexoptJob.onStartJobImpl(mJobService, mJobParameters); mPreRebootDexoptJob.waitForRunningJob(); } @@ -382,7 +382,7 @@ public class PreRebootDexoptJobTest { mPreRebootDexoptJob.onUpdateReady(null /* otaSlot */); // The job scheduler starts the job. - mPreRebootDexoptJob.onStartJob(mJobService, mJobParameters); + mPreRebootDexoptJob.onStartJobImpl(mJobService, mJobParameters); var jobFinishedCalledAfterNewJobStarted = new Semaphore(0); @@ -395,10 +395,10 @@ public class PreRebootDexoptJobTest { // 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. - mPreRebootDexoptJob.onStopJob(oldParameters); + mPreRebootDexoptJob.onStopJobImpl(oldParameters); // The job scheduler starts the new job. - mPreRebootDexoptJob.onStartJob(mJobService, mJobParameters); + mPreRebootDexoptJob.onStartJobImpl(mJobService, mJobParameters); doAnswer(invocation -> { jobFinishedCalledAfterNewJobStarted.release(); @@ -441,10 +441,12 @@ public class PreRebootDexoptJobTest { // lock, the order of execution may be reversed. When this happens, the `onStartJob` request // should not succeed. mPreRebootDexoptJob.onUpdateReady(null /* otaSlot */); - assertThat(mPreRebootDexoptJob.onStartJob(mJobService, oldParameters)).isFalse(); + mPreRebootDexoptJob.onStartJobImpl(mJobService, oldParameters); + assertThat(mPreRebootDexoptJob.hasRunningJob()).isFalse(); // The job scheduler starts the new job. This request should succeed. - assertThat(mPreRebootDexoptJob.onStartJob(mJobService, mJobParameters)).isTrue(); + mPreRebootDexoptJob.onStartJobImpl(mJobService, mJobParameters); + assertThat(mPreRebootDexoptJob.hasRunningJob()).isTrue(); } /** @@ -466,7 +468,7 @@ public class PreRebootDexoptJobTest { mPreRebootDexoptJob.onUpdateReady(null /* otaSlot */); // The job scheduler starts the job. - mPreRebootDexoptJob.onStartJob(mJobService, mJobParameters); + mPreRebootDexoptJob.onStartJobImpl(mJobService, mJobParameters); // Another update arrives, requesting a synchronous job run, replacing the old job. The new // job, which is synchronous, is started right after the old job is cancelled by @@ -482,7 +484,7 @@ public class PreRebootDexoptJobTest { // The `onStopJob` call finally arrives. This call should be a no-op because the job has // already been cancelled by ourselves during the `onUpdateReadyStartNow` call above. It // should not cancel the new job. - mPreRebootDexoptJob.onStopJob(oldParameters); + mPreRebootDexoptJob.onStopJobImpl(oldParameters); // The new job should not be cancelled. assertThat(jobExited.tryAcquire()).isFalse(); |