From 09e1810c64af345028d96462e6eeebbfadc82944 Mon Sep 17 00:00:00 2001 From: Kweku Adams Date: Tue, 22 Mar 2022 17:28:07 +0000 Subject: [PATCH] Make sure all jobs are sorted by override state. Override state is only set via adb, which means its used for testing job execution. Make sure we put jobs under test at the front of the queue to avoid issues with execution delay. Bug: 141645789 Bug: 223437753 Test: atest FrameworksServicesTests:PendingJobQueueTest Change-Id: Icc4788117c46ada49c030f82f26affbd56e6ef69 --- .../android/server/job/PendingJobQueue.java | 30 +++++++++++------ .../server/job/PendingJobQueueTest.java | 32 ++++++++----------- 2 files changed, 34 insertions(+), 28 deletions(-) diff --git a/apex/jobscheduler/service/java/com/android/server/job/PendingJobQueue.java b/apex/jobscheduler/service/java/com/android/server/job/PendingJobQueue.java index 993e178de0eef..e3426743c0a4d 100644 --- a/apex/jobscheduler/service/java/com/android/server/job/PendingJobQueue.java +++ b/apex/jobscheduler/service/java/com/android/server/job/PendingJobQueue.java @@ -54,6 +54,13 @@ class PendingJobQueue { } else if (t2 == AppJobQueue.NO_NEXT_TIMESTAMP) { return -1; } + final int o1 = ajq1.peekNextOverrideState(); + final int o2 = ajq2.peekNextOverrideState(); + if (o1 != o2) { + // Higher override state (OVERRIDE_FULL) should be before lower state + // (OVERRIDE_SOFT) + return Integer.compare(o2, o1); + } return Long.compare(t1, t2); }); @@ -185,6 +192,7 @@ class PendingJobQueue { private static final class AppJobQueue { static final long NO_NEXT_TIMESTAMP = -1L; + static final int NO_NEXT_OVERRIDE_STATE = -1; private static class AdjustedJobStatus { public long adjustedEnqueueTime; @@ -207,7 +215,7 @@ class PendingJobQueue { if (job1.overrideState != job2.overrideState) { // Higher override state (OVERRIDE_FULL) should be before lower state // (OVERRIDE_SOFT) - return job2.overrideState - job1.overrideState; + return Integer.compare(job2.overrideState, job1.overrideState); } final boolean job1EJ = job1.isRequestedExpeditedJob(); @@ -223,18 +231,15 @@ class PendingJobQueue { if (job1Priority != job2Priority) { // Use the priority set by an app for intra-app job ordering. Higher // priority should be before lower priority. - return job2Priority - job1Priority; + return Integer.compare(job2Priority, job1Priority); } if (job1.lastEvaluatedBias != job2.lastEvaluatedBias) { // Higher bias should go first. - return job2.lastEvaluatedBias - job1.lastEvaluatedBias; + return Integer.compare(job2.lastEvaluatedBias, job1.lastEvaluatedBias); } - if (job1.enqueueTime < job2.enqueueTime) { - return -1; - } - return job1.enqueueTime > job2.enqueueTime ? 1 : 0; + return Long.compare(job1.enqueueTime, job2.enqueueTime); }; private static final Pools.Pool mAdjustedJobStatusPool = @@ -344,9 +349,14 @@ class PendingJobQueue { if (mCurIndex >= mJobs.size()) { return null; } - JobStatus next = mJobs.get(mCurIndex).job; - mCurIndex++; - return next; + return mJobs.get(mCurIndex++).job; + } + + int peekNextOverrideState() { + if (mCurIndex >= mJobs.size()) { + return NO_NEXT_OVERRIDE_STATE; + } + return mJobs.get(mCurIndex).job.overrideState; } long peekNextTimestamp() { diff --git a/services/tests/servicestests/src/com/android/server/job/PendingJobQueueTest.java b/services/tests/servicestests/src/com/android/server/job/PendingJobQueueTest.java index 150e3c60b91ee..bce6b1b2487e6 100644 --- a/services/tests/servicestests/src/com/android/server/job/PendingJobQueueTest.java +++ b/services/tests/servicestests/src/com/android/server/job/PendingJobQueueTest.java @@ -28,7 +28,6 @@ import android.platform.test.annotations.LargeTest; import android.util.Log; import android.util.SparseArray; import android.util.SparseBooleanArray; -import android.util.SparseIntArray; import android.util.SparseLongArray; import com.android.server.job.controllers.JobStatus; @@ -371,38 +370,34 @@ public class PendingJobQueueTest { private void checkPendingJobInvariants(PendingJobQueue jobQueue) { final SparseBooleanArray regJobSeen = new SparseBooleanArray(); - final SparseIntArray lastOverrideStateSeen = new SparseIntArray(); // Latest priority enqueue times seen for each priority for each app. final SparseArray latestPriorityRegEnqueueTimesPerUid = new SparseArray<>(); final SparseArray latestPriorityEjEnqueueTimesPerUid = new SparseArray<>(); final int noEntry = -1; + int prevOverrideState = noEntry; JobStatus job; jobQueue.resetIterator(); while ((job = jobQueue.next()) != null) { final int uid = job.getSourceUid(); - // Invariant #1: All jobs (for a UID) are sorted by override state + // Invariant #1: All jobs are sorted by override state // Invariant #2: All jobs (for a UID) are sorted by priority order // Invariant #3: Jobs (for a UID) with the same priority are sorted by enqueue time. // Invariant #4: EJs (for a UID) should be before regular jobs - final int prevOverrideState = lastOverrideStateSeen.get(uid, noEntry); - lastOverrideStateSeen.put(uid, job.overrideState); - if (prevOverrideState == noEntry) { - // First job for UID - continue; - } - // Invariant 1 if (prevOverrideState != job.overrideState) { - assertTrue(prevOverrideState > job.overrideState); - // Override state can make ordering weird. Clear the other cached states for this - // UID to avoid confusion in the other checks. - latestPriorityEjEnqueueTimesPerUid.remove(uid); - latestPriorityRegEnqueueTimesPerUid.remove(uid); - regJobSeen.delete(uid); + if (prevOverrideState != noEntry) { + assertTrue(prevOverrideState > job.overrideState); + } + // Override state can make ordering weird. Clear the other cached states + // to avoid confusion in the other checks. + latestPriorityEjEnqueueTimesPerUid.clear(); + latestPriorityRegEnqueueTimesPerUid.clear(); + regJobSeen.clear(); + prevOverrideState = job.overrideState; } final int priority = job.getEffectivePriority(); @@ -423,8 +418,9 @@ public class PendingJobQueueTest { final long lastSeenPriorityEnqueueTime = latestPriorityEnqueueTimes.get(priority, noEntry); if (lastSeenPriorityEnqueueTime != noEntry) { - assertTrue("Jobs with same priority not sorted by enqueue time: " - + lastSeenPriorityEnqueueTime + " vs " + job.enqueueTime, + assertTrue("Jobs with same priority for uid " + uid + + " not sorted by enqueue time: " + + lastSeenPriorityEnqueueTime + " before " + job.enqueueTime, lastSeenPriorityEnqueueTime <= job.enqueueTime); } } else {