Merge "Add NamedListenerSet that can be used to improve Trace performance by caching listener names" into udc-qpr-dev

2023-08-08 18:32:41 +00:00
parent 022e4bb63a 7828879228
commit cd5177b606
7 changed files with 348 additions and 37 deletions
--- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotifLiveDataStoreImpl.kt
+++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotifLiveDataStoreImpl.kt
@@ -21,7 +21,6 @@ import com.android.systemui.dagger.SysUISingleton
 import com.android.systemui.dagger.qualifiers.Main
 import com.android.systemui.util.Assert
 import com.android.systemui.util.ListenerSet
-import com.android.systemui.util.isNotEmpty
 import com.android.systemui.util.traceSection
 import java.util.Collections.unmodifiableList
 import java.util.concurrent.Executor
--- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/provider/DebugModeFilterProvider.kt
+++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/provider/DebugModeFilterProvider.kt
@@ -26,7 +26,6 @@ import com.android.systemui.statusbar.commandline.CommandRegistry
 import com.android.systemui.statusbar.notification.collection.NotificationEntry
 import com.android.systemui.util.Assert
 import com.android.systemui.util.ListenerSet
-import com.android.systemui.util.isNotEmpty
 import java.io.PrintWriter
 import javax.inject.Inject

--- a/packages/SystemUI/src/com/android/systemui/util/IListenerSet.kt
+++ b/packages/SystemUI/src/com/android/systemui/util/IListenerSet.kt
@@ -0,0 +1,36 @@
+/*
+ * Copyright (C) 2023 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.util
+
+/**
+ * A collection of listeners, observers, callbacks, etc.
+ *
+ * This container is optimized for infrequent mutation and frequent iteration, with thread safety
+ * and reentrant-safety guarantees as well. Specifically, to ensure that
+ * [ConcurrentModificationException] is never thrown, this iterator will not reflect changes made to
+ * the set after the iterator is constructed.
+ */
+interface IListenerSet<E : Any> : Set<E> {
+    /**
+     * A thread-safe, reentrant-safe method to add a listener. Does nothing if the listener is
+     * already in the set.
+     */
+    fun addIfAbsent(element: E): Boolean
+
+    /** A thread-safe, reentrant-safe method to remove a listener. */
+    fun remove(element: E): Boolean
+}
--- a/packages/SystemUI/src/com/android/systemui/util/ListenerSet.kt
+++ b/packages/SystemUI/src/com/android/systemui/util/ListenerSet.kt
@@ -29,20 +29,12 @@ import java.util.concurrent.CopyOnWriteArrayList
 class ListenerSet<E : Any>
 /** Private constructor takes the internal list so that we can use auto-delegation */
 private constructor(private val listeners: CopyOnWriteArrayList<E>) :
-    Collection<E> by listeners, Set<E> {
+    Collection<E> by listeners, IListenerSet<E> {

    /** Create a new instance */
    constructor() : this(CopyOnWriteArrayList())

-    /**
-     * A thread-safe, reentrant-safe method to add a listener. Does nothing if the listener is
-     * already in the set.
-     */
-    fun addIfAbsent(element: E): Boolean = listeners.addIfAbsent(element)
+    override fun addIfAbsent(element: E): Boolean = listeners.addIfAbsent(element)

-    /** A thread-safe, reentrant-safe method to remove a listener. */
-    fun remove(element: E): Boolean = listeners.remove(element)
+    override fun remove(element: E): Boolean = listeners.remove(element)
 }
-
-/** Extension to match Collection which is implemented to only be (easily) accessible in kotlin */
-fun <T : Any> ListenerSet<T>.isNotEmpty(): Boolean = !isEmpty()
--- a/packages/SystemUI/src/com/android/systemui/util/NamedListenerSet.kt
+++ b/packages/SystemUI/src/com/android/systemui/util/NamedListenerSet.kt
@@ -0,0 +1,96 @@
+/*
+ * Copyright (C) 2023 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.util
+
+import java.util.concurrent.CopyOnWriteArrayList
+import java.util.function.Consumer
+
+/**
+ * A collection of listeners, observers, callbacks, etc.
+ *
+ * This container is optimized for infrequent mutation and frequent iteration, with thread safety
+ * and reentrant-safety guarantees as well. Specifically, to ensure that
+ * [ConcurrentModificationException] is never thrown, this iterator will not reflect changes made to
+ * the set after the iterator is constructed.
+ *
+ * This class provides all the abilities of [ListenerSet], except that each listener has a name
+ * calculated at runtime which can be used for time-efficient tracing of listener invocations.
+ */
+class NamedListenerSet<E : Any>(
+    private val getName: (E) -> String = { it.javaClass.name },
+) : IListenerSet<E> {
+    private val listeners = CopyOnWriteArrayList<NamedListener>()
+
+    override val size: Int
+        get() = listeners.size
+
+    override fun isEmpty() = listeners.isEmpty()
+
+    override fun iterator(): Iterator<E> = iterator {
+        listeners.iterator().forEach { yield(it.listener) }
+    }
+
+    override fun containsAll(elements: Collection<E>) =
+        listeners.count { it.listener in elements } == elements.size
+
+    override fun contains(element: E) = listeners.firstOrNull { it.listener == element } != null
+
+    override fun addIfAbsent(element: E): Boolean = listeners.addIfAbsent(NamedListener(element))
+
+    override fun remove(element: E): Boolean = listeners.removeIf { it.listener == element }
+
+    /** A wrapper for the listener with an associated name. */
+    inner class NamedListener(val listener: E) {
+        val name: String = getName(listener)
+
+        override fun hashCode(): Int {
+            return listener.hashCode()
+        }
+
+        override fun equals(other: Any?): Boolean =
+            when {
+                other === null -> false
+                other === this -> true
+                other !is NamedListenerSet<*>.NamedListener -> false
+                listener == other.listener -> true
+                else -> false
+            }
+    }
+
+    /** Iterate the listeners in the set, providing the name for each one as well. */
+    inline fun forEachNamed(block: (String, E) -> Unit) =
+        namedIterator().forEach { element -> block(element.name, element.listener) }
+
+    /**
+     * Iterate the listeners in the set, wrapping each call to the block with [traceSection] using
+     * the listener name.
+     */
+    inline fun forEachTraced(block: (E) -> Unit) = forEachNamed { name, listener ->
+        traceSection(name) { block(listener) }
+    }
+
+    /**
+     * Iterate the listeners in the set, wrapping each call to the block with [traceSection] using
+     * the listener name.
+     */
+    fun forEachTraced(consumer: Consumer<E>) = forEachNamed { name, listener ->
+        traceSection(name) { consumer.accept(listener) }
+    }
+
+    /** Iterate over the [NamedListener]s currently in the set. */
+    fun namedIterator(): Iterator<NamedListener> = listeners.iterator()
+}
--- a/packages/SystemUI/tests/src/com/android/systemui/util/ListenerSetTest.kt
+++ b/packages/SystemUI/tests/src/com/android/systemui/util/ListenerSetTest.kt
@@ -16,43 +16,128 @@

 package com.android.systemui.util

-import android.test.suitebuilder.annotation.SmallTest
-import androidx.test.runner.AndroidJUnit4
+import androidx.test.ext.junit.runners.AndroidJUnit4
+import androidx.test.filters.SmallTest
 import com.android.systemui.SysuiTestCase
 import com.google.common.truth.Truth.assertThat
-import org.junit.Before
 import org.junit.Test
 import org.junit.runner.RunWith

@SmallTest
@RunWith(AndroidJUnit4::class)
-class ListenerSetTest : SysuiTestCase() {
+open class ListenerSetTest : SysuiTestCase() {

-    var runnableSet: ListenerSet<Runnable> = ListenerSet()
+    private val runnableSet: IListenerSet<Runnable> = makeRunnableListenerSet()

-    @Before
-    fun setup() {
-        runnableSet = ListenerSet()
-    }
+    open fun makeRunnableListenerSet(): IListenerSet<Runnable> = ListenerSet()

    @Test
    fun addIfAbsent_doesNotDoubleAdd() {
        // setup & preconditions
        val runnable1 = Runnable { }
        val runnable2 = Runnable { }
-        assertThat(runnableSet.toList()).isEmpty()
+        assertThat(runnableSet).isEmpty()

        // Test that an element can be added
        assertThat(runnableSet.addIfAbsent(runnable1)).isTrue()
-        assertThat(runnableSet.toList()).containsExactly(runnable1)
+        assertThat(runnableSet).containsExactly(runnable1)

        // Test that a second element can be added
        assertThat(runnableSet.addIfAbsent(runnable2)).isTrue()
-        assertThat(runnableSet.toList()).containsExactly(runnable1, runnable2)
+        assertThat(runnableSet).containsExactly(runnable1, runnable2)

        // Test that re-adding the first element does nothing and returns false
        assertThat(runnableSet.addIfAbsent(runnable1)).isFalse()
-        assertThat(runnableSet.toList()).containsExactly(runnable1, runnable2)
+        assertThat(runnableSet).containsExactly(runnable1, runnable2)
+    }
+
+    @Test
+    fun isEmpty_changes() {
+        val runnable = Runnable { }
+        assertThat(runnableSet).isEmpty()
+        assertThat(runnableSet.isEmpty()).isTrue()
+        assertThat(runnableSet.isNotEmpty()).isFalse()
+
+        assertThat(runnableSet.addIfAbsent(runnable)).isTrue()
+        assertThat(runnableSet).isNotEmpty()
+        assertThat(runnableSet.isEmpty()).isFalse()
+        assertThat(runnableSet.isNotEmpty()).isTrue()
+
+        assertThat(runnableSet.remove(runnable)).isTrue()
+        assertThat(runnableSet).isEmpty()
+        assertThat(runnableSet.isEmpty()).isTrue()
+        assertThat(runnableSet.isNotEmpty()).isFalse()
+    }
+
+    @Test
+    fun size_changes() {
+        assertThat(runnableSet).isEmpty()
+        assertThat(runnableSet.size).isEqualTo(0)
+
+        assertThat(runnableSet.addIfAbsent(Runnable { })).isTrue()
+        assertThat(runnableSet.size).isEqualTo(1)
+
+        assertThat(runnableSet.addIfAbsent(Runnable { })).isTrue()
+        assertThat(runnableSet.size).isEqualTo(2)
+    }
+
+    @Test
+    fun contains_worksAsExpected() {
+        val runnable1 = Runnable { }
+        val runnable2 = Runnable { }
+        assertThat(runnableSet).isEmpty()
+        assertThat(runnable1 in runnableSet).isFalse()
+        assertThat(runnable2 in runnableSet).isFalse()
+        assertThat(runnableSet).doesNotContain(runnable1)
+        assertThat(runnableSet).doesNotContain(runnable2)
+
+        assertThat(runnableSet.addIfAbsent(runnable1)).isTrue()
+        assertThat(runnable1 in runnableSet).isTrue()
+        assertThat(runnable2 in runnableSet).isFalse()
+        assertThat(runnableSet).contains(runnable1)
+        assertThat(runnableSet).doesNotContain(runnable2)
+
+        assertThat(runnableSet.addIfAbsent(runnable2)).isTrue()
+        assertThat(runnable1 in runnableSet).isTrue()
+        assertThat(runnable2 in runnableSet).isTrue()
+        assertThat(runnableSet).contains(runnable1)
+        assertThat(runnableSet).contains(runnable2)
+
+        assertThat(runnableSet.remove(runnable1)).isTrue()
+        assertThat(runnable1 in runnableSet).isFalse()
+        assertThat(runnable2 in runnableSet).isTrue()
+        assertThat(runnableSet).doesNotContain(runnable1)
+        assertThat(runnableSet).contains(runnable2)
+    }
+
+    @Test
+    fun containsAll_worksAsExpected() {
+        val runnable1 = Runnable { }
+        val runnable2 = Runnable { }
+
+        assertThat(runnableSet).isEmpty()
+        assertThat(runnableSet.containsAll(listOf())).isTrue()
+        assertThat(runnableSet.containsAll(listOf(runnable1))).isFalse()
+        assertThat(runnableSet.containsAll(listOf(runnable2))).isFalse()
+        assertThat(runnableSet.containsAll(listOf(runnable1, runnable2))).isFalse()
+
+        assertThat(runnableSet.addIfAbsent(runnable1)).isTrue()
+        assertThat(runnableSet.containsAll(listOf())).isTrue()
+        assertThat(runnableSet.containsAll(listOf(runnable1))).isTrue()
+        assertThat(runnableSet.containsAll(listOf(runnable2))).isFalse()
+        assertThat(runnableSet.containsAll(listOf(runnable1, runnable2))).isFalse()
+
+        assertThat(runnableSet.addIfAbsent(runnable2)).isTrue()
+        assertThat(runnableSet.containsAll(listOf())).isTrue()
+        assertThat(runnableSet.containsAll(listOf(runnable1))).isTrue()
+        assertThat(runnableSet.containsAll(listOf(runnable2))).isTrue()
+        assertThat(runnableSet.containsAll(listOf(runnable1, runnable2))).isTrue()
+
+        assertThat(runnableSet.remove(runnable1)).isTrue()
+        assertThat(runnableSet.containsAll(listOf())).isTrue()
+        assertThat(runnableSet.containsAll(listOf(runnable1))).isFalse()
+        assertThat(runnableSet.containsAll(listOf(runnable2))).isTrue()
+        assertThat(runnableSet.containsAll(listOf(runnable1, runnable2))).isFalse()
    }

    @Test
@@ -60,22 +145,22 @@ class ListenerSetTest : SysuiTestCase() {
        // setup and preconditions
        val runnable1 = Runnable { }
        val runnable2 = Runnable { }
-        assertThat(runnableSet.toList()).isEmpty()
+        assertThat(runnableSet).isEmpty()
        runnableSet.addIfAbsent(runnable1)
        runnableSet.addIfAbsent(runnable2)
-        assertThat(runnableSet.toList()).containsExactly(runnable1, runnable2)
+        assertThat(runnableSet).containsExactly(runnable1, runnable2)

        // Test that removing the first runnable only removes that one runnable
        assertThat(runnableSet.remove(runnable1)).isTrue()
-        assertThat(runnableSet.toList()).containsExactly(runnable2)
+        assertThat(runnableSet).containsExactly(runnable2)

        // Test that removing a non-present runnable does not error
        assertThat(runnableSet.remove(runnable1)).isFalse()
-        assertThat(runnableSet.toList()).containsExactly(runnable2)
+        assertThat(runnableSet).containsExactly(runnable2)

        // Test that removing the other runnable succeeds
        assertThat(runnableSet.remove(runnable2)).isTrue()
-        assertThat(runnableSet.toList()).isEmpty()
+        assertThat(runnableSet).isEmpty()
    }

    @Test
@@ -92,17 +177,17 @@ class ListenerSetTest : SysuiTestCase() {
        val runnable2 = Runnable {
            runnablesCalled.add(2)
        }
-        assertThat(runnableSet.toList()).isEmpty()
+        assertThat(runnableSet).isEmpty()
        runnableSet.addIfAbsent(runnable1)
        runnableSet.addIfAbsent(runnable2)
-        assertThat(runnableSet.toList()).containsExactly(runnable1, runnable2)
+        assertThat(runnableSet).containsExactly(runnable1, runnable2)

        // Test that both runnables are called and 1 was removed
        for (runnable in runnableSet) {
            runnable.run()
        }
        assertThat(runnablesCalled).containsExactly(1, 2)
-        assertThat(runnableSet.toList()).containsExactly(runnable2)
+        assertThat(runnableSet).containsExactly(runnable2)
    }

    @Test
@@ -120,16 +205,16 @@ class ListenerSetTest : SysuiTestCase() {
        val runnable2 = Runnable {
            runnablesCalled.add(2)
        }
-        assertThat(runnableSet.toList()).isEmpty()
+        assertThat(runnableSet).isEmpty()
        runnableSet.addIfAbsent(runnable1)
        runnableSet.addIfAbsent(runnable2)
-        assertThat(runnableSet.toList()).containsExactly(runnable1, runnable2)
+        assertThat(runnableSet).containsExactly(runnable1, runnable2)

        // Test that both original runnables are called and 99 was added but not called
        for (runnable in runnableSet) {
            runnable.run()
        }
        assertThat(runnablesCalled).containsExactly(1, 2)
-        assertThat(runnableSet.toList()).containsExactly(runnable1, runnable2, runnable99)
+        assertThat(runnableSet).containsExactly(runnable1, runnable2, runnable99)
    }
 }
--- a/packages/SystemUI/tests/src/com/android/systemui/util/NamedListenerSetTest.kt
+++ b/packages/SystemUI/tests/src/com/android/systemui/util/NamedListenerSetTest.kt
@@ -0,0 +1,104 @@
+/*
+ * Copyright (C) 2023 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.util
+
+import androidx.test.ext.junit.runners.AndroidJUnit4
+import androidx.test.filters.SmallTest
+import com.google.common.truth.Truth.assertThat
+import org.junit.Test
+import org.junit.runner.RunWith
+
+@SmallTest
+@RunWith(AndroidJUnit4::class)
+class NamedListenerSetTest : ListenerSetTest() {
+    override fun makeRunnableListenerSet(): IListenerSet<Runnable> = NamedListenerSet()
+
+    private val runnableSet = NamedListenerSet(NamedRunnable::name)
+
+    class NamedRunnable(val name: String, private val block: () -> Unit = {}) : Runnable {
+        override fun run() = block()
+    }
+
+    @Test
+    fun addIfAbsent_addsMultipleWithSameName_onlyIfInstanceIsAbsent() {
+        // setup & preconditions
+        val runnable1 = NamedRunnable("A")
+        val runnable2 = NamedRunnable("A")
+        assertThat(runnableSet).isEmpty()
+
+        // Test that an element can be added
+        assertThat(runnableSet.addIfAbsent(runnable1)).isTrue()
+        assertThat(runnableSet).containsExactly(runnable1)
+
+        // Test that a second element can be added, even with the same name
+        assertThat(runnableSet.addIfAbsent(runnable2)).isTrue()
+        assertThat(runnableSet).containsExactly(runnable1, runnable2)
+
+        // Test that re-adding the first element does nothing and returns false
+        assertThat(runnableSet.addIfAbsent(runnable1)).isFalse()
+        assertThat(runnableSet).containsExactly(runnable1, runnable2)
+    }
+
+    @Test
+    fun forEachNamed_includesCorrectNames() {
+        val runnable1 = NamedRunnable("A")
+        val runnable2 = NamedRunnable("X")
+        val runnable3 = NamedRunnable("X")
+        assertThat(runnableSet).isEmpty()
+
+        assertThat(runnableSet.addIfAbsent(runnable1)).isTrue()
+        assertThat(runnableSet.toNamedPairs())
+            .containsExactly(
+                "A" to runnable1,
+            )
+
+        assertThat(runnableSet.addIfAbsent(runnable2)).isTrue()
+        assertThat(runnableSet.toNamedPairs())
+            .containsExactly(
+                "A" to runnable1,
+                "X" to runnable2,
+            )
+
+        assertThat(runnableSet.addIfAbsent(runnable3)).isTrue()
+        assertThat(runnableSet.toNamedPairs())
+            .containsExactly(
+                "A" to runnable1,
+                "X" to runnable2,
+                "X" to runnable3,
+            )
+
+        assertThat(runnableSet.remove(runnable1)).isTrue()
+        assertThat(runnableSet.toNamedPairs())
+            .containsExactly(
+                "X" to runnable2,
+                "X" to runnable3,
+            )
+
+        assertThat(runnableSet.remove(runnable2)).isTrue()
+        assertThat(runnableSet.toNamedPairs())
+            .containsExactly(
+                "X" to runnable3,
+            )
+    }
+
+    /**
+     * This private method uses [NamedListenerSet.forEachNamed] to produce a list of pairs in order
+     * to validate that method.
+     */
+    private fun <T : Any> NamedListenerSet<T>.toNamedPairs() =
+        sequence { forEachNamed { name, listener -> yield(name to listener) } }.toList()
+}