Cleanup Side Effects; Able to Freeze

Author: steve-the-edwards

benchmarks/performance-poetry/complex-poetry/src/androidTest/java/com/squareup/benchmarks/performance/complex/poetry/ComposeInheritanceTest.kt

@@ -8,6 +8,7 @@ import androidx.test.platform.app.InstrumentationRegistry
 import androidx.test.uiautomator.UiDevice
 import com.squareup.benchmarks.performance.complex.poetry.PerformancePoetryActivity.Companion.EXTRA_PERF_CONFIG_INITIALIZING
 import com.squareup.benchmarks.performance.complex.poetry.PerformancePoetryActivity.Companion.EXTRA_PERF_CONFIG_REPEAT
+import com.squareup.benchmarks.performance.complex.poetry.PerformancePoetryActivity.Companion.EXTRA_RUNTIME_COMPOSE_RUNTIME
 import com.squareup.benchmarks.performance.complex.poetry.PerformancePoetryActivity.Companion.EXTRA_RUNTIME_FRAME_TIMEOUT
 import com.squareup.benchmarks.performance.complex.poetry.cyborgs.landscapeOrientation
 import com.squareup.benchmarks.performance.complex.poetry.cyborgs.openRavenAndNavigate
@@ -16,6 +17,7 @@ import com.squareup.benchmarks.performance.complex.poetry.cyborgs.waitForPoetry
 import com.squareup.benchmarks.performance.complex.poetry.instrumentation.RenderPassCountingInterceptor
 import org.junit.Assert.fail
 import org.junit.Before
+import org.junit.Ignore
 import org.junit.Test
 import org.junit.runner.RunWith
 
@@ -30,7 +32,8 @@ class RenderPassTest {
     val useInitializingState: Boolean,
     val useHighFrequencyRange: Boolean,
     val baselineExpectation: RenderExpectation,
-    val frameTimeoutExpectation: RenderExpectation
+    val frameTimeoutExpectation: RenderExpectation,
+    val frameTimeoutComposeExpectation: RenderExpectation
   )
 
   data class RenderExpectation(
@@ -56,15 +59,15 @@ class RenderPassTest {
   }
 
   @Test fun renderPassCounterBaselineComplexWithInitializingState() {
-    runRenderPassCounter(COMPLEX_INITIALIZING, useFrameTimeout = false)
+    runRenderPassCounter(COMPLEX_INITIALIZING)
   }
 
   @Test fun renderPassCounterBaselineComplexNoInitializingState() {
-    runRenderPassCounter(COMPLEX_NO_INITIALIZING, useFrameTimeout = false)
+    runRenderPassCounter(COMPLEX_NO_INITIALIZING)
   }
 
   @Test fun renderPassCounterBaselineComplexNoInitializingStateHighFrequencyEvents() {
-    runRenderPassCounter(COMPLEX_NO_INITIALIZING_HIGH_FREQUENCY, useFrameTimeout = false)
+    runRenderPassCounter(COMPLEX_NO_INITIALIZING_HIGH_FREQUENCY)
   }
 
   @Test fun renderPassCounterFrameTimeoutComplexWithInitializingState() {
@@ -79,19 +82,41 @@ class RenderPassTest {
     runRenderPassCounter(COMPLEX_NO_INITIALIZING_HIGH_FREQUENCY, useFrameTimeout = true)
   }
 
+  @Ignore("Not sure why but this gets stuck on initializing. Compose doesn't get the next" +
+    " frame when this is started by the test, but it does when running directly.")
+  @Test fun renderPassCounterFrameTimeoutComposeComplexWithInitializingState() {
+    runRenderPassCounter(COMPLEX_INITIALIZING, useFrameTimeout = true, useCompose = true)
+  }
+
+  @Test fun renderPassCounterFrameTimeoutComposeComplexNoInitializingState() {
+    runRenderPassCounter(COMPLEX_NO_INITIALIZING, useFrameTimeout = true, useCompose = true)
+  }
+
+  @Test fun renderPassCounterFrameTimeoutComposeComplexNoInitializingStateHighFrequencyEvents() {
+    runRenderPassCounter(
+      COMPLEX_NO_INITIALIZING_HIGH_FREQUENCY,
+      useFrameTimeout = true,
+      useCompose = true
+    )
+  }
+
   private fun runRenderPassCounter(
     scenario: Scenario,
-    useFrameTimeout: Boolean
+    useFrameTimeout: Boolean = false,
+    useCompose: Boolean = false
   ) {
+    if (useCompose) require(useFrameTimeout) { "Only use Compose Frame Timeout." }
+
     val intent = Intent(context, PerformancePoetryActivity::class.java).apply {
       addFlags(Intent.FLAG_ACTIVITY_NEW_TASK)
       addFlags(Intent.FLAG_ACTIVITY_CLEAR_TASK)
       putExtra(
         EXTRA_PERF_CONFIG_INITIALIZING,
         scenario.useInitializingState
       )
+      putExtra(EXTRA_RUNTIME_FRAME_TIMEOUT, useFrameTimeout)
       if (useFrameTimeout) {
-        putExtra(EXTRA_RUNTIME_FRAME_TIMEOUT, useFrameTimeout)
+        putExtra(EXTRA_RUNTIME_COMPOSE_RUNTIME, useCompose)
       }
       if (scenario.useHighFrequencyRange) {
         putExtra(EXTRA_PERF_CONFIG_REPEAT, PerformancePoetryActivity.HIGH_FREQUENCY_REPEAT_COUNT)
@@ -111,11 +136,19 @@ class RenderPassTest {
     device.openRavenAndNavigate()
 
     val expectation =
-      if (useFrameTimeout) scenario.frameTimeoutExpectation else
+      if (useFrameTimeout) {
+        if (useCompose) {
+          scenario.frameTimeoutComposeExpectation
+        } else {
+          scenario.frameTimeoutExpectation
+        }
+      } else {
         scenario.baselineExpectation
+      }
 
     val title = if (useFrameTimeout) {
-      "Runtime: FrameTimeout; "
+      val usingCompose = if (useCompose) "(Using Compose Optimizations)" else "(No Compose)"
+      "Runtime: FrameTimeout $usingCompose;"
     } else {
       "Runtime: RenderPerAction; "
     } + scenario.title
@@ -227,6 +260,11 @@ class RenderPassTest {
         totalPasses = 41..42,
         freshRenderedNodes = 85..85,
         staleRenderedNodes = 436..436
+      ),
+      frameTimeoutComposeExpectation = RenderExpectation(
+        totalPasses = 41..42,
+        freshRenderedNodes = 85..85,
+        staleRenderedNodes = 436..436
       )
     )
 
@@ -243,6 +281,11 @@ class RenderPassTest {
         totalPasses = 40..41,
         freshRenderedNodes = 83..83,
         staleRenderedNodes = 431..431
+      ),
+      frameTimeoutComposeExpectation = RenderExpectation(
+        totalPasses = 40..41,
+        freshRenderedNodes = 113..113,
+        staleRenderedNodes = 82..82
       )
     )
 
@@ -252,11 +295,11 @@ class RenderPassTest {
      *
      * Physical Pixel 6:
      * ```
-     RenderExpectation(
-     totalPasses = 56..61,
-     freshRenderedNodes = 106..108,
-     staleRenderedNodes = 679..698
-     )
+    RenderExpectation(
+    totalPasses = 56..61,
+    freshRenderedNodes = 106..108,
+    staleRenderedNodes = 679..698
+    )
      * ```
      * We use the values expected in CI for what we commit to the repo.
      */
@@ -270,9 +313,14 @@ class RenderPassTest {
         staleRenderedNodes = 2350..2350
       ),
       frameTimeoutExpectation = RenderExpectation(
-        totalPasses = 64..68,
+        totalPasses = 60..64,
         freshRenderedNodes = 106..108,
         staleRenderedNodes = 679..698
+      ),
+      frameTimeoutComposeExpectation = RenderExpectation(
+        totalPasses = 60..64,
+        freshRenderedNodes = 259..259,
+        staleRenderedNodes = 207..207
       )
     )
 

benchmarks/performance-poetry/complex-poetry/src/androidTest/java/com/squareup/benchmarks/performance/complex/poetry/RenderPassTest.kt

@@ -326,7 +326,7 @@ class PerformancePoemWorkflow(
           .copy(selection = stanzaIndex)
 
         if (stanzaIndex != NO_SELECTED_STANZA) {
-          val previousStanzas = renderProps.stanzas.subList(0, stanzaIndex)
+          val stackedStanzas = renderProps.stanzas.subList(0, stanzaIndex + 1)
             .mapIndexed { index, _ ->
               context.ChildRendering(
                 StanzaWorkflow,
@@ -337,28 +337,13 @@ class PerformancePoemWorkflow(
                 ),
                 key = "$index",
               ) {
-                noAction()
+                when (it) {
+                  CloseStanzas -> ClearSelection(simulatedPerfConfig)
+                  ShowPreviousStanza -> SelectPrevious(simulatedPerfConfig)
+                  ShowNextStanza -> SelectNext(simulatedPerfConfig)
+                }
               }
-            }
-          val visibleStanza = context.ChildRendering(
-            StanzaWorkflow,
-            Props(
-              poem = renderProps,
-              index = stanzaIndex,
-              eventHandlerTag = ActionHandlingTracingInterceptor::keyForTrace
-            ),
-            key = "$stanzaIndex",
-          ) {
-            when (it) {
-              CloseStanzas -> ClearSelection(simulatedPerfConfig)
-              ShowPreviousStanza -> SelectPrevious(simulatedPerfConfig)
-              ShowNextStanza -> SelectNext(simulatedPerfConfig)
-            }
-          }
-
-          val stackedStanzas = visibleStanza.let {
-            (previousStanzas + it).toBackStackScreen<Screen>()
-          }
+            }.toBackStackScreen<Screen>()
 
           return OverviewDetailScreen(
             overviewRendering = BackStackScreen(stanzaListOverview),

benchmarks/performance-poetry/complex-poetry/src/main/java/com/squareup/benchmarks/performance/complex/poetry/PerformancePoemWorkflow.kt

@@ -25,6 +25,7 @@ import com.squareup.workflow1.ui.WorkflowUiExperimentalApi
 import com.squareup.workflow1.ui.container.BackStackScreen
 import kotlinx.coroutines.delay
 import kotlinx.coroutines.flow.MutableStateFlow
+import java.lang.IllegalStateException
 
 /**
  * Version of [PoemsBrowserWorkflow] that takes in a [SimulatedPerfConfig] to control the
@@ -161,6 +162,22 @@ class PerformancePoemsBrowserWorkflow(
     renderState: State,
     context: RenderContext,
   ): OverviewDetailScreen {
+
+    // Again, then entire `Initializing` state is a smell, which is most obvious from the
+    // use of `Worker.from { Unit }`. A Worker doing no work and only shuttling the state
+    // along is usually the sign you have an extraneous state that can be collapsed!
+    // Don't try this at home.
+    if (renderState is Initializing) {
+      context.runningWorker(TraceableWorker.from("BrowserInitializing") { Unit }, "init") {
+        isLoading.value = true
+        action {
+          isLoading.value = false
+          state = NoSelection
+        }
+      }
+      return OverviewDetailScreen(overviewRendering = BackStackScreen(BlankScreen))
+    }
+
     val poemListProps = Props(
       poems = renderProps,
       eventHandlerTag = ActionHandlingTracingInterceptor::keyForTrace
@@ -174,20 +191,6 @@ class PerformancePoemsBrowserWorkflow(
       choosePoem(selected)
     }
     when (renderState) {
-      // Again, then entire `Initializing` state is a smell, which is most obvious from the
-      // use of `Worker.from { Unit }`. A Worker doing no work and only shuttling the state
-      // along is usually the sign you have an extraneous state that can be collapsed!
-      // Don't try this at home.
-      is Initializing -> {
-        context.runningWorker(TraceableWorker.from("BrowserInitializing") { Unit }, "init") {
-        isLoading.value = true
-        action {
-          isLoading.value = false
-          state = NoSelection
-        }
-      }
-        return OverviewDetailScreen(overviewRendering = BackStackScreen(BlankScreen))
-      }
       is NoSelection -> {
         return OverviewDetailScreen(
             overviewRendering = BackStackScreen(
@@ -243,8 +246,10 @@ class PerformancePoemsBrowserWorkflow(
           key = "",
         ) { clearSelection }
       }
+      else -> {
+        throw IllegalStateException("$renderState state is impossible.")
+      }
     }
-
   }
 
   override fun snapshotState(state: State): Snapshot? = null

benchmarks/performance-poetry/complex-poetry/src/main/java/com/squareup/benchmarks/performance/complex/poetry/PerformancePoemsBrowserWorkflow.kt

@@ -84,7 +84,8 @@ class PerformancePoetryActivity : AppCompatActivity() {
     }
 
     val isFrameTimeout = intent.getBooleanExtra(EXTRA_RUNTIME_FRAME_TIMEOUT, false)
-    val runtimeConfig = if (isFrameTimeout) {
+    val useCompose = intent.getBooleanExtra(EXTRA_RUNTIME_COMPOSE_RUNTIME, false)
+    val runtimeConfig = if (true) {
       FrameTimeout(useComposeInRuntime = true)
     } else {
       RenderPerAction
@@ -247,6 +248,8 @@ class PerformancePoetryActivity : AppCompatActivity() {
     const val EXTRA_PERF_CONFIG_DELAY = "complex.poetry.performance.config.delay.length"
     const val EXTRA_RUNTIME_FRAME_TIMEOUT =
       "complex.poetry.performance.config.runtime.frametimeout"
+    const val EXTRA_RUNTIME_COMPOSE_RUNTIME =
+      "complex.poetry.performance.config.runtime.compose"
 
     const val SELECT_ON_TIMEOUT_LOG_NAME =
       "kotlinx.coroutines.selects.SelectBuilderImpl\$onTimeout\$\$inlined\$Runnable"

benchmarks/performance-poetry/complex-poetry/src/main/java/com/squareup/benchmarks/performance/complex/poetry/PerformancePoetryActivity.kt

@@ -2,6 +2,7 @@ package com.squareup.benchmarks.performance.complex.poetry.cyborgs
 
 import android.widget.ImageButton
 import android.widget.ProgressBar
+import androidx.compose.runtime.currentComposer
 import androidx.test.uiautomator.By
 import androidx.test.uiautomator.BySelector
 import androidx.test.uiautomator.UiDevice
@@ -48,6 +49,7 @@ fun UiDevice.resetToRootPoetryList() {
  */
 fun UiDevice.openRavenAndNavigate() {
   waitForIdle()
+  waitForIdle(5_000)
   waitForAndClick(RavenPoemSelector)
   waitForLoadingInterstitial()
   waitForAndClick(By.textStartsWith("Deep into that darkness peering"))