Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 2700aba1 authored by Kweku Adams's avatar Kweku Adams
Browse files

Fix pending job sorting. 

We want to always run an app's expedited jobs before its own regular
jobs. This means that we order an app's EJs before its regular jobs. In
order to satisfy the transitivity constraint, we must also order one
app's EJs ahead of a differing app's jobs iff we had to order the first
app's EJ ahead of a job that was earlier than the second app's jobs.

The current sorting system fails the transitivity requirement.

Test case: regJob1 enqueued at t1 for UID A, regJob2 enqueued at t2 for
UID B, EJ3 enqueued at t3 for UID A. Because we expedite EJs, EJ3 is
ordered before regJob1. Because we sort jobs of differing apps by enqueue
time, regJob1 comes before regJob2 and regJob2 comes before EJ3. This
violates transitivity.

The new sorting system will order a later EJ ahead of a differing app's
jobs iff the first app has a regular job that is earlier than the
differing app's first regular job.

Bug: 191592712
Test: atest FrameworksMockingServicesTests:JobSchedulerServiceTest
Test: atest CtsJobSchedulerTestCases
Change-Id: I5391a4f35269d7d43eefc1954788f9df160b1d22
parent dc9a03db

apex/jobscheduler/service/java/com/android/server/job/JobSchedulerService.java

+101 −25
Original line number Diff line number Diff line
@@ -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

    class PendingJobComparator implements Comparator<JobStatus> {

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

                }

            }

        }



        @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)

                // 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()) {

            final boolean o1FGJ = o1.isRequestedExpeditedJob();

            if (o1FGJ != o2.isRequestedExpeditedJob()) {

                if (o1EJ != o2EJ) {

                    // Attempt to run requested expedited jobs ahead of regular jobs, regardless of

                    // expedited job quota.

                return o1FGJ ? -1 : 1;

                    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;

    };

        }

    }



    @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

+225 −6
Original line number Diff line number Diff line
@@ -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);

                    }

                }

            }

        }

    }

}