From dd09e2ad7033da293b9d278147d625bfaa8561cc Mon Sep 17 00:00:00 2001 From: Jeff Sharkey Date: Thu, 20 Oct 2022 17:03:55 -0600 Subject: [PATCH] BroadcastQueue: only OOM adjust for manifest. The legacy stack was purposefully lax about OOM adjusting, by only requesting it when delivering to manifest receivers. This change mirrors that behavior in the modern stack, with tests to confirm. Bug: 254720487 Test: atest FrameworksMockingServicesTests:BroadcastRecordTest Test: atest FrameworksMockingServicesTests:BroadcastQueueTest Test: atest FrameworksMockingServicesTests:BroadcastQueueModernImplTest Change-Id: I954d024d15493f02551c6e4a655d76339512430a --- .../server/am/BroadcastProcessQueue.java | 62 +++++++++++++------ .../server/am/BroadcastQueueModernImpl.java | 22 ++++--- .../android/server/am/BroadcastQueueTest.java | 37 ++++++++++- 3 files changed, 91 insertions(+), 30 deletions(-) diff --git a/services/core/java/com/android/server/am/BroadcastProcessQueue.java b/services/core/java/com/android/server/am/BroadcastProcessQueue.java index 5123517e272d3..093c7b510d776 100644 --- a/services/core/java/com/android/server/am/BroadcastProcessQueue.java +++ b/services/core/java/com/android/server/am/BroadcastProcessQueue.java @@ -85,9 +85,16 @@ class BroadcastProcessQueue { @Nullable ProcessRecord app; /** - * Track name to use for {@link Trace} events. + * Track name to use for {@link Trace} events, defined as part of upgrading + * into a running slot. */ - @Nullable String traceTrackName; + @Nullable String runningTraceTrackName; + + /** + * Flag indicating if this process should be OOM adjusted, defined as part + * of upgrading into a running slot. + */ + boolean runningOomAdjusted; /** * Snapshotted value of {@link ProcessRecord#getCpuDelayTime()}, typically @@ -141,7 +148,8 @@ class BroadcastProcessQueue { private boolean mActiveViaColdStart; /** - * Count of {@link #mPending} broadcasts of these various flavors. + * Count of {@link #mPending} and {@link #mPendingUrgent} broadcasts of + * these various flavors. */ private int mCountForeground; private int mCountOrdered; @@ -150,6 +158,7 @@ class BroadcastProcessQueue { private int mCountInteractive; private int mCountResultTo; private int mCountInstrumented; + private int mCountManifest; private @UptimeMillisLong long mRunnableAt = Long.MAX_VALUE; private @Reason int mRunnableAtReason = REASON_EMPTY; @@ -206,7 +215,7 @@ class BroadcastProcessQueue { // with implicit responsiveness expectations. final ArrayDeque queue = record.isUrgent() ? mPendingUrgent : mPending; queue.addLast(newBroadcastArgs); - onBroadcastEnqueued(record); + onBroadcastEnqueued(record, recordIndex); } /** @@ -224,7 +233,8 @@ class BroadcastProcessQueue { while (it.hasNext()) { final SomeArgs args = it.next(); final BroadcastRecord testRecord = (BroadcastRecord) args.arg1; - final Object testReceiver = testRecord.receivers.get(args.argi1); + final int testRecordIndex = args.argi1; + final Object testReceiver = testRecord.receivers.get(testRecordIndex); if ((record.callingUid == testRecord.callingUid) && (record.userId == testRecord.userId) && record.intent.filterEquals(testRecord.intent) @@ -233,8 +243,8 @@ class BroadcastProcessQueue { args.arg1 = record; args.argi1 = recordIndex; args.argi2 = blockedUntilTerminalCount; - onBroadcastDequeued(testRecord); - onBroadcastEnqueued(record); + onBroadcastDequeued(testRecord, testRecordIndex); + onBroadcastEnqueued(record, recordIndex); return true; } } @@ -284,13 +294,13 @@ class BroadcastProcessQueue { while (it.hasNext()) { final SomeArgs args = it.next(); final BroadcastRecord record = (BroadcastRecord) args.arg1; - final int index = args.argi1; - if (predicate.test(record, index)) { - consumer.accept(record, index); + final int recordIndex = args.argi1; + if (predicate.test(record, recordIndex)) { + consumer.accept(record, recordIndex); if (andRemove) { args.recycle(); it.remove(); - onBroadcastDequeued(record); + onBroadcastDequeued(record, recordIndex); } didSomething = true; } @@ -385,7 +395,7 @@ class BroadcastProcessQueue { mActiveCountSinceIdle++; mActiveViaColdStart = false; next.recycle(); - onBroadcastDequeued(mActive); + onBroadcastDequeued(mActive, mActiveIndex); } /** @@ -403,7 +413,7 @@ class BroadcastProcessQueue { /** * Update summary statistics when the given record has been enqueued. */ - private void onBroadcastEnqueued(@NonNull BroadcastRecord record) { + private void onBroadcastEnqueued(@NonNull BroadcastRecord record, int recordIndex) { if (record.isForeground()) { mCountForeground++; } @@ -425,13 +435,16 @@ class BroadcastProcessQueue { if (record.callerInstrumented) { mCountInstrumented++; } + if (record.receivers.get(recordIndex) instanceof ResolveInfo) { + mCountManifest++; + } invalidateRunnableAt(); } /** * Update summary statistics when the given record has been dequeued. */ - private void onBroadcastDequeued(@NonNull BroadcastRecord record) { + private void onBroadcastDequeued(@NonNull BroadcastRecord record, int recordIndex) { if (record.isForeground()) { mCountForeground--; } @@ -453,34 +466,37 @@ class BroadcastProcessQueue { if (record.callerInstrumented) { mCountInstrumented--; } + if (record.receivers.get(recordIndex) instanceof ResolveInfo) { + mCountManifest--; + } invalidateRunnableAt(); } public void traceProcessStartingBegin() { Trace.asyncTraceForTrackBegin(Trace.TRACE_TAG_ACTIVITY_MANAGER, - traceTrackName, toShortString() + " starting", hashCode()); + runningTraceTrackName, toShortString() + " starting", hashCode()); } public void traceProcessRunningBegin() { Trace.asyncTraceForTrackBegin(Trace.TRACE_TAG_ACTIVITY_MANAGER, - traceTrackName, toShortString() + " running", hashCode()); + runningTraceTrackName, toShortString() + " running", hashCode()); } public void traceProcessEnd() { Trace.asyncTraceForTrackEnd(Trace.TRACE_TAG_ACTIVITY_MANAGER, - traceTrackName, hashCode()); + runningTraceTrackName, hashCode()); } public void traceActiveBegin() { final int cookie = mActive.receivers.get(mActiveIndex).hashCode(); Trace.asyncTraceForTrackBegin(Trace.TRACE_TAG_ACTIVITY_MANAGER, - traceTrackName, mActive.toShortString() + " scheduled", cookie); + runningTraceTrackName, mActive.toShortString() + " scheduled", cookie); } public void traceActiveEnd() { final int cookie = mActive.receivers.get(mActiveIndex).hashCode(); Trace.asyncTraceForTrackEnd(Trace.TRACE_TAG_ACTIVITY_MANAGER, - traceTrackName, cookie); + runningTraceTrackName, cookie); } /** @@ -539,6 +555,14 @@ class BroadcastProcessQueue { return (queue != null) ? (BroadcastRecord) queue.peekFirst().arg1 : null; } + /** + * Quickly determine if this queue has broadcasts waiting to be delivered to + * manifest receivers, which indicates we should request an OOM adjust. + */ + public boolean isPendingManifest() { + return mCountManifest > 0; + } + /** * Quickly determine if this queue has broadcasts that are still waiting to * be delivered at some point in the future. diff --git a/services/core/java/com/android/server/am/BroadcastQueueModernImpl.java b/services/core/java/com/android/server/am/BroadcastQueueModernImpl.java index 4c831bd47ee4c..6b0bbbe003888 100644 --- a/services/core/java/com/android/server/am/BroadcastQueueModernImpl.java +++ b/services/core/java/com/android/server/am/BroadcastQueueModernImpl.java @@ -401,7 +401,8 @@ class BroadcastQueueModernImpl extends BroadcastQueue { mRunnableHead = removeFromRunnableList(mRunnableHead, queue); // Emit all trace events for this process into a consistent track - queue.traceTrackName = TAG + ".mRunning[" + queueIndex + "]"; + queue.runningTraceTrackName = TAG + ".mRunning[" + queueIndex + "]"; + queue.runningOomAdjusted = queue.isPendingManifest(); // If we're already warm, schedule next pending broadcast now; // otherwise we'll wait for the cold start to circle back around @@ -415,9 +416,8 @@ class BroadcastQueueModernImpl extends BroadcastQueue { scheduleReceiverColdLocked(queue); } - // We've moved at least one process into running state above, so we - // need to kick off an OOM adjustment pass - updateOomAdj = true; + // Only kick off an OOM adjustment pass if needed + updateOomAdj |= queue.runningOomAdjusted; // Move to considering next runnable queue queue = nextQueue; @@ -1245,8 +1245,6 @@ class BroadcastQueueModernImpl extends BroadcastQueue { if (queue.app != null) { queue.app.mReceivers.incrementCurReceivers(); - queue.app.mState.forceProcessStateUpTo(ActivityManager.PROCESS_STATE_RECEIVER); - // Don't bump its LRU position if it's in the background restricted. if (mService.mInternal.getRestrictionLevel( queue.uid) < ActivityManager.RESTRICTION_LEVEL_RESTRICTED_BUCKET) { @@ -1256,7 +1254,10 @@ class BroadcastQueueModernImpl extends BroadcastQueue { mService.mOomAdjuster.mCachedAppOptimizer.unfreezeTemporarily(queue.app, OOM_ADJ_REASON_START_RECEIVER); - mService.enqueueOomAdjTargetLocked(queue.app); + if (queue.runningOomAdjusted) { + queue.app.mState.forceProcessStateUpTo(ActivityManager.PROCESS_STATE_RECEIVER); + mService.enqueueOomAdjTargetLocked(queue.app); + } } } @@ -1266,10 +1267,11 @@ class BroadcastQueueModernImpl extends BroadcastQueue { */ private void notifyStoppedRunning(@NonNull BroadcastProcessQueue queue) { if (queue.app != null) { - // Update during our next pass; no need for an immediate update - mService.enqueueOomAdjTargetLocked(queue.app); - queue.app.mReceivers.decrementCurReceivers(); + + if (queue.runningOomAdjusted) { + mService.enqueueOomAdjTargetLocked(queue.app); + } } } diff --git a/services/tests/mockingservicestests/src/com/android/server/am/BroadcastQueueTest.java b/services/tests/mockingservicestests/src/com/android/server/am/BroadcastQueueTest.java index d9a26c68f3edf..0a428c415e4de 100644 --- a/services/tests/mockingservicestests/src/com/android/server/am/BroadcastQueueTest.java +++ b/services/tests/mockingservicestests/src/com/android/server/am/BroadcastQueueTest.java @@ -888,7 +888,7 @@ public class BroadcastQueueTest { }) { // Confirm expected OOM adjustments; we were invoked once to upgrade // and once to downgrade - assertEquals(ActivityManager.PROCESS_STATE_RECEIVER, + assertEquals(String.valueOf(receiverApp), ActivityManager.PROCESS_STATE_RECEIVER, receiverApp.mState.getReportedProcState()); verify(mAms, times(2)).enqueueOomAdjTargetLocked(eq(receiverApp)); @@ -1599,4 +1599,39 @@ public class BroadcastQueueTest { assertTrue(mQueue.isBeyondBarrierLocked(afterFirst)); assertTrue(mQueue.isBeyondBarrierLocked(afterSecond)); } + + /** + * Verify that we OOM adjust for manifest receivers. + */ + @Test + public void testOomAdjust_Manifest() throws Exception { + final ProcessRecord callerApp = makeActiveProcessRecord(PACKAGE_RED); + + final Intent airplane = new Intent(Intent.ACTION_AIRPLANE_MODE_CHANGED); + enqueueBroadcast(makeBroadcastRecord(airplane, callerApp, + List.of(makeManifestReceiver(PACKAGE_GREEN, CLASS_GREEN), + makeManifestReceiver(PACKAGE_GREEN, CLASS_BLUE), + makeManifestReceiver(PACKAGE_GREEN, CLASS_RED)))); + + waitForIdle(); + verify(mAms, atLeastOnce()).enqueueOomAdjTargetLocked(any()); + } + + /** + * Verify that we never OOM adjust for registered receivers. + */ + @Test + public void testOomAdjust_Registered() throws Exception { + final ProcessRecord callerApp = makeActiveProcessRecord(PACKAGE_RED); + final ProcessRecord receiverApp = makeActiveProcessRecord(PACKAGE_GREEN); + + final Intent airplane = new Intent(Intent.ACTION_AIRPLANE_MODE_CHANGED); + enqueueBroadcast(makeBroadcastRecord(airplane, callerApp, + List.of(makeRegisteredReceiver(receiverApp), + makeRegisteredReceiver(receiverApp), + makeRegisteredReceiver(receiverApp)))); + + waitForIdle(); + verify(mAms, never()).enqueueOomAdjTargetLocked(any()); + } }