From 00bc472c02eb8351899c913244b8b95bd07829c6 Mon Sep 17 00:00:00 2001 From: Dan Shi Date: Sun, 2 Apr 2023 06:47:53 +0000 Subject: [PATCH] Revert "More robust updating of "runnable" list." This reverts commit 7529b207666881c747c2685e4279a4cea6e0a581. Reason for revert: b/276599346 possibly slow down coverage build boot time Change-Id: I87dbf23f4ed9dbadaa947f01edbb92a4a01e3dca --- .../server/am/BroadcastProcessQueue.java | 83 ++++--------------- .../server/am/BroadcastQueueModernImpl.java | 54 ++++-------- .../am/BroadcastQueueModernImplTest.java | 14 ++-- 3 files changed, 39 insertions(+), 112 deletions(-) diff --git a/services/core/java/com/android/server/am/BroadcastProcessQueue.java b/services/core/java/com/android/server/am/BroadcastProcessQueue.java index 23eaae23e99f8..5140f9056e119 100644 --- a/services/core/java/com/android/server/am/BroadcastProcessQueue.java +++ b/services/core/java/com/android/server/am/BroadcastProcessQueue.java @@ -21,7 +21,6 @@ import static com.android.server.am.BroadcastRecord.deliveryStateToString; import static com.android.server.am.BroadcastRecord.isDeliveryStateTerminal; import static com.android.server.am.BroadcastRecord.isReceiverEquals; -import android.annotation.CheckResult; import android.annotation.IntDef; import android.annotation.NonNull; import android.annotation.Nullable; @@ -342,12 +341,7 @@ class BroadcastProcessQueue { * Predicates that choose to remove a broadcast must finish * delivery of the matched broadcast, to ensure that situations like ordered * broadcasts are handled consistently. - * - * @return if this operation may have changed internal state, indicating - * that the caller is responsible for invoking - * {@link BroadcastQueueModernImpl#updateRunnableList} */ - @CheckResult public boolean forEachMatchingBroadcast(@NonNull BroadcastPredicate predicate, @NonNull BroadcastConsumer consumer, boolean andRemove) { boolean didSomething = false; @@ -360,7 +354,6 @@ class BroadcastProcessQueue { return didSomething; } - @CheckResult private boolean forEachMatchingBroadcastInQueue(@NonNull ArrayDeque queue, @NonNull BroadcastPredicate predicate, @NonNull BroadcastConsumer consumer, boolean andRemove) { @@ -377,10 +370,6 @@ class BroadcastProcessQueue { args.recycle(); it.remove(); onBroadcastDequeued(record, recordIndex, recordWouldBeSkipped); - } else { - // Even if we're leaving broadcast in queue, it may have - // been mutated in such a way to change our runnable time - invalidateRunnableAt(); } didSomething = true; } @@ -392,43 +381,30 @@ class BroadcastProcessQueue { /** * Update the actively running "warm" process for this process. - * - * @return if this operation may have changed internal state, indicating - * that the caller is responsible for invoking - * {@link BroadcastQueueModernImpl#updateRunnableList} */ - @CheckResult - public boolean setProcessAndUidFrozen(@Nullable ProcessRecord app, boolean uidFrozen) { + public void setProcess(@Nullable ProcessRecord app) { this.app = app; + if (app != null) { + setProcessInstrumented(app.getActiveInstrumentation() != null); + setProcessPersistent(app.isPersistent()); + } else { + setProcessInstrumented(false); + setProcessPersistent(false); + } // Since we may have just changed our PID, invalidate cached strings mCachedToString = null; mCachedToShortString = null; - - boolean didSomething = false; - if (app != null) { - didSomething |= setProcessInstrumented(app.getActiveInstrumentation() != null); - didSomething |= setProcessPersistent(app.isPersistent()); - didSomething |= setUidFrozen(uidFrozen); - } else { - didSomething |= setProcessInstrumented(false); - didSomething |= setProcessPersistent(false); - didSomething |= setUidFrozen(uidFrozen); - } - return didSomething; } /** * Update if this UID is in the "frozen" state, typically signaling that * broadcast dispatch should be paused or delayed. */ - private boolean setUidFrozen(boolean frozen) { + public void setUidFrozen(boolean frozen) { if (mUidFrozen != frozen) { mUidFrozen = frozen; invalidateRunnableAt(); - return true; - } else { - return false; } } @@ -437,13 +413,10 @@ class BroadcastProcessQueue { * signaling that broadcast dispatch should bypass all pauses or delays, to * avoid holding up test suites. */ - private boolean setProcessInstrumented(boolean instrumented) { + public void setProcessInstrumented(boolean instrumented) { if (mProcessInstrumented != instrumented) { mProcessInstrumented = instrumented; invalidateRunnableAt(); - return true; - } else { - return false; } } @@ -451,13 +424,10 @@ class BroadcastProcessQueue { * Update if this process is in the "persistent" state, which signals broadcast dispatch should * bypass all pauses or delays to prevent the system from becoming out of sync with itself. */ - private boolean setProcessPersistent(boolean persistent) { + public void setProcessPersistent(boolean persistent) { if (mProcessPersistent != persistent) { mProcessPersistent = persistent; invalidateRunnableAt(); - return true; - } else { - return false; } } @@ -677,20 +647,8 @@ class BroadcastProcessQueue { return mActive != null; } - /** - * @return if this operation may have changed internal state, indicating - * that the caller is responsible for invoking - * {@link BroadcastQueueModernImpl#updateRunnableList} - */ - @CheckResult - boolean forceDelayBroadcastDelivery(long delayedDurationMs) { - if (mForcedDelayedDurationMs != delayedDurationMs) { - mForcedDelayedDurationMs = delayedDurationMs; - invalidateRunnableAt(); - return true; - } else { - return false; - } + void forceDelayBroadcastDelivery(long delayedDurationMs) { + mForcedDelayedDurationMs = delayedDurationMs; } /** @@ -762,21 +720,10 @@ class BroadcastProcessQueue { * broadcasts would be prioritized for dispatching, even if there are urgent broadcasts * waiting. This is typically used in case there are callers waiting for "barrier" to be * reached. - * - * @return if this operation may have changed internal state, indicating - * that the caller is responsible for invoking - * {@link BroadcastQueueModernImpl#updateRunnableList} */ - @CheckResult @VisibleForTesting - boolean setPrioritizeEarliest(boolean prioritizeEarliest) { - if (mPrioritizeEarliest != prioritizeEarliest) { - mPrioritizeEarliest = prioritizeEarliest; - invalidateRunnableAt(); - return true; - } else { - return false; - } + void setPrioritizeEarliest(boolean prioritizeEarliest) { + mPrioritizeEarliest = prioritizeEarliest; } /** diff --git a/services/core/java/com/android/server/am/BroadcastQueueModernImpl.java b/services/core/java/com/android/server/am/BroadcastQueueModernImpl.java index e35f8226d3b1a..e574b490f3fd0 100644 --- a/services/core/java/com/android/server/am/BroadcastQueueModernImpl.java +++ b/services/core/java/com/android/server/am/BroadcastQueueModernImpl.java @@ -360,6 +360,8 @@ class BroadcastQueueModernImpl extends BroadcastQueue { // If app isn't running, and there's nothing in the queue, clean up if (queue.isEmpty() && !queue.isActive() && !queue.isProcessWarm()) { removeProcessQueue(queue.processName, queue.uid); + } else { + updateQueueDeferred(queue); } } @@ -494,7 +496,8 @@ class BroadcastQueueModernImpl extends BroadcastQueue { // relevant per-process queue final BroadcastProcessQueue queue = getProcessQueue(app); if (queue != null) { - setQueueProcess(queue, app); + queue.setProcess(app); + queue.setUidFrozen(mUidFrozen.get(queue.uid, false)); } boolean didSomething = false; @@ -535,7 +538,8 @@ class BroadcastQueueModernImpl extends BroadcastQueue { // relevant per-process queue final BroadcastProcessQueue queue = getProcessQueue(app); if (queue != null) { - setQueueProcess(queue, null); + queue.setProcess(null); + queue.setUidFrozen(mUidFrozen.get(queue.uid, false)); } if ((mRunningColdStart != null) && (mRunningColdStart == queue)) { @@ -562,7 +566,6 @@ class BroadcastQueueModernImpl extends BroadcastQueue { return (r.receivers.get(i) instanceof BroadcastFilter); }, mBroadcastConsumerSkip, true); if (didSomething || queue.isEmpty()) { - updateQueueDeferred(queue); updateRunnableList(queue); enqueueUpdateRunningList(); } @@ -628,7 +631,6 @@ class BroadcastQueueModernImpl extends BroadcastQueue { setDeliveryState(queue, null, r, i, receiver, BroadcastRecord.DELIVERY_DEFERRED, "deferred at enqueue time"); } - updateQueueDeferred(queue); updateRunnableList(queue); enqueueUpdateRunningList(); } @@ -1077,7 +1079,6 @@ class BroadcastQueueModernImpl extends BroadcastQueue { final int queueIndex = getRunningIndexOf(queue); mRunning[queueIndex] = null; - updateQueueDeferred(queue); updateRunnableList(queue); enqueueUpdateRunningList(); @@ -1160,7 +1161,6 @@ class BroadcastQueueModernImpl extends BroadcastQueue { getReceiverUid(otherReceiver)); if (otherQueue != null) { otherQueue.invalidateRunnableAt(); - updateQueueDeferred(otherQueue); updateRunnableList(otherQueue); } } @@ -1291,7 +1291,6 @@ class BroadcastQueueModernImpl extends BroadcastQueue { if (queuePredicate.test(leaf)) { if (leaf.forEachMatchingBroadcast(broadcastPredicate, broadcastConsumer, andRemove)) { - updateQueueDeferred(leaf); updateRunnableList(leaf); didSomething = true; } @@ -1305,40 +1304,29 @@ class BroadcastQueueModernImpl extends BroadcastQueue { return didSomething; } - private boolean forEachMatchingQueue( + private void forEachMatchingQueue( @NonNull Predicate queuePredicate, @NonNull Consumer queueConsumer) { - boolean didSomething = false; for (int i = mProcessQueues.size() - 1; i >= 0; i--) { BroadcastProcessQueue leaf = mProcessQueues.valueAt(i); while (leaf != null) { if (queuePredicate.test(leaf)) { queueConsumer.accept(leaf); - updateQueueDeferred(leaf); updateRunnableList(leaf); - didSomething = true; } leaf = leaf.processNameNext; } } - if (didSomething) { - enqueueUpdateRunningList(); - } - return didSomething; } - @SuppressWarnings("CheckResult") - private void updateQueueDeferred(@NonNull BroadcastProcessQueue leaf) { + private void updateQueueDeferred( + @NonNull BroadcastProcessQueue leaf) { if (leaf.isDeferredUntilActive()) { - // We ignore the returned value here since callers are invoking us - // just before updateRunnableList() leaf.forEachMatchingBroadcast((r, i) -> { return r.deferUntilActive && (r.getDeliveryState(i) == BroadcastRecord.DELIVERY_PENDING); }, mBroadcastConsumerDefer, false); } else if (leaf.hasDeferredBroadcasts()) { - // We ignore the returned value here since callers are invoking us - // just before updateRunnableList() leaf.forEachMatchingBroadcast((r, i) -> { return r.deferUntilActive && (r.getDeliveryState(i) == BroadcastRecord.DELIVERY_DEFERRED); @@ -1368,7 +1356,10 @@ class BroadcastQueueModernImpl extends BroadcastQueue { while (leaf != null) { // Update internal state by refreshing values previously // read from any known running process - setQueueProcess(leaf, leaf.app); + leaf.setProcess(leaf.app); + leaf.setUidFrozen(frozen); + updateQueueDeferred(leaf); + updateRunnableList(leaf); leaf = leaf.processNameNext; } enqueueUpdateRunningList(); @@ -1529,20 +1520,8 @@ class BroadcastQueueModernImpl extends BroadcastQueue { private void updateWarmProcess(@NonNull BroadcastProcessQueue queue) { if (!queue.isProcessWarm()) { - setQueueProcess(queue, mService.getProcessRecordLocked(queue.processName, queue.uid)); - } - } - - /** - * Update the {@link ProcessRecord} associated with the given - * {@link BroadcastProcessQueue}. Also updates any runnable status that - * might have changed as a side-effect. - */ - private void setQueueProcess(@NonNull BroadcastProcessQueue queue, - @Nullable ProcessRecord app) { - if (queue.setProcessAndUidFrozen(app, mUidFrozen.get(queue.uid, false))) { - updateQueueDeferred(queue); - updateRunnableList(queue); + queue.setProcess(mService.getProcessRecordLocked(queue.processName, queue.uid)); + queue.setUidFrozen(mUidFrozen.get(queue.uid, false)); } } @@ -1736,7 +1715,8 @@ class BroadcastQueueModernImpl extends BroadcastQueue { } BroadcastProcessQueue created = new BroadcastProcessQueue(mConstants, processName, uid); - setQueueProcess(created, mService.getProcessRecordLocked(processName, uid)); + created.setProcess(mService.getProcessRecordLocked(processName, uid)); + created.setUidFrozen(mUidFrozen.get(uid, false)); if (leaf == null) { mProcessQueues.put(uid, created); diff --git a/services/tests/mockingservicestests/src/com/android/server/am/BroadcastQueueModernImplTest.java b/services/tests/mockingservicestests/src/com/android/server/am/BroadcastQueueModernImplTest.java index 74b68e7f3e0fd..c68db4f673372 100644 --- a/services/tests/mockingservicestests/src/com/android/server/am/BroadcastQueueModernImplTest.java +++ b/services/tests/mockingservicestests/src/com/android/server/am/BroadcastQueueModernImplTest.java @@ -371,9 +371,9 @@ public final class BroadcastQueueModernImplTest { List.of(makeMockRegisteredReceiver()), false); queue.enqueueOrReplaceBroadcast(airplaneRecord, 0, false); - queue.setProcessAndUidFrozen(null, false); + queue.setUidFrozen(false); final long notCachedRunnableAt = queue.getRunnableAt(); - queue.setProcessAndUidFrozen(null, true); + queue.setUidFrozen(true); final long cachedRunnableAt = queue.getRunnableAt(); assertThat(cachedRunnableAt).isGreaterThan(notCachedRunnableAt); assertFalse(queue.isRunnable()); @@ -398,9 +398,9 @@ public final class BroadcastQueueModernImplTest { List.of(makeMockRegisteredReceiver()), false); queue.enqueueOrReplaceBroadcast(airplaneRecord, 0, false); - queue.setProcessAndUidFrozen(null, false); + queue.setUidFrozen(false); final long notCachedRunnableAt = queue.getRunnableAt(); - queue.setProcessAndUidFrozen(null, true); + queue.setUidFrozen(true); final long cachedRunnableAt = queue.getRunnableAt(); assertThat(cachedRunnableAt).isGreaterThan(notCachedRunnableAt); assertTrue(queue.isRunnable()); @@ -430,13 +430,13 @@ public final class BroadcastQueueModernImplTest { // verify that: // (a) the queue is immediately runnable by existence of a fg-priority broadcast // (b) the next one up is the fg-priority broadcast despite its later enqueue time - queue.setProcessAndUidFrozen(null, false); + queue.setUidFrozen(false); assertTrue(queue.isRunnable()); assertThat(queue.getRunnableAt()).isAtMost(airplaneRecord.enqueueClockTime); assertEquals(ProcessList.SCHED_GROUP_DEFAULT, queue.getPreferredSchedulingGroupLocked()); assertEquals(queue.peekNextBroadcastRecord(), airplaneRecord); - queue.setProcessAndUidFrozen(null, true); + queue.setUidFrozen(true); assertTrue(queue.isRunnable()); assertThat(queue.getRunnableAt()).isAtMost(airplaneRecord.enqueueClockTime); assertEquals(ProcessList.SCHED_GROUP_DEFAULT, queue.getPreferredSchedulingGroupLocked()); @@ -499,7 +499,7 @@ public final class BroadcastQueueModernImplTest { private void doRunnableAt_Frozen(BroadcastRecord testRecord, int testRunnableAtReason) { final BroadcastProcessQueue queue = new BroadcastProcessQueue(mConstants, PACKAGE_GREEN, getUidForPackage(PACKAGE_GREEN)); - queue.setProcessAndUidFrozen(null, true); + queue.setUidFrozen(true); final BroadcastRecord lazyRecord = makeBroadcastRecord( new Intent(Intent.ACTION_AIRPLANE_MODE_CHANGED),