From 2887df96862f4985803a39dc78d879c027bfcfe2 Mon Sep 17 00:00:00 2001 From: Jeff DeCew Date: Fri, 28 Apr 2023 10:34:20 -0400 Subject: [PATCH] Small fixes to DumpManager 1) Use a TreeMap instead of an ArrayMap for dumpables and buffers to ensure consistent and predictable dump order across bugreports. 2) Instead of selecting just the first dumpable to match the target string, select the one with the shortest name; this guaruntees that an exact match will always be returned. Test: dumpsysui --list (observe: dumpables and buffers are in alphabetical order) Test: dumpsysui Log (observe: dumps QSLog instead of WifiTableLog) Test: atest DumpManagerTest Fixes: 252783246 Change-Id: Ifbdf3428b9343e1146e39e20f65a5897f6821f90 --- .../com/android/systemui/dump/DumpManager.kt | 50 +++-- .../android/systemui/dump/DumpManagerTest.kt | 174 ++++++++++++++---- 2 files changed, 169 insertions(+), 55 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/dump/DumpManager.kt b/packages/SystemUI/src/com/android/systemui/dump/DumpManager.kt index 7d1ffca88d9f9..ab8052c54f928 100644 --- a/packages/SystemUI/src/com/android/systemui/dump/DumpManager.kt +++ b/packages/SystemUI/src/com/android/systemui/dump/DumpManager.kt @@ -16,12 +16,12 @@ package com.android.systemui.dump -import android.util.ArrayMap import com.android.systemui.Dumpable import com.android.systemui.ProtoDumpable import com.android.systemui.dump.nano.SystemUIProtoDump import com.android.systemui.plugins.log.LogBuffer import java.io.PrintWriter +import java.util.TreeMap import javax.inject.Inject import javax.inject.Singleton @@ -36,8 +36,9 @@ import javax.inject.Singleton */ @Singleton open class DumpManager @Inject constructor() { - private val dumpables: MutableMap> = ArrayMap() - private val buffers: MutableMap> = ArrayMap() + // NOTE: Using TreeMap ensures that iteration is in a predictable & alphabetical order. + private val dumpables: MutableMap> = TreeMap() + private val buffers: MutableMap> = TreeMap() /** See [registerCriticalDumpable]. */ fun registerCriticalDumpable(module: Dumpable) { @@ -132,7 +133,8 @@ open class DumpManager @Inject constructor() { } /** - * Dumps the first dumpable or buffer whose registered name ends with [target] + * Dumps the alphabetically first, shortest-named dumpable or buffer whose registered name ends + * with [target]. */ @Synchronized fun dumpTarget( @@ -141,19 +143,14 @@ open class DumpManager @Inject constructor() { args: Array, tailLength: Int, ) { - for (dumpable in dumpables.values) { - if (dumpable.name.endsWith(target)) { - dumpDumpable(dumpable, pw, args) - return + sequence { + findBestTargetMatch(dumpables, target)?.let { + yield(it.name to { dumpDumpable(it, pw, args) }) } - } - - for (buffer in buffers.values) { - if (buffer.name.endsWith(target)) { - dumpBuffer(buffer, pw, tailLength) - return + findBestTargetMatch(buffers, target)?.let { + yield(it.name to { dumpBuffer(it, pw, tailLength) }) } - } + }.sortedBy { it.first }.minByOrNull { it.first.length }?.second?.invoke() } @Synchronized @@ -162,11 +159,8 @@ open class DumpManager @Inject constructor() { protoDump: SystemUIProtoDump, args: Array ) { - for (dumpable in dumpables.values) { - if (dumpable.dumpable is ProtoDumpable && dumpable.name.endsWith(target)) { - dumpProtoDumpable(dumpable.dumpable, protoDump, args) - return - } + findBestProtoTargetMatch(dumpables, target)?.let { + dumpProtoDumpable(it, protoDump, args) } } @@ -303,6 +297,22 @@ open class DumpManager @Inject constructor() { val existingDumpable = dumpables[name]?.dumpable ?: buffers[name]?.dumpable return existingDumpable == null || newDumpable == existingDumpable } + + private fun findBestTargetMatch(map: Map, target: String): V? = map + .asSequence() + .filter { it.key.endsWith(target) } + .minByOrNull { it.key.length } + ?.value + + private fun findBestProtoTargetMatch( + map: Map>, + target: String + ): ProtoDumpable? = map + .asSequence() + .filter { it.key.endsWith(target) } + .filter { it.value.dumpable is ProtoDumpable } + .minByOrNull { it.key.length } + ?.value?.dumpable as? ProtoDumpable } private data class RegisteredDumpable( diff --git a/packages/SystemUI/tests/src/com/android/systemui/dump/DumpManagerTest.kt b/packages/SystemUI/tests/src/com/android/systemui/dump/DumpManagerTest.kt index 0c5a74cd39fa3..55826141739fb 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/dump/DumpManagerTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/dump/DumpManagerTest.kt @@ -60,16 +60,14 @@ class DumpManagerTest : SysuiTestCase() { // WHEN a dumpable is dumped explicitly val args = arrayOf() - dumpManager.dumpTarget("dumpable2", pw, arrayOf(), tailLength = 0) + dumpManager.dumpTarget("dumpable2", pw, args, tailLength = 0) // THEN only the requested one has their dump() method called - verify(dumpable1, never()) - .dump(any(PrintWriter::class.java), any(Array::class.java)) + verify(dumpable1, never()).dump(any(), any()) verify(dumpable2).dump(pw, args) - verify(dumpable3, never()) - .dump(any(PrintWriter::class.java), any(Array::class.java)) - verify(buffer1, never()).dump(any(PrintWriter::class.java), anyInt()) - verify(buffer2, never()).dump(any(PrintWriter::class.java), anyInt()) + verify(dumpable3, never()).dump(any(), any()) + verify(buffer1, never()).dump(any(), anyInt()) + verify(buffer2, never()).dump(any(), anyInt()) } @Test @@ -82,17 +80,15 @@ class DumpManagerTest : SysuiTestCase() { dumpManager.registerBuffer("buffer2", buffer2) // WHEN a buffer is dumped explicitly - dumpManager.dumpTarget("buffer1", pw, arrayOf(), tailLength = 14) + val args = arrayOf() + dumpManager.dumpTarget("buffer1", pw, args, tailLength = 14) // THEN only the requested one has their dump() method called - verify(dumpable1, never()) - .dump(any(PrintWriter::class.java), any(Array::class.java)) - verify(dumpable2, never()) - .dump(any(PrintWriter::class.java), any(Array::class.java)) - verify(dumpable2, never()) - .dump(any(PrintWriter::class.java), any(Array::class.java)) + verify(dumpable1, never()).dump(any(), any()) + verify(dumpable2, never()).dump(any(), any()) + verify(dumpable3, never()).dump(any(), any()) verify(buffer1).dump(pw, tailLength = 14) - verify(buffer2, never()).dump(any(PrintWriter::class.java), anyInt()) + verify(buffer2, never()).dump(any(), anyInt()) } @Test @@ -108,6 +104,122 @@ class DumpManagerTest : SysuiTestCase() { verify(dumpable1).dump(pw, args) } + @Test + fun testDumpTarget_selectsShortestNamedDumpable() { + // GIVEN a variety of registered dumpables and buffers + dumpManager.registerCriticalDumpable("first-dumpable", dumpable1) + dumpManager.registerCriticalDumpable("scnd-dumpable", dumpable2) + dumpManager.registerCriticalDumpable("third-dumpable", dumpable3) + + // WHEN a dumpable is dumped by a suffix that matches multiple options + val args = arrayOf() + dumpManager.dumpTarget("dumpable", pw, args, tailLength = 0) + + // THEN the matching dumpable with the shorter name is dumped + verify(dumpable1, never()).dump(any(), any()) + verify(dumpable2).dump(pw, args) + verify(dumpable3, never()).dump(any(), any()) + } + + @Test + fun testDumpTarget_selectsShortestNamedBuffer() { + // GIVEN a variety of registered dumpables and buffers + dumpManager.registerBuffer("first-buffer", buffer1) + dumpManager.registerBuffer("scnd-buffer", buffer2) + + // WHEN a dumpable is dumped by a suffix that matches multiple options + val args = arrayOf() + dumpManager.dumpTarget("buffer", pw, args, tailLength = 14) + + // THEN the matching buffer with the shorter name is dumped + verify(buffer1, never()).dump(any(), anyInt()) + verify(buffer2).dump(pw, tailLength = 14) + } + + @Test + fun testDumpTarget_selectsShortestNamedMatch_dumpable() { + // GIVEN a variety of registered dumpables and buffers + dumpManager.registerCriticalDumpable("dumpable1", dumpable1) + dumpManager.registerCriticalDumpable("dumpable2", dumpable2) + dumpManager.registerCriticalDumpable("dumpable3", dumpable3) + dumpManager.registerBuffer("big-buffer1", buffer1) + dumpManager.registerBuffer("big-buffer2", buffer2) + + // WHEN a dumpable is dumped by a suffix that matches multiple options + val args = arrayOf() + dumpManager.dumpTarget("2", pw, args, tailLength = 14) + + // THEN the matching dumpable with the shorter name is dumped + verify(dumpable1, never()).dump(any(), any()) + verify(dumpable2).dump(pw, args) + verify(dumpable3, never()).dump(any(), any()) + verify(buffer1, never()).dump(any(), anyInt()) + verify(buffer2, never()).dump(any(), anyInt()) + } + + @Test + fun testDumpTarget_selectsShortestNamedMatch_buffer() { + // GIVEN a variety of registered dumpables and buffers + dumpManager.registerCriticalDumpable("dumpable1", dumpable1) + dumpManager.registerCriticalDumpable("dumpable2", dumpable2) + dumpManager.registerCriticalDumpable("dumpable3", dumpable3) + dumpManager.registerBuffer("buffer1", buffer1) + dumpManager.registerBuffer("buffer2", buffer2) + + // WHEN a dumpable is dumped by a suffix that matches multiple options + val args = arrayOf() + dumpManager.dumpTarget("2", pw, args, tailLength = 14) + + // THEN the matching buffer with the shorter name is dumped + verify(dumpable1, never()).dump(any(), any()) + verify(dumpable2, never()).dump(any(), any()) + verify(dumpable3, never()).dump(any(), any()) + verify(buffer1, never()).dump(any(), anyInt()) + verify(buffer2).dump(pw, tailLength = 14) + } + + @Test + fun testDumpTarget_selectsTheAlphabeticallyFirstShortestMatch_dumpable() { + // GIVEN a variety of registered dumpables and buffers + dumpManager.registerCriticalDumpable("d1x", dumpable1) + dumpManager.registerCriticalDumpable("d2x", dumpable2) + dumpManager.registerCriticalDumpable("a3x", dumpable3) + dumpManager.registerBuffer("ab1x", buffer1) + dumpManager.registerBuffer("b2x", buffer2) + + // WHEN a dumpable is dumped by a suffix that matches multiple options + val args = arrayOf() + dumpManager.dumpTarget("x", pw, args, tailLength = 14) + + // THEN the alphabetically first dumpable/buffer (of the 3 letter names) is dumped + verify(dumpable1, never()).dump(any(), any()) + verify(dumpable2, never()).dump(any(), any()) + verify(dumpable3).dump(pw, args) + verify(buffer1, never()).dump(any(), anyInt()) + verify(buffer2, never()).dump(any(), anyInt()) + } + + @Test + fun testDumpTarget_selectsTheAlphabeticallyFirstShortestMatch_buffer() { + // GIVEN a variety of registered dumpables and buffers + dumpManager.registerCriticalDumpable("d1x", dumpable1) + dumpManager.registerCriticalDumpable("d2x", dumpable2) + dumpManager.registerCriticalDumpable("az1x", dumpable3) + dumpManager.registerBuffer("b1x", buffer1) + dumpManager.registerBuffer("b2x", buffer2) + + // WHEN a dumpable is dumped by a suffix that matches multiple options + val args = arrayOf() + dumpManager.dumpTarget("x", pw, args, tailLength = 14) + + // THEN the alphabetically first dumpable/buffer (of the 3 letter names) is dumped + verify(dumpable1, never()).dump(any(), any()) + verify(dumpable2, never()).dump(any(), any()) + verify(dumpable3, never()).dump(any(), any()) + verify(buffer1).dump(pw, tailLength = 14) + verify(buffer2, never()).dump(any(), anyInt()) + } + @Test fun testDumpDumpables() { // GIVEN a variety of registered dumpables and buffers @@ -125,8 +237,8 @@ class DumpManagerTest : SysuiTestCase() { verify(dumpable1).dump(pw, args) verify(dumpable2).dump(pw, args) verify(dumpable3).dump(pw, args) - verify(buffer1, never()).dump(any(PrintWriter::class.java), anyInt()) - verify(buffer2, never()).dump(any(PrintWriter::class.java), anyInt()) + verify(buffer1, never()).dump(any(), anyInt()) + verify(buffer2, never()).dump(any(), anyInt()) } @Test @@ -142,12 +254,9 @@ class DumpManagerTest : SysuiTestCase() { dumpManager.dumpBuffers(pw, tailLength = 1) // THEN all buffers are dumped (and no dumpables) - verify(dumpable1, never()) - .dump(any(PrintWriter::class.java), any(Array::class.java)) - verify(dumpable2, never()) - .dump(any(PrintWriter::class.java), any(Array::class.java)) - verify(dumpable3, never()) - .dump(any(PrintWriter::class.java), any(Array::class.java)) + verify(dumpable1, never()).dump(any(), any()) + verify(dumpable2, never()).dump(any(), any()) + verify(dumpable3, never()).dump(any(), any()) verify(buffer1).dump(pw, tailLength = 1) verify(buffer2).dump(pw, tailLength = 1) } @@ -168,10 +277,9 @@ class DumpManagerTest : SysuiTestCase() { // THEN only critical modules are dumped (and no buffers) verify(dumpable1).dump(pw, args) verify(dumpable2).dump(pw, args) - verify(dumpable3, never()) - .dump(any(PrintWriter::class.java), any(Array::class.java)) - verify(buffer1, never()).dump(any(PrintWriter::class.java), anyInt()) - verify(buffer2, never()).dump(any(PrintWriter::class.java), anyInt()) + verify(dumpable3, never()).dump(any(), any()) + verify(buffer1, never()).dump(any(), anyInt()) + verify(buffer2, never()).dump(any(), anyInt()) } @Test @@ -188,10 +296,8 @@ class DumpManagerTest : SysuiTestCase() { dumpManager.dumpNormal(pw, args, tailLength = 2) // THEN the normal module and all buffers are dumped - verify(dumpable1, never()) - .dump(any(PrintWriter::class.java), any(Array::class.java)) - verify(dumpable2, never()) - .dump(any(PrintWriter::class.java), any(Array::class.java)) + verify(dumpable1, never()).dump(any(), any()) + verify(dumpable2, never()).dump(any(), any()) verify(dumpable3).dump(pw, args) verify(buffer1).dump(pw, tailLength = 2) verify(buffer2).dump(pw, tailLength = 2) @@ -213,9 +319,7 @@ class DumpManagerTest : SysuiTestCase() { // THEN the unregistered dumpables (both normal and critical) are not dumped verify(dumpable1).dump(pw, args) - verify(dumpable2, never()) - .dump(any(PrintWriter::class.java), any(Array::class.java)) - verify(dumpable3, never()) - .dump(any(PrintWriter::class.java), any(Array::class.java)) + verify(dumpable2, never()).dump(any(), any()) + verify(dumpable3, never()).dump(any(), any()) } }