From 3a4d67b965e8f5b0a025c263c2f4799db366bb8a Mon Sep 17 00:00:00 2001 From: Jordan Silva Date: Mon, 21 Oct 2024 10:21:56 +0000 Subject: [PATCH] Revert "Update OverviewCommandHelper to use Executors.MAIN to reduce the percentage of missed frames" This CL didn't improve the performance metrics significantly enough to use Executors.MAIN. b/366077002#comment12 This reverts commit 7eae20bcb11ab487ae2b16083ba8b4ba37463c85. Reason for revert: 366077002 Change-Id: Ic92b83a65aef7f8cd5c00110fb1ab7343d4b12b6 --- .../quickstep/OverviewCommandHelper.kt | 10 +++++--- .../quickstep/OverviewCommandHelperTest.kt | 25 ++++--------------- 2 files changed, 11 insertions(+), 24 deletions(-) diff --git a/quickstep/src/com/android/quickstep/OverviewCommandHelper.kt b/quickstep/src/com/android/quickstep/OverviewCommandHelper.kt index e23947b62d..461f963dc1 100644 --- a/quickstep/src/com/android/quickstep/OverviewCommandHelper.kt +++ b/quickstep/src/com/android/quickstep/OverviewCommandHelper.kt @@ -53,7 +53,6 @@ import com.android.systemui.shared.recents.model.ThumbnailData import com.android.systemui.shared.system.InteractionJankMonitorWrapper import java.io.PrintWriter import java.util.concurrent.ConcurrentLinkedDeque -import java.util.concurrent.Executor import kotlin.coroutines.resume import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.SupervisorJob @@ -70,7 +69,6 @@ constructor( private val overviewComponentObserver: OverviewComponentObserver, private val taskAnimationManager: TaskAnimationManager, private val dispatcherProvider: DispatcherProvider = ProductionDispatchers, - private val uiExecutor: Executor = Executors.MAIN_EXECUTOR, ) { private val coroutineScope = CoroutineScope(SupervisorJob() + dispatcherProvider.default) @@ -87,7 +85,7 @@ constructor( get() = overviewComponentObserver.containerInterface private val visibleRecentsView: RecentsView<*, *>? - get() = containerInterface.getVisibleRecentsView() + get() = containerInterface.getVisibleRecentsView>() /** * Adds a command to be executed next, after all pending tasks are completed. Max commands that @@ -107,7 +105,11 @@ constructor( if (commandQueue.size == 1) { Log.d(TAG, "execute: $command - queue size: ${commandQueue.size}") - uiExecutor.execute { processNextCommand() } + if (enableOverviewCommandHelperTimeout()) { + coroutineScope.launch(dispatcherProvider.main) { processNextCommand() } + } else { + Executors.MAIN_EXECUTOR.execute { processNextCommand() } + } } else { Log.d(TAG, "not executed: $command - queue size: ${commandQueue.size}") } diff --git a/quickstep/tests/multivalentTests/src/com/android/quickstep/OverviewCommandHelperTest.kt b/quickstep/tests/multivalentTests/src/com/android/quickstep/OverviewCommandHelperTest.kt index b0db737c94..0ae710f866 100644 --- a/quickstep/tests/multivalentTests/src/com/android/quickstep/OverviewCommandHelperTest.kt +++ b/quickstep/tests/multivalentTests/src/com/android/quickstep/OverviewCommandHelperTest.kt @@ -41,6 +41,7 @@ import org.junit.Test import org.junit.runner.RunWith import org.mockito.Mockito.doAnswer import org.mockito.Mockito.spy +import org.mockito.Mockito.`when` import org.mockito.kotlin.any import org.mockito.kotlin.mock @@ -55,12 +56,10 @@ class OverviewCommandHelperTest { private val testScope = TestScope(dispatcher) private var pendingCallbacksWithDelays = mutableListOf() - private lateinit var pendingCommandsToExecute: MutableList @Suppress("UNCHECKED_CAST") @Before fun setup() { - pendingCommandsToExecute = mutableListOf() setFlagsRule.setFlags(true, Flags.FLAG_ENABLE_OVERVIEW_COMMAND_HELPER_TIMEOUT) sut = @@ -69,8 +68,7 @@ class OverviewCommandHelperTest { touchInteractionService = mock(), overviewComponentObserver = mock(), taskAnimationManager = mock(), - dispatcherProvider = TestDispatcherProvider(dispatcher), - uiExecutor = { runnable -> pendingCommandsToExecute += runnable }, + dispatcherProvider = TestDispatcherProvider(dispatcher) ) ) @@ -96,21 +94,12 @@ class OverviewCommandHelperTest { pendingCallbacksWithDelays.add(delayInMillis) } - /** - * This function runs all the pending commands from the Executor for testing purposes. Replacing - * the uiExecutor allows the test to execute the command queue manually, making it possible to - * assert each state of the commands in the queue individually. - */ - private fun executePendingCommands() = pendingCommandsToExecute.forEach { it.run() } - @Test fun whenFirstCommandIsAdded_executeCommandImmediately() = testScope.runTest { // Add command to queue val commandInfo: CommandInfo = sut.addCommand(CommandType.HOME)!! assertThat(commandInfo.status).isEqualTo(CommandStatus.IDLE) - executePendingCommands() - assertThat(commandInfo.status).isEqualTo(CommandStatus.PROCESSING) runCurrent() assertThat(commandInfo.status).isEqualTo(CommandStatus.COMPLETED) } @@ -125,7 +114,7 @@ class OverviewCommandHelperTest { val commandInfo: CommandInfo = sut.addCommand(commandType)!! assertThat(commandInfo.status).isEqualTo(CommandStatus.IDLE) - executePendingCommands() + runCurrent() assertThat(commandInfo.status).isEqualTo(CommandStatus.PROCESSING) advanceTimeBy(200L) @@ -146,14 +135,12 @@ class OverviewCommandHelperTest { val commandInfo2: CommandInfo = sut.addCommand(commandType2)!! assertThat(commandInfo2.status).isEqualTo(CommandStatus.IDLE) - executePendingCommands() + runCurrent() assertThat(commandInfo1.status).isEqualTo(CommandStatus.PROCESSING) assertThat(commandInfo2.status).isEqualTo(CommandStatus.IDLE) advanceTimeBy(101L) assertThat(commandInfo1.status).isEqualTo(CommandStatus.COMPLETED) - - executePendingCommands() assertThat(commandInfo2.status).isEqualTo(CommandStatus.PROCESSING) advanceTimeBy(101L) @@ -174,14 +161,12 @@ class OverviewCommandHelperTest { val commandInfo2: CommandInfo = sut.addCommand(commandType2)!! assertThat(commandInfo2.status).isEqualTo(CommandStatus.IDLE) - executePendingCommands() + runCurrent() assertThat(commandInfo1.status).isEqualTo(CommandStatus.PROCESSING) assertThat(commandInfo2.status).isEqualTo(CommandStatus.IDLE) advanceTimeBy(QUEUE_TIMEOUT) assertThat(commandInfo1.status).isEqualTo(CommandStatus.CANCELED) - - executePendingCommands() assertThat(commandInfo2.status).isEqualTo(CommandStatus.PROCESSING) advanceTimeBy(101)