diff --git a/apex/jobscheduler/service/java/com/android/server/job/JobSchedulerService.java b/apex/jobscheduler/service/java/com/android/server/job/JobSchedulerService.java index 96cbed75622f2..78670c7c73b1c 100644 --- a/apex/jobscheduler/service/java/com/android/server/job/JobSchedulerService.java +++ b/apex/jobscheduler/service/java/com/android/server/job/JobSchedulerService.java @@ -73,7 +73,9 @@ import android.util.IndentingPrintWriter; import android.util.Log; import android.util.Slog; import android.util.SparseArray; +import android.util.SparseBooleanArray; import android.util.SparseIntArray; +import android.util.SparseLongArray; import android.util.SparseSetArray; import android.util.TimeUtils; import android.util.proto.ProtoOutputStream; @@ -686,26 +688,92 @@ public class JobSchedulerService extends com.android.server.SystemService final Constants mConstants; final ConstantsObserver mConstantsObserver; - private static final Comparator sPendingJobComparator = (o1, o2) -> { - // Jobs with an override state set (via adb) should be put first as tests/developers - // expect the jobs to run immediately. - if (o1.overrideState != o2.overrideState) { - // Higher override state (OVERRIDE_FULL) should be before lower state (OVERRIDE_SOFT) - return o2.overrideState - o1.overrideState; - } - if (o1.getSourceUid() == o2.getSourceUid()) { - final boolean o1FGJ = o1.isRequestedExpeditedJob(); - if (o1FGJ != o2.isRequestedExpeditedJob()) { - // Attempt to run requested expedited jobs ahead of regular jobs, regardless of - // expedited job quota. - return o1FGJ ? -1 : 1; + @VisibleForTesting + class PendingJobComparator implements Comparator { + private final SparseBooleanArray mUidHasEjCache = new SparseBooleanArray(); + private final SparseLongArray mEarliestRegEnqueueTimeCache = new SparseLongArray(); + + /** + * Refresh sorting determinants based on the current state of {@link #mPendingJobs}. + */ + @GuardedBy("mLock") + @VisibleForTesting + void refreshLocked() { + mUidHasEjCache.clear(); + mEarliestRegEnqueueTimeCache.clear(); + for (int i = 0; i < mPendingJobs.size(); ++i) { + final JobStatus job = mPendingJobs.get(i); + final int uid = job.getSourceUid(); + if (job.isRequestedExpeditedJob()) { + mUidHasEjCache.put(uid, true); + } else { + final long earliestEnqueueTime = + mEarliestRegEnqueueTimeCache.get(uid, Long.MAX_VALUE); + mEarliestRegEnqueueTimeCache.put(uid, + Math.min(earliestEnqueueTime, job.enqueueTime)); + } } } - if (o1.enqueueTime < o2.enqueueTime) { - return -1; + + @Override + public int compare(JobStatus o1, JobStatus o2) { + if (o1 == o2) { + return 0; + } + // Jobs with an override state set (via adb) should be put first as tests/developers + // expect the jobs to run immediately. + if (o1.overrideState != o2.overrideState) { + // Higher override state (OVERRIDE_FULL) should be before lower state + // (OVERRIDE_SOFT) + return o2.overrideState - o1.overrideState; + } + final boolean o1EJ = o1.isRequestedExpeditedJob(); + final boolean o2EJ = o2.isRequestedExpeditedJob(); + if (o1.getSourceUid() == o2.getSourceUid()) { + if (o1EJ != o2EJ) { + // Attempt to run requested expedited jobs ahead of regular jobs, regardless of + // expedited job quota. + return o1EJ ? -1 : 1; + } + } + final boolean uid1HasEj = mUidHasEjCache.get(o1.getSourceUid()); + final boolean uid2HasEj = mUidHasEjCache.get(o2.getSourceUid()); + if ((uid1HasEj || uid2HasEj) && (o1EJ || o2EJ)) { + // We MUST prioritize EJs ahead of regular jobs within a single app. Since we do + // that, in order to satisfy the transitivity constraint of the comparator, if + // any UID has an EJ, we must ensure that the EJ is ordered ahead of the regular + // job of a different app IF the app with an EJ had another job that came before + // the differing app. For example, if app A has regJob1 at t1 and eJob3 at t3 and + // app B has regJob2 at t2, eJob3 must be ordered before regJob2 because it will be + // ordered before regJob1. + // Regular jobs don't need to jump the line. + + final long uid1EarliestRegEnqueueTime = Math.min(o1.enqueueTime, + mEarliestRegEnqueueTimeCache.get(o1.getSourceUid(), Long.MAX_VALUE)); + final long uid2EarliestRegEnqueueTime = Math.min(o2.enqueueTime, + mEarliestRegEnqueueTimeCache.get(o2.getSourceUid(), Long.MAX_VALUE)); + + if (o1EJ && o2EJ) { + if (uid1EarliestRegEnqueueTime < uid2EarliestRegEnqueueTime) { + return -1; + } else if (uid1EarliestRegEnqueueTime > uid2EarliestRegEnqueueTime) { + return 1; + } + } else if (o1EJ && uid1EarliestRegEnqueueTime < o2.enqueueTime) { + return -1; + } else if (o2EJ && uid2EarliestRegEnqueueTime < o1.enqueueTime) { + return 1; + } + } + if (o1.enqueueTime < o2.enqueueTime) { + return -1; + } + return o1.enqueueTime > o2.enqueueTime ? 1 : 0; } - return o1.enqueueTime > o2.enqueueTime ? 1 : 0; - }; + } + + @VisibleForTesting + final PendingJobComparator mPendingJobComparator = new PendingJobComparator(); static void addOrderedItem(ArrayList array, T newItem, Comparator comparator) { int where = Collections.binarySearch(array, newItem, comparator); @@ -1115,7 +1183,7 @@ public class JobSchedulerService extends com.android.server.SystemService // This is a new job, we can just immediately put it on the pending // list and try to run it. mJobPackageTracker.notePending(jobStatus); - addOrderedItem(mPendingJobs, jobStatus, sPendingJobComparator); + addOrderedItem(mPendingJobs, jobStatus, mPendingJobComparator); maybeRunPendingJobsLocked(); } else { evaluateControllerStatesLocked(jobStatus); @@ -1919,7 +1987,7 @@ public class JobSchedulerService extends com.android.server.SystemService if (js != null) { if (isReadyToBeExecutedLocked(js)) { mJobPackageTracker.notePending(js); - addOrderedItem(mPendingJobs, js, sPendingJobComparator); + addOrderedItem(mPendingJobs, js, mPendingJobComparator); } } else { Slog.e(TAG, "Given null job to check individually"); @@ -2064,6 +2132,7 @@ public class JobSchedulerService extends com.android.server.SystemService * Run through list of jobs and execute all possible - at least one is expired so we do * as many as we can. */ + @GuardedBy("mLock") private void queueReadyJobsForExecutionLocked() { // This method will check and capture all ready jobs, so we don't need to keep any messages // in the queue. @@ -2079,7 +2148,7 @@ public class JobSchedulerService extends com.android.server.SystemService mPendingJobs.clear(); stopNonReadyActiveJobsLocked(); mJobs.forEachJob(mReadyQueueFunctor); - mReadyQueueFunctor.postProcess(); + mReadyQueueFunctor.postProcessLocked(); if (DEBUG) { final int queuedJobs = mPendingJobs.size(); @@ -2106,16 +2175,19 @@ public class JobSchedulerService extends com.android.server.SystemService } } - public void postProcess() { + @GuardedBy("mLock") + private void postProcessLocked() { noteJobsPending(newReadyJobs); mPendingJobs.addAll(newReadyJobs); if (mPendingJobs.size() > 1) { - mPendingJobs.sort(sPendingJobComparator); + mPendingJobComparator.refreshLocked(); + mPendingJobs.sort(mPendingJobComparator); } newReadyJobs.clear(); } } + private final ReadyJobQueueFunctor mReadyQueueFunctor = new ReadyJobQueueFunctor(); /** @@ -2180,7 +2252,9 @@ public class JobSchedulerService extends com.android.server.SystemService } } - public void postProcess() { + @GuardedBy("mLock") + @VisibleForTesting + void postProcessLocked() { if (unbatchedCount > 0 || forceBatchedCount >= mConstants.MIN_READY_NON_ACTIVE_JOBS_COUNT) { if (DEBUG) { @@ -2189,7 +2263,8 @@ public class JobSchedulerService extends com.android.server.SystemService noteJobsPending(runnableJobs); mPendingJobs.addAll(runnableJobs); if (mPendingJobs.size() > 1) { - mPendingJobs.sort(sPendingJobComparator); + mPendingJobComparator.refreshLocked(); + mPendingJobs.sort(mPendingJobComparator); } } else { if (DEBUG) { @@ -2210,6 +2285,7 @@ public class JobSchedulerService extends com.android.server.SystemService } private final MaybeReadyJobQueueFunctor mMaybeQueueFunctor = new MaybeReadyJobQueueFunctor(); + @GuardedBy("mLock") private void maybeQueueReadyJobsForExecutionLocked() { if (DEBUG) Slog.d(TAG, "Maybe queuing ready jobs..."); @@ -2217,7 +2293,7 @@ public class JobSchedulerService extends com.android.server.SystemService mPendingJobs.clear(); stopNonReadyActiveJobsLocked(); mJobs.forEachJob(mMaybeQueueFunctor); - mMaybeQueueFunctor.postProcess(); + mMaybeQueueFunctor.postProcessLocked(); } /** Returns true if both the calling and source users for the job are started. */ diff --git a/services/tests/mockingservicestests/src/com/android/server/job/JobSchedulerServiceTest.java b/services/tests/mockingservicestests/src/com/android/server/job/JobSchedulerServiceTest.java index f2bb47bfb8ad1..b2471fac8b45c 100644 --- a/services/tests/mockingservicestests/src/com/android/server/job/JobSchedulerServiceTest.java +++ b/services/tests/mockingservicestests/src/com/android/server/job/JobSchedulerServiceTest.java @@ -30,6 +30,7 @@ import static com.android.server.job.JobSchedulerService.RARE_INDEX; import static com.android.server.job.JobSchedulerService.sElapsedRealtimeClock; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyInt; @@ -54,6 +55,9 @@ import android.os.Looper; import android.os.RemoteException; import android.os.ServiceManager; import android.os.SystemClock; +import android.util.Log; +import android.util.SparseBooleanArray; +import android.util.SparseLongArray; import com.android.server.AppStateTracker; import com.android.server.AppStateTrackerImpl; @@ -75,8 +79,11 @@ import org.mockito.quality.Strictness; import java.time.Clock; import java.time.Duration; import java.time.ZoneOffset; +import java.util.Random; public class JobSchedulerServiceTest { + private static final String TAG = JobSchedulerServiceTest.class.getSimpleName(); + private JobSchedulerService mService; private MockitoSession mMockingSession; @@ -178,12 +185,21 @@ public class JobSchedulerServiceTest { } private static JobInfo.Builder createJobInfo() { - return new JobInfo.Builder(351, new ComponentName("foo", "bar")); + return createJobInfo(351); + } + + private static JobInfo.Builder createJobInfo(int jobId) { + return new JobInfo.Builder(jobId, new ComponentName("foo", "bar")); } private JobStatus createJobStatus(String testTag, JobInfo.Builder jobInfoBuilder) { + return createJobStatus(testTag, jobInfoBuilder, 1234); + } + + private JobStatus createJobStatus(String testTag, JobInfo.Builder jobInfoBuilder, + int callingUid) { return JobStatus.createFromJobInfo( - jobInfoBuilder.build(), 1234, "com.android.test", 0, testTag); + jobInfoBuilder.build(), callingUid, "com.android.test", 0, testTag); } /** @@ -716,7 +732,7 @@ public class JobSchedulerServiceTest { assertEquals(i + 1, maybeQueueFunctor.runnableJobs.size()); assertEquals(sElapsedRealtimeClock.millis(), job.getFirstForceBatchedTimeElapsed()); } - maybeQueueFunctor.postProcess(); + maybeQueueFunctor.postProcessLocked(); assertEquals(0, mService.mPendingJobs.size()); // Enough RARE jobs to run. @@ -728,7 +744,7 @@ public class JobSchedulerServiceTest { assertEquals(i + 1, maybeQueueFunctor.runnableJobs.size()); assertEquals(sElapsedRealtimeClock.millis(), job.getFirstForceBatchedTimeElapsed()); } - maybeQueueFunctor.postProcess(); + maybeQueueFunctor.postProcessLocked(); assertEquals(5, mService.mPendingJobs.size()); // Not enough RARE jobs to run, but a non-batched job saves the day. @@ -745,7 +761,7 @@ public class JobSchedulerServiceTest { assertEquals(sElapsedRealtimeClock.millis(), job.getFirstForceBatchedTimeElapsed()); } maybeQueueFunctor.accept(activeJob); - maybeQueueFunctor.postProcess(); + maybeQueueFunctor.postProcessLocked(); assertEquals(3, mService.mPendingJobs.size()); // Not enough RARE jobs to run, but an old RARE job saves the day. @@ -764,7 +780,7 @@ public class JobSchedulerServiceTest { } maybeQueueFunctor.accept(oldRareJob); assertEquals(oldBatchTime, oldRareJob.getFirstForceBatchedTimeElapsed()); - maybeQueueFunctor.postProcess(); + maybeQueueFunctor.postProcessLocked(); assertEquals(3, mService.mPendingJobs.size()); } @@ -853,4 +869,207 @@ public class JobSchedulerServiceTest { 0, "")); } } + + @Test + public void testPendingJobSorting() { + // First letter in job variable name indicate regular (r) or expedited (e). + // Capital letters in job variable name indicate the app/UID. + // Numbers in job variable name indicate the enqueue time. + // Expected sort order: + // eA7 > rA1 > eB6 > rB2 > eC3 > rD4 > eE5 > eF9 > rF8 > eC11 > rC10 > rG12 > rG13 > eE14 + // Intentions: + // * A jobs let us test skipping both regular and expedited jobs of other apps + // * B jobs let us test skipping only regular job of another app without going too far + // * C jobs test that regular jobs don't skip over other app's jobs and that EJs only + // skip up to level of the earliest regular job + // * E jobs test that expedited jobs don't skip the line when the app has no regular jobs + // * F jobs test correct expedited/regular ordering doesn't push jobs too high in list + // * G jobs test correct ordering for regular jobs + JobStatus rA1 = createJobStatus("testPendingJobSorting", createJobInfo(1), 1); + JobStatus rB2 = createJobStatus("testPendingJobSorting", createJobInfo(2), 2); + JobStatus eC3 = createJobStatus("testPendingJobSorting", + createJobInfo(3).setExpedited(true), 3); + JobStatus rD4 = createJobStatus("testPendingJobSorting", createJobInfo(4), 4); + JobStatus eE5 = createJobStatus("testPendingJobSorting", + createJobInfo(5).setExpedited(true), 5); + JobStatus eB6 = createJobStatus("testPendingJobSorting", + createJobInfo(6).setExpedited(true), 2); + JobStatus eA7 = createJobStatus("testPendingJobSorting", + createJobInfo(7).setExpedited(true), 1); + JobStatus rF8 = createJobStatus("testPendingJobSorting", createJobInfo(8), 6); + JobStatus eF9 = createJobStatus("testPendingJobSorting", + createJobInfo(9).setExpedited(true), 6); + JobStatus rC10 = createJobStatus("testPendingJobSorting", createJobInfo(10), 3); + JobStatus eC11 = createJobStatus("testPendingJobSorting", + createJobInfo(11).setExpedited(true), 3); + JobStatus rG12 = createJobStatus("testPendingJobSorting", createJobInfo(12), 7); + JobStatus rG13 = createJobStatus("testPendingJobSorting", createJobInfo(13), 7); + JobStatus eE14 = createJobStatus("testPendingJobSorting", + createJobInfo(14).setExpedited(true), 5); + + rA1.enqueueTime = 1; + rB2.enqueueTime = 2; + eC3.enqueueTime = 3; + rD4.enqueueTime = 4; + eE5.enqueueTime = 5; + eB6.enqueueTime = 6; + eA7.enqueueTime = 7; + rF8.enqueueTime = 8; + eF9.enqueueTime = 9; + rC10.enqueueTime = 10; + eC11.enqueueTime = 11; + rG12.enqueueTime = 12; + rG13.enqueueTime = 13; + eE14.enqueueTime = 14; + + mService.mPendingJobs.clear(); + // Add in random order so sorting is apparent. + mService.mPendingJobs.add(eC3); + mService.mPendingJobs.add(eE5); + mService.mPendingJobs.add(rA1); + mService.mPendingJobs.add(rG13); + mService.mPendingJobs.add(rD4); + mService.mPendingJobs.add(eA7); + mService.mPendingJobs.add(rG12); + mService.mPendingJobs.add(rF8); + mService.mPendingJobs.add(eB6); + mService.mPendingJobs.add(eE14); + mService.mPendingJobs.add(eF9); + mService.mPendingJobs.add(rB2); + mService.mPendingJobs.add(rC10); + mService.mPendingJobs.add(eC11); + + mService.mPendingJobComparator.refreshLocked(); + mService.mPendingJobs.sort(mService.mPendingJobComparator); + + final JobStatus[] expectedOrder = new JobStatus[]{ + eA7, rA1, eB6, rB2, eC3, rD4, eE5, eF9, rF8, eC11, rC10, rG12, rG13, eE14}; + for (int i = 0; i < expectedOrder.length; ++i) { + assertEquals("List wasn't correctly sorted @ index " + i, + expectedOrder[i].getJobId(), mService.mPendingJobs.get(i).getJobId()); + } + } + + private void checkPendingJobInvariants() { + long regJobEnqueueTime = 0; + final SparseBooleanArray regJobSeen = new SparseBooleanArray(); + final SparseLongArray ejEnqueueTimes = new SparseLongArray(); + + for (int i = 0; i < mService.mPendingJobs.size(); ++i) { + final JobStatus job = mService.mPendingJobs.get(i); + final int uid = job.getSourceUid(); + + if (!job.isRequestedExpeditedJob()) { + // Invariant #1: Regular jobs are sorted by enqueue time. + assertTrue("Regular job with earlier enqueue time sorted after a later time: " + + regJobEnqueueTime + " vs " + job.enqueueTime, + regJobEnqueueTime <= job.enqueueTime); + regJobEnqueueTime = job.enqueueTime; + regJobSeen.put(uid, true); + } else { + // Invariant #2: EJs should be before regular jobs for an individual app + if (regJobSeen.get(uid)) { + fail("UID " + uid + " had an EJ ordered after a regular job"); + } + final long ejEnqueueTime = ejEnqueueTimes.get(uid, 0); + // Invariant #3: EJs for an individual app should be sorted by enqueue time. + assertTrue("EJ with earlier enqueue time sorted after a later time: " + + ejEnqueueTime + " vs " + job.enqueueTime, + ejEnqueueTime <= job.enqueueTime); + ejEnqueueTimes.put(uid, job.enqueueTime); + } + } + } + + private static String sortedJobToString(JobStatus job) { + return "testJob " + job.getSourceUid() + "/" + job.getJobId() + "/" + + job.isRequestedExpeditedJob() + "@" + job.enqueueTime; + } + + @Test + public void testPendingJobSorting_Random() { + Random random = new Random(1); // Always use the same series of pseudo random values. + + mService.mPendingJobs.clear(); + + for (int i = 0; i < 2500; ++i) { + JobStatus job = createJobStatus("testPendingJobSorting_Random", + createJobInfo(i).setExpedited(random.nextBoolean()), random.nextInt(250)); + job.enqueueTime = Math.abs(random.nextInt(1_000_000)); + mService.mPendingJobs.add(job); + + mService.mPendingJobComparator.refreshLocked(); + try { + mService.mPendingJobs.sort(mService.mPendingJobComparator); + } catch (Exception e) { + for (JobStatus toDump : mService.mPendingJobs) { + Log.i(TAG, sortedJobToString(toDump)); + } + throw e; + } + checkPendingJobInvariants(); + } + } + + private int sign(int i) { + if (i > 0) { + return 1; + } + if (i < 0) { + return -1; + } + return 0; + } + + @Test + public void testPendingJobSortingTransitivity() { + Random random = new Random(1); // Always use the same series of pseudo random values. + + mService.mPendingJobs.clear(); + + for (int i = 0; i < 250; ++i) { + JobStatus job = createJobStatus("testPendingJobSortingTransitivity", + createJobInfo(i).setExpedited(random.nextBoolean()), random.nextInt(50)); + job.enqueueTime = Math.abs(random.nextInt(1_000_000)); + job.overrideState = random.nextInt(4); + mService.mPendingJobs.add(job); + } + + mService.mPendingJobComparator.refreshLocked(); + + for (int i = 0; i < mService.mPendingJobs.size(); ++i) { + final JobStatus job1 = mService.mPendingJobs.get(i); + + for (int j = 0; j < mService.mPendingJobs.size(); ++j) { + final JobStatus job2 = mService.mPendingJobs.get(j); + final int sign12 = sign(mService.mPendingJobComparator.compare(job1, job2)); + final int sign21 = sign(mService.mPendingJobComparator.compare(job2, job1)); + if (sign12 != -sign21) { + final String job1String = sortedJobToString(job1); + final String job2String = sortedJobToString(job2); + fail("compare(" + job1String + ", " + job2String + ") != " + + "-compare(" + job2String + ", " + job1String + ")"); + } + + for (int k = 0; k < mService.mPendingJobs.size(); ++k) { + final JobStatus job3 = mService.mPendingJobs.get(k); + final int sign23 = sign(mService.mPendingJobComparator.compare(job2, job3)); + final int sign13 = sign(mService.mPendingJobComparator.compare(job1, job3)); + + // Confirm 1 < 2 < 3 or 1 > 2 > 3 or 1 == 2 == 3 + if ((sign12 == sign23 && sign12 != sign13) + // Confirm that if 1 == 2, then (1 < 3 AND 2 < 3) OR (1 > 3 && 2 > 3) + || (sign12 == 0 && sign13 != sign23)) { + final String job1String = sortedJobToString(job1); + final String job2String = sortedJobToString(job2); + final String job3String = sortedJobToString(job3); + fail("Transitivity fail" + + ": compare(" + job1String + ", " + job2String + ")=" + sign12 + + ", compare(" + job2String + ", " + job3String + ")=" + sign23 + + ", compare(" + job1String + ", " + job3String + ")=" + sign13); + } + } + } + } + } }