From 0687577f872028df2256f432c45759199e63bbf2 Mon Sep 17 00:00:00 2001 From: Kweku Adams Date: Mon, 26 Jul 2021 12:57:45 -0700 Subject: [PATCH] Address sorting edge case. There are times when the enqueue time of two different jobs are exactly the same. In those cases, if an EJ skipped over one regular job with an enqueue time X, then it should skip all other regular jobs with enqueue time X. Bug: 194324016 Test: atest FrameworksMockingServicesTests:JobSchedulerServiceTest Test: atest CtsJobSchedulerTestCases Change-Id: I77e106676238dd3ae198fab619af430e48d433a2 --- .../server/job/JobSchedulerService.java | 18 +++---- .../server/job/JobSchedulerServiceTest.java | 54 +++++++++++++++---- 2 files changed, 52 insertions(+), 20 deletions(-) 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 78670c7c73b1c..591e8ba859fc8 100644 --- a/apex/jobscheduler/service/java/com/android/server/job/JobSchedulerService.java +++ b/apex/jobscheduler/service/java/com/android/server/job/JobSchedulerService.java @@ -690,7 +690,6 @@ public class JobSchedulerService extends com.android.server.SystemService @VisibleForTesting class PendingJobComparator implements Comparator { - private final SparseBooleanArray mUidHasEjCache = new SparseBooleanArray(); private final SparseLongArray mEarliestRegEnqueueTimeCache = new SparseLongArray(); /** @@ -699,14 +698,11 @@ public class JobSchedulerService extends com.android.server.SystemService @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 { + if (!job.isRequestedExpeditedJob()) { final long earliestEnqueueTime = mEarliestRegEnqueueTimeCache.get(uid, Long.MAX_VALUE); mEarliestRegEnqueueTimeCache.put(uid, @@ -736,9 +732,7 @@ public class JobSchedulerService extends com.android.server.SystemService return o1EJ ? -1 : 1; } } - final boolean uid1HasEj = mUidHasEjCache.get(o1.getSourceUid()); - final boolean uid2HasEj = mUidHasEjCache.get(o2.getSourceUid()); - if ((uid1HasEj || uid2HasEj) && (o1EJ || o2EJ)) { + if (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 @@ -759,9 +753,13 @@ public class JobSchedulerService extends com.android.server.SystemService } else if (uid1EarliestRegEnqueueTime > uid2EarliestRegEnqueueTime) { return 1; } - } else if (o1EJ && uid1EarliestRegEnqueueTime < o2.enqueueTime) { + } else if (o1EJ && uid1EarliestRegEnqueueTime <= o2.enqueueTime) { + // Include = to ensure that if we sorted an EJ ahead of a regular job at time X + // then we make sure to sort it ahead of all regular jobs at time X. return -1; - } else if (o2EJ && uid2EarliestRegEnqueueTime < o1.enqueueTime) { + } else if (o2EJ && uid2EarliestRegEnqueueTime <= o1.enqueueTime) { + // Include = to ensure that if we sorted an EJ ahead of a regular job at time X + // then we make sure to sort it ahead of all regular jobs at time X. return 1; } } 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 b2471fac8b45c..56bcb834dbfde 100644 --- a/services/tests/mockingservicestests/src/com/android/server/job/JobSchedulerServiceTest.java +++ b/services/tests/mockingservicestests/src/com/android/server/job/JobSchedulerServiceTest.java @@ -885,6 +885,7 @@ public class JobSchedulerServiceTest { // * 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 + // * H job tests correct behavior when enqueue times are the same JobStatus rA1 = createJobStatus("testPendingJobSorting", createJobInfo(1), 1); JobStatus rB2 = createJobStatus("testPendingJobSorting", createJobInfo(2), 2); JobStatus eC3 = createJobStatus("testPendingJobSorting", @@ -896,6 +897,7 @@ public class JobSchedulerServiceTest { createJobInfo(6).setExpedited(true), 2); JobStatus eA7 = createJobStatus("testPendingJobSorting", createJobInfo(7).setExpedited(true), 1); + JobStatus rH8 = createJobStatus("testPendingJobSorting", createJobInfo(8), 14); JobStatus rF8 = createJobStatus("testPendingJobSorting", createJobInfo(8), 6); JobStatus eF9 = createJobStatus("testPendingJobSorting", createJobInfo(9).setExpedited(true), 6); @@ -915,6 +917,7 @@ public class JobSchedulerServiceTest { eB6.enqueueTime = 6; eA7.enqueueTime = 7; rF8.enqueueTime = 8; + rH8.enqueueTime = 8; eF9.enqueueTime = 9; rC10.enqueueTime = 10; eC11.enqueueTime = 11; @@ -931,6 +934,7 @@ public class JobSchedulerServiceTest { mService.mPendingJobs.add(rD4); mService.mPendingJobs.add(eA7); mService.mPendingJobs.add(rG12); + mService.mPendingJobs.add(rH8); mService.mPendingJobs.add(rF8); mService.mPendingJobs.add(eB6); mService.mPendingJobs.add(eE14); @@ -943,7 +947,7 @@ public class JobSchedulerServiceTest { mService.mPendingJobs.sort(mService.mPendingJobComparator); final JobStatus[] expectedOrder = new JobStatus[]{ - eA7, rA1, eB6, rB2, eC3, rD4, eE5, eF9, rF8, eC11, rC10, rG12, rG13, eE14}; + eA7, rA1, eB6, rB2, eC3, rD4, eE5, eF9, rH8, 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()); @@ -995,7 +999,7 @@ public class JobSchedulerServiceTest { 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)); + job.enqueueTime = random.nextInt(1_000_000); mService.mPendingJobs.add(job); mService.mPendingJobComparator.refreshLocked(); @@ -1023,18 +1027,48 @@ public class JobSchedulerServiceTest { @Test public void testPendingJobSortingTransitivity() { - Random random = new Random(1); // Always use the same series of pseudo random values. + // Always use the same series of pseudo random values. + for (int seed : new int[]{1337, 7357, 606, 6357, 41106010, 3, 2, 1}) { + Random random = new Random(seed); - mService.mPendingJobs.clear(); + 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); + for (int i = 0; i < 300; ++i) { + JobStatus job = createJobStatus("testPendingJobSortingTransitivity", + createJobInfo(i).setExpedited(random.nextBoolean()), random.nextInt(50)); + job.enqueueTime = random.nextInt(1_000_000); + job.overrideState = random.nextInt(4); + mService.mPendingJobs.add(job); + } + + verifyPendingJobComparatorTransitivity(); } + } + @Test + public void testPendingJobSortingTransitivity_Concentrated() { + // Always use the same series of pseudo random values. + for (int seed : new int[]{1337, 6000, 637739, 6357, 1, 7, 13}) { + Random random = new Random(seed); + + mService.mPendingJobs.clear(); + + for (int i = 0; i < 300; ++i) { + JobStatus job = createJobStatus("testPendingJobSortingTransitivity_Concentrated", + createJobInfo(i).setExpedited(random.nextFloat() < .03), + random.nextInt(20)); + job.enqueueTime = random.nextInt(250); + job.overrideState = random.nextFloat() < .01 + ? JobStatus.OVERRIDE_SORTING : JobStatus.OVERRIDE_NONE; + mService.mPendingJobs.add(job); + Log.d(TAG, sortedJobToString(job)); + } + + verifyPendingJobComparatorTransitivity(); + } + } + + private void verifyPendingJobComparatorTransitivity() { mService.mPendingJobComparator.refreshLocked(); for (int i = 0; i < mService.mPendingJobs.size(); ++i) {