From a06ea9df724414429c514677550f9eafa6270685 Mon Sep 17 00:00:00 2001 From: Steve Elliott Date: Thu, 3 Feb 2022 16:12:58 -0500 Subject: [PATCH] Asynchronous notif pipeline evaluation This CL introduces NotifPipelineChoreographer, which can be used to "schedule" listener invocations at a later time. In effect, this replaces the following code: foo.addListener { expensive() } bar.addListener { expensive() } With: val choreographer: NotifPipelineChoreographer = ... choreographer.addOnEvalListener { expensive() } foo.addListener { choreographer.schedule() } bar.addListener { choreographer.schedule() } This will coalesce the calls to expensive(), which will be evaluated at some point in the future when the NotifPipelineChoreographer invokes its registered OnEvalListeners. The implementation included is based on android.view.Choreographer, which will perform a scheduled evaluation at most once per frame, in conjunction with a 100ms timeout to force evaluation. The NotifPipelineChoreographer is used to coalesce all evaluations of ShadeListBuilder.buildList, which includes all Pluggable invalidations and new emissions from the NotifCollection. Test: atest, perfetto + unlock phone (transition from keyguard) and verify ShadeListBuilder.buildList is invoked at most once per frame Fixes: 217800329 Change-Id: I4be70b495bdda5a32a202a92b9b28f2cff3402c9 --- .../collection/NotifPipelineChoreographer.kt | 140 ++++++++++++++++++ .../collection/ShadeListBuilder.java | 16 +- .../dagger/NotificationsModule.java | 2 + .../NotifPipelineChoreographerTest.kt | 100 +++++++++++++ .../collection/ShadeListBuilderTest.java | 51 ++++++- 5 files changed, 299 insertions(+), 10 deletions(-) create mode 100644 packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotifPipelineChoreographer.kt create mode 100644 packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/NotifPipelineChoreographerTest.kt diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotifPipelineChoreographer.kt b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotifPipelineChoreographer.kt new file mode 100644 index 0000000000000..6f8e5da70497e --- /dev/null +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotifPipelineChoreographer.kt @@ -0,0 +1,140 @@ +/* + * Copyright (C) 2022 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.systemui.statusbar.notification.collection + +import android.view.Choreographer +import com.android.systemui.dagger.SysUISingleton +import com.android.systemui.dagger.qualifiers.Main +import com.android.systemui.util.ListenerSet +import com.android.systemui.util.concurrency.DelayableExecutor +import dagger.Module +import dagger.Provides + +/** + * Choreographs evaluation resulting from multiple asynchronous sources. Specifically, it exposes + * [schedule], and [addOnEvalListener]; the former will "schedule" an asynchronous invocation of the + * latter. Multiple invocations of [schedule] before any added listeners are invoked have no effect. + */ +interface NotifPipelineChoreographer { + /** + * Schedules all listeners registered with [addOnEvalListener] to be asynchronously executed at + * some point in the future. The exact timing is up to the implementation. + */ + fun schedule() + + /** Cancels a pending evaluation triggered by any recent calls to [schedule]. */ + fun cancel() + + /** Adds a listener [Runnable] that will be invoked when the scheduled evaluation occurs. */ + fun addOnEvalListener(onEvalListener: Runnable) + + /** Removes a listener previously registered with [addOnEvalListener]. */ + fun removeOnEvalListener(onEvalListener: Runnable) +} + +@Module +object NotifPipelineChoreographerModule { + @Provides + @JvmStatic + @SysUISingleton + fun provideChoreographer( + choreographer: Choreographer, + @Main mainExecutor: DelayableExecutor + ): NotifPipelineChoreographer = NotifPipelineChoreographerImpl(choreographer, mainExecutor) +} + +private const val TIMEOUT_MS: Long = 100 + +private class NotifPipelineChoreographerImpl( + private val viewChoreographer: Choreographer, + private val executor: DelayableExecutor +) : NotifPipelineChoreographer { + + private val listeners = ListenerSet() + private var timeoutSubscription: Runnable? = null + private var isScheduled = false + + private val frameCallback = Choreographer.FrameCallback { + if (isScheduled) { + isScheduled = false + timeoutSubscription?.run() + listeners.forEach { it.run() } + } + } + + override fun schedule() { + if (isScheduled) return + isScheduled = true + viewChoreographer.postFrameCallback(frameCallback) + if (!isScheduled) { + // Guard against synchronous evaluation of the frame callback. + return + } + timeoutSubscription = executor.executeDelayed(::onTimeout, TIMEOUT_MS) + } + + override fun cancel() { + if (!isScheduled) return + timeoutSubscription?.run() + viewChoreographer.removeFrameCallback(frameCallback) + } + + override fun addOnEvalListener(onEvalListener: Runnable) { + listeners.addIfAbsent(onEvalListener) + } + + override fun removeOnEvalListener(onEvalListener: Runnable) { + listeners.remove(onEvalListener) + } + + private fun onTimeout() { + if (isScheduled) { + isScheduled = false + viewChoreographer.removeFrameCallback(frameCallback) + listeners.forEach { it.run() } + } + } +} + +class FakeNotifPipelineChoreographer : NotifPipelineChoreographer { + + var isScheduled = false + val listeners = ListenerSet() + + fun runIfScheduled() { + if (isScheduled) { + isScheduled = false + listeners.forEach { it.run() } + } + } + + override fun schedule() { + isScheduled = true + } + + override fun cancel() { + isScheduled = false + } + + override fun addOnEvalListener(onEvalListener: Runnable) { + listeners.addIfAbsent(onEvalListener) + } + + override fun removeOnEvalListener(onEvalListener: Runnable) { + listeners.remove(onEvalListener) + } +} diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/ShadeListBuilder.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/ShadeListBuilder.java index b328ae8cd0bb2..e0c2fd5632262 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/ShadeListBuilder.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/ShadeListBuilder.java @@ -122,20 +122,23 @@ public class ShadeListBuilder implements Dumpable { private List mReadOnlyNotifList = Collections.unmodifiableList(mNotifList); private List mReadOnlyNewNotifList = Collections.unmodifiableList(mNewNotifList); + private final NotifPipelineChoreographer mChoreographer; @Inject public ShadeListBuilder( - SystemClock systemClock, - NotifPipelineFlags flags, - ShadeListBuilderLogger logger, DumpManager dumpManager, - NotificationInteractionTracker interactionTracker + NotifPipelineChoreographer pipelineChoreographer, + NotifPipelineFlags flags, + NotificationInteractionTracker interactionTracker, + ShadeListBuilderLogger logger, + SystemClock systemClock ) { Assert.isMainThread(); mSystemClock = systemClock; mLogger = logger; mAlwaysLogList = flags.isDevLoggingEnabled(); mInteractionTracker = interactionTracker; + mChoreographer = pipelineChoreographer; dumpManager.registerDumpable(TAG, this); setSectioners(Collections.emptyList()); @@ -148,6 +151,7 @@ public class ShadeListBuilder implements Dumpable { public void attach(NotifCollection collection) { Assert.isMainThread(); collection.setBuildListener(mReadyForBuildListener); + mChoreographer.addOnEvalListener(this::buildList); } /** @@ -290,7 +294,7 @@ public class ShadeListBuilder implements Dumpable { mLogger.logOnBuildList(); mAllEntries = entries; - buildList(); + mChoreographer.schedule(); } }; @@ -1281,7 +1285,7 @@ public class ShadeListBuilder implements Dumpable { private void rebuildListIfBefore(@PipelineState.StateName int state) { mPipelineState.requireIsBefore(state); if (mPipelineState.is(STATE_IDLE)) { - buildList(); + mChoreographer.schedule(); } } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/dagger/NotificationsModule.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/dagger/NotificationsModule.java index 45a9092068e43..35d4582d9dee8 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/dagger/NotificationsModule.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/dagger/NotificationsModule.java @@ -47,6 +47,7 @@ import com.android.systemui.statusbar.notification.collection.NotifInflaterImpl; import com.android.systemui.statusbar.notification.collection.NotifLiveDataStore; import com.android.systemui.statusbar.notification.collection.NotifLiveDataStoreImpl; import com.android.systemui.statusbar.notification.collection.NotifPipeline; +import com.android.systemui.statusbar.notification.collection.NotifPipelineChoreographerModule; import com.android.systemui.statusbar.notification.collection.coordinator.ShadeEventCoordinator; import com.android.systemui.statusbar.notification.collection.coordinator.VisualStabilityCoordinator; import com.android.systemui.statusbar.notification.collection.coordinator.dagger.CoordinatorsModule; @@ -103,6 +104,7 @@ import dagger.Provides; */ @Module(includes = { CoordinatorsModule.class, + NotifPipelineChoreographerModule.class, NotifPanelEventSourceModule.class, NotificationSectionHeadersModule.class, }) diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/NotifPipelineChoreographerTest.kt b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/NotifPipelineChoreographerTest.kt new file mode 100644 index 0000000000000..3820b98ab632b --- /dev/null +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/NotifPipelineChoreographerTest.kt @@ -0,0 +1,100 @@ +/* + * Copyright (C) 2022 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.systemui.statusbar.notification.collection + +import android.testing.AndroidTestingRunner +import android.view.Choreographer +import androidx.test.filters.SmallTest +import com.android.systemui.SysuiTestCase +import com.android.systemui.util.concurrency.DelayableExecutor +import com.android.systemui.util.mockito.any +import com.android.systemui.util.mockito.mock +import com.android.systemui.util.mockito.withArgCaptor +import org.junit.Assert.assertTrue +import org.junit.Test +import org.junit.runner.RunWith +import org.mockito.Mockito.anyLong +import org.mockito.Mockito.verify +import org.mockito.Mockito.`when` as whenever + +@SmallTest +@RunWith(AndroidTestingRunner::class) +class NotifPipelineChoreographerTest : SysuiTestCase() { + + val viewChoreographer: Choreographer = mock() + val timeoueSubscription: Runnable = mock() + val executor: DelayableExecutor = mock().also { + whenever(it.executeDelayed(any(), anyLong())).thenReturn(timeoueSubscription) + } + + val pipelineChoreographer: NotifPipelineChoreographer = NotifPipelineChoreographerModule + .provideChoreographer(viewChoreographer, executor) + + @Test + fun scheduleThenEvalFrameCallback() { + // GIVEN a registered eval listener and scheduled choreographer + var hasEvaluated = false + pipelineChoreographer.addOnEvalListener { + hasEvaluated = true + } + pipelineChoreographer.schedule() + val frameCallback: Choreographer.FrameCallback = withArgCaptor { + verify(viewChoreographer).postFrameCallback(capture()) + } + // WHEN the choreographer would invoke its callback + frameCallback.doFrame(0) + // THEN the choreographer would evaluate, and the timeoutSubscription would have been + // cancelled + assertTrue(hasEvaluated) + verify(timeoueSubscription).run() + } + + @Test + fun scheduleThenEvalTimeoutCallback() { + // GIVEN a registered eval listener and scheduled choreographer + var hasEvaluated = false + pipelineChoreographer.addOnEvalListener { + hasEvaluated = true + } + pipelineChoreographer.schedule() + val frameCallback: Choreographer.FrameCallback = withArgCaptor { + verify(viewChoreographer).postFrameCallback(capture()) + } + val runnable: Runnable = withArgCaptor { + verify(executor).executeDelayed(capture(), anyLong()) + } + // WHEN the executor would invoke its callback (indicating a timeout) + runnable.run() + // THEN the choreographer would evaluate, and the FrameCallback would have been unregistered + assertTrue(hasEvaluated) + verify(viewChoreographer).removeFrameCallback(frameCallback) + } + + @Test + fun scheduleThenCancel() { + // GIVEN a scheduled choreographer + pipelineChoreographer.schedule() + val frameCallback: Choreographer.FrameCallback = withArgCaptor { + verify(viewChoreographer).postFrameCallback(capture()) + } + // WHEN the scheduled run is cancelled + pipelineChoreographer.cancel() + // THEN both the FrameCallback is unregistered and the timeout subscription is cancelled. + verify(viewChoreographer).removeFrameCallback(frameCallback) + verify(timeoueSubscription).run() + } +} \ No newline at end of file diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/ShadeListBuilderTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/ShadeListBuilderTest.java index cb248b05e4bd6..f470715b92c4c 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/ShadeListBuilderTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/ShadeListBuilderTest.java @@ -111,6 +111,8 @@ public class ShadeListBuilderTest extends SysuiTestCase { @Captor private ArgumentCaptor mBuildListenerCaptor; + private final FakeNotifPipelineChoreographer mPipelineChoreographer = + new FakeNotifPipelineChoreographer(); private CollectionReadyForBuildListener mReadyForBuildListener; private List mPendingSet = new ArrayList<>(); private List mEntrySet = new ArrayList<>(); @@ -127,11 +129,12 @@ public class ShadeListBuilderTest extends SysuiTestCase { allowTestableLooperAsMainThread(); mListBuilder = new ShadeListBuilder( - mSystemClock, - mNotifPipelineFlags, - mLogger, mDumpManager, - mInteractionTracker + mPipelineChoreographer, + mNotifPipelineFlags, + mInteractionTracker, + mLogger, + mSystemClock ); mListBuilder.setOnRenderListListener(mOnRenderListListener); @@ -567,6 +570,7 @@ public class ShadeListBuilderTest extends SysuiTestCase { // WHEN the pipeline is kicked off mReadyForBuildListener.onBuildList(singletonList(entry)); + mPipelineChoreographer.runIfScheduled(); // THEN the entry's initialization time is reset assertFalse(entry.hasFinishedInitialization()); @@ -1024,26 +1028,38 @@ public class ShadeListBuilderTest extends SysuiTestCase { clearInvocations(mOnRenderListListener); packageFilter.invalidateList(); + assertTrue(mPipelineChoreographer.isScheduled()); + mPipelineChoreographer.runIfScheduled(); verify(mOnRenderListListener).onRenderList(anyList()); clearInvocations(mOnRenderListListener); idPromoter.invalidateList(); + assertTrue(mPipelineChoreographer.isScheduled()); + mPipelineChoreographer.runIfScheduled(); verify(mOnRenderListListener).onRenderList(anyList()); clearInvocations(mOnRenderListListener); section.invalidateList(); + assertTrue(mPipelineChoreographer.isScheduled()); + mPipelineChoreographer.runIfScheduled(); verify(mOnRenderListListener).onRenderList(anyList()); clearInvocations(mOnRenderListListener); hypeComparator.invalidateList(); + assertTrue(mPipelineChoreographer.isScheduled()); + mPipelineChoreographer.runIfScheduled(); verify(mOnRenderListListener).onRenderList(anyList()); clearInvocations(mOnRenderListListener); sectionComparator.invalidateList(); + assertTrue(mPipelineChoreographer.isScheduled()); + mPipelineChoreographer.runIfScheduled(); verify(mOnRenderListListener).onRenderList(anyList()); clearInvocations(mOnRenderListListener); preRenderInvalidator.invalidateList(); + assertTrue(mPipelineChoreographer.isScheduled()); + mPipelineChoreographer.runIfScheduled(); verify(mOnRenderListListener).onRenderList(anyList()); } @@ -1515,6 +1531,7 @@ public class ShadeListBuilderTest extends SysuiTestCase { // WHEN visual stability manager allows group changes again mStabilityManager.setAllowGroupChanges(true); mStabilityManager.invalidateList(); + mPipelineChoreographer.runIfScheduled(); // THEN entries are grouped verifyBuiltList( @@ -1553,6 +1570,7 @@ public class ShadeListBuilderTest extends SysuiTestCase { // WHEN section changes are allowed again mStabilityManager.setAllowSectionChanges(true); mStabilityManager.invalidateList(); + mPipelineChoreographer.runIfScheduled(); // THEN the section updates assertEquals(newSectioner, mEntrySet.get(0).getSection().getSectioner()); @@ -1772,6 +1790,30 @@ public class ShadeListBuilderTest extends SysuiTestCase { verifyBuiltList(); } + @Test + public void testMultipleInvalidationsCoalesce() { + // GIVEN a PreGroupFilter and a FinalizeFilter + NotifFilter filter1 = new PackageFilter(PACKAGE_5); + NotifFilter filter2 = new PackageFilter(PACKAGE_0); + mListBuilder.addPreGroupFilter(filter1); + mListBuilder.addFinalizeFilter(filter2); + + // WHEN both filters invalidate + filter1.invalidateList(); + filter2.invalidateList(); + + // THEN the pipeline choreographer is scheduled to evaluate, AND the pipeline hasn't + // actually run. + assertTrue(mPipelineChoreographer.isScheduled()); + verify(mOnRenderListListener, never()).onRenderList(anyList()); + + // WHEN the pipeline choreographer actually runs + mPipelineChoreographer.runIfScheduled(); + + // THEN the pipeline runs + verify(mOnRenderListListener).onRenderList(anyList()); + } + @Test public void testIsSorted() { Comparator intCmp = Integer::compare; @@ -1914,6 +1956,7 @@ public class ShadeListBuilderTest extends SysuiTestCase { } mReadyForBuildListener.onBuildList(mEntrySet); + mPipelineChoreographer.runIfScheduled(); } private void verifyBuiltList(ExpectedEntry ...expectedEntries) {