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
2022-02-03 16:12:58 -05:00
parent 135c992b0e
commit a06ea9df72
5 changed files with 299 additions and 10 deletions
--- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotifPipelineChoreographer.kt
+++ 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<Runnable>()
+    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<Runnable>()
+
+    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)
+    }
+}
--- 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<ListEntry> mReadOnlyNotifList = Collections.unmodifiableList(mNotifList);
    private List<ListEntry> 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();
        }
    }

--- 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,
 })
--- 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
@@ -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<DelayableExecutor>().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()
+    }
+}
--- 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<CollectionReadyForBuildListener> mBuildListenerCaptor;

+    private final FakeNotifPipelineChoreographer mPipelineChoreographer =
+            new FakeNotifPipelineChoreographer();
    private CollectionReadyForBuildListener mReadyForBuildListener;
    private List<NotificationEntryBuilder> mPendingSet = new ArrayList<>();
    private List<NotificationEntry> 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<Integer> intCmp = Integer::compare;
@@ -1914,6 +1956,7 @@ public class ShadeListBuilderTest extends SysuiTestCase {
        }

        mReadyForBuildListener.onBuildList(mEntrySet);
+        mPipelineChoreographer.runIfScheduled();
    }

    private void verifyBuiltList(ExpectedEntry ...expectedEntries) {