Merge "Fix pending job sorting." into sc-dev

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<JobStatus> sPendingJobComparator = (o1, o2) -> {
+    @VisibleForTesting
-        // Jobs with an override state set (via adb) should be put first as tests/developers
+    class PendingJobComparator implements Comparator<JobStatus> {
-        // expect the jobs to run immediately.
+        private final SparseBooleanArray mUidHasEjCache = new SparseBooleanArray();
-        if (o1.overrideState != o2.overrideState) {
+        private final SparseLongArray mEarliestRegEnqueueTimeCache = new SparseLongArray();
-            // Higher override state (OVERRIDE_FULL) should be before lower state (OVERRIDE_SOFT)
+
-            return o2.overrideState - o1.overrideState;
+        /**
-        }
+         * Refresh sorting determinants based on the current state of {@link #mPendingJobs}.
-        if (o1.getSourceUid() == o2.getSourceUid()) {
+         */
-            final boolean o1FGJ = o1.isRequestedExpeditedJob();
+        @GuardedBy("mLock")
-            if (o1FGJ != o2.isRequestedExpeditedJob()) {
+        @VisibleForTesting
-                // Attempt to run requested expedited jobs ahead of regular jobs, regardless of
+        void refreshLocked() {
-                // expedited job quota.
+            mUidHasEjCache.clear();
-                return o1FGJ ? -1 : 1;
+            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 <T> void addOrderedItem(ArrayList<T> array, T newItem, Comparator<T> 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. */

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);
+                    }
+                }
+            }
+        }
+    }
 }