Merge pull request #868 from square/sedwards/proto-conflate

Add Conflate Stale Renderings Runtime

Author: steve-the-edwards

.github/workflows/kotlin.yml

@@ -100,6 +100,34 @@ jobs :
         with :
           report_paths : '**/build/test-results/test/TEST-*.xml'
 
+  jvm-conflate-runtime-test :
+    name : Conflate Stale Renderings Runtime JVM Tests
+    runs-on : ubuntu-latest
+    timeout-minutes : 20
+    steps :
+      - uses : actions/checkout@v3
+      - uses : gradle/wrapper-validation-action@v1
+      - name : set up JDK 11
+        uses : actions/setup-java@v3
+        with :
+          distribution : 'zulu'
+          java-version : 11
+
+      ## Actual task
+      - uses : gradle/gradle-build-action@v2
+        name : Check with Gradle
+        with :
+          arguments : |
+            jvmTest --stacktrace --continue -Pworkflow.runtime=conflate
+          cache-read-only : false
+
+      # Report as Github Pull Request Check.
+      - name : Publish Test Report
+        uses : mikepenz/action-junit-report@v3
+        if : always() # always run even if the previous step fails
+        with :
+          report_paths : '**/build/test-results/test/TEST-*.xml'
+
   ios-tests :
     name : iOS Tests
     runs-on : macos-latest
@@ -228,6 +256,54 @@ jobs :
           name : instrumentation-test-results-${{ matrix.api-level }}
           path : ./**/build/reports/androidTests/connected/**
 
+  conflate-renderings-instrumentation-tests :
+    name : Conflate Stale Renderings Instrumentation tests
+    runs-on : macos-latest
+    timeout-minutes : 45
+    strategy :
+      # Allow tests to continue on other devices if they fail on one device.
+      fail-fast : false
+      matrix :
+        api-level :
+          - 29
+      # Unclear that older versions actually honor command to disable animation.
+      # Newer versions are reputed to be too slow: https://github.com/ReactiveCircus/android-emulator-runner/issues/222
+    steps :
+      - uses : actions/checkout@v3
+      - name : set up JDK 11
+        uses : actions/setup-java@v3
+        with :
+          distribution : 'zulu'
+          java-version : 11
+
+      ## Build before running tests, using cache.
+      - uses: gradle/gradle-build-action@v2
+        name : Build instrumented tests
+        with :
+          # Unfortunately I don't think we can key this cache based on our project property so
+          # we clean and rebuild.
+          arguments : |
+            clean assembleDebugAndroidTest --stacktrace -Pworkflow.runtime=conflate
+          cache-read-only: false
+
+      ## Actual task
+      - name : Instrumentation Tests
+        uses : reactivecircus/android-emulator-runner@v2
+        with :
+          # @ychescale9 suspects Galaxy Nexus is the fastest one
+          profile : Galaxy Nexus
+          api-level : ${{ matrix.api-level }}
+          arch : x86_64
+          # Skip the benchmarks as this is running on emulators
+          script : ./gradlew connectedCheck -x :benchmarks:dungeon-benchmark:connectedCheck -x :benchmarks:performance-poetry:complex-benchmark:connectedCheck -x :benchmarks:performance-poetry:complex-poetry:connectedCheck --stacktrace -Pworkflow.runtime=conflate
+
+      - name : Upload results
+        if : ${{ always() }}
+        uses : actions/upload-artifact@v3
+        with :
+          name : instrumentation-test-results-${{ matrix.api-level }}
+          path : ./**/build/reports/androidTests/connected/**
+
   upload-to-mobiledev :
     name : mobile.dev | Build & Upload
     runs-on : ubuntu-latest

workflow-config/config-android/src/main/java/com/squareup/workflow1/config/AndroidRuntimeConfigTools.kt

@@ -1,6 +1,7 @@
 package com.squareup.workflow1.config
 
 import com.squareup.workflow1.RuntimeConfig
+import com.squareup.workflow1.RuntimeConfig.ConflateStaleRenderings
 import com.squareup.workflow1.RuntimeConfig.RenderPerAction
 import com.squareup.workflow1.WorkflowExperimentalRuntime
 
@@ -17,12 +18,15 @@ public class AndroidRuntimeConfigTools {
      * this function, and then pass that to the call to [renderWorkflowIn] as the [RuntimeConfig].
      *
      * Current options are:
+     * "conflate" : [ConflateStaleRenderings] Process all queued actions before passing rendering
+     *      to the UI layer.
      * "baseline" : [RenderPerAction] Original Workflow Runtime. Note that this doesn't need to
      *      be specified as it is the current default and is assumed by this utility.
      */
     @WorkflowExperimentalRuntime
     public fun getAppWorkflowRuntimeConfig(): RuntimeConfig {
       return when (BuildConfig.WORKFLOW_RUNTIME) {
+        "conflate" -> ConflateStaleRenderings
         else -> RenderPerAction
       }
     }

workflow-config/config-jvm/src/main/java/com/squareup/workflow1/config/JvmTestRuntimeConfigTools.kt

@@ -1,6 +1,7 @@
 package com.squareup.workflow1.config
 
 import com.squareup.workflow1.RuntimeConfig
+import com.squareup.workflow1.RuntimeConfig.ConflateStaleRenderings
 import com.squareup.workflow1.RuntimeConfig.RenderPerAction
 import com.squareup.workflow1.WorkflowExperimentalRuntime
 
@@ -16,16 +17,18 @@ public class JvmTestRuntimeConfigTools {
      * [RuntimeConfig].
      *
      * Current options are:
+     * "conflate" : [ConflateStaleRenderings] Process all queued actions before passing rendering
+     *      to the UI layer.
      * "baseline" : [RenderPerAction] Original Workflow Runtime. Note that this doesn't need to
      *      be specified as it is the current default and is assumed by this utility.
      */
     @OptIn(WorkflowExperimentalRuntime::class)
     public fun getTestRuntimeConfig(): RuntimeConfig {
-      return RenderPerAction
-      // val runtimeConfig = System.getProperty("workflow.runtime", "baseline")
-      // return when (runtimeConfig) {
-      //   else -> RenderPerAction
-      // }
+      val runtimeConfig = System.getProperty("workflow.runtime", "baseline")
+      return when (runtimeConfig) {
+        "conflate" -> ConflateStaleRenderings
+        else -> RenderPerAction
+      }
     }
   }
 }

workflow-core/api/workflow-core.api

@@ -1,6 +1,10 @@
 public abstract interface class com/squareup/workflow1/ActionProcessingResult {
 }
 
+public final class com/squareup/workflow1/ActionsExhausted : com/squareup/workflow1/ActionProcessingResult {
+	public static final field INSTANCE Lcom/squareup/workflow1/ActionsExhausted;
+}
+
 public abstract interface class com/squareup/workflow1/BaseRenderContext {
 	public abstract fun eventHandler (Lkotlin/jvm/functions/Function0;Lkotlin/jvm/functions/Function10;)Lkotlin/jvm/functions/Function9;
 	public abstract fun eventHandler (Lkotlin/jvm/functions/Function0;Lkotlin/jvm/functions/Function11;)Lkotlin/jvm/functions/Function10;

workflow-core/src/commonMain/kotlin/com/squareup/workflow1/WorkflowAction.kt

@@ -119,6 +119,8 @@ public sealed interface ActionProcessingResult
 
 public object PropsUpdated : ActionProcessingResult
 
+public object ActionsExhausted : ActionProcessingResult
+
 /** Wrapper around a potentially-nullable [OutputT] value. */
 public class WorkflowOutput<out OutputT>(public val value: OutputT) : ActionProcessingResult {
   override fun toString(): String = "WorkflowOutput($value)"

workflow-runtime/api/workflow-runtime.api

@@ -28,6 +28,10 @@ public final class com/squareup/workflow1/RuntimeConfig$Companion {
 	public final fun getDEFAULT_CONFIG ()Lcom/squareup/workflow1/RuntimeConfig;
 }
 
+public final class com/squareup/workflow1/RuntimeConfig$ConflateStaleRenderings : com/squareup/workflow1/RuntimeConfig {
+	public static final field INSTANCE Lcom/squareup/workflow1/RuntimeConfig$ConflateStaleRenderings;
+}
+
 public final class com/squareup/workflow1/RuntimeConfig$RenderPerAction : com/squareup/workflow1/RuntimeConfig {
 	public static final field INSTANCE Lcom/squareup/workflow1/RuntimeConfig$RenderPerAction;
 }
@@ -195,7 +199,7 @@ public final class com/squareup/workflow1/internal/SubtreeManager : com/squareup
 	public final fun commitRenderedChildren ()V
 	public final fun createChildSnapshots ()Ljava/util/Map;
 	public fun render (Lcom/squareup/workflow1/Workflow;Ljava/lang/Object;Ljava/lang/String;Lkotlin/jvm/functions/Function1;)Ljava/lang/Object;
-	public final fun tickChildren (Lkotlinx/coroutines/selects/SelectBuilder;)V
+	public final fun tickChildren (Lkotlinx/coroutines/selects/SelectBuilder;)Z
 }
 
 public final class com/squareup/workflow1/internal/SystemUtilsKt {
@@ -235,7 +239,7 @@ public final class com/squareup/workflow1/internal/WorkflowNode : com/squareup/w
 	public final fun render (Lcom/squareup/workflow1/StatefulWorkflow;Ljava/lang/Object;)Ljava/lang/Object;
 	public fun runningSideEffect (Ljava/lang/String;Lkotlin/jvm/functions/Function2;)V
 	public final fun snapshot (Lcom/squareup/workflow1/StatefulWorkflow;)Lcom/squareup/workflow1/TreeSnapshot;
-	public final fun tick (Lkotlinx/coroutines/selects/SelectBuilder;)V
+	public final fun tick (Lkotlinx/coroutines/selects/SelectBuilder;)Z
 	public fun toString ()Ljava/lang/String;
 }
 
@@ -272,6 +276,7 @@ public final class com/squareup/workflow1/internal/WorkflowRunner {
 	public final fun cancelRuntime (Ljava/util/concurrent/CancellationException;)V
 	public static synthetic fun cancelRuntime$default (Lcom/squareup/workflow1/internal/WorkflowRunner;Ljava/util/concurrent/CancellationException;ILjava/lang/Object;)V
 	public final fun nextRendering ()Lcom/squareup/workflow1/RenderingAndSnapshot;
-	public final fun processAction (Lkotlin/coroutines/Continuation;)Ljava/lang/Object;
+	public final fun processAction (ZLkotlin/coroutines/Continuation;)Ljava/lang/Object;
+	public static synthetic fun processAction$default (Lcom/squareup/workflow1/internal/WorkflowRunner;ZLkotlin/coroutines/Continuation;ILjava/lang/Object;)Ljava/lang/Object;
 }
 

workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/RenderWorkflow.kt

@@ -1,5 +1,6 @@
 package com.squareup.workflow1
 
+import com.squareup.workflow1.RuntimeConfig.ConflateStaleRenderings
 import com.squareup.workflow1.internal.WorkflowRunner
 import com.squareup.workflow1.internal.chained
 import kotlinx.coroutines.CancellationException
@@ -101,7 +102,7 @@ import kotlinx.coroutines.launch
  * A [StateFlow] of [RenderingAndSnapshot]s that will emit any time the root workflow creates a new
  * rendering.
  */
-@OptIn(ExperimentalCoroutinesApi::class)
+@OptIn(ExperimentalCoroutinesApi::class, WorkflowExperimentalRuntime::class)
 public fun <PropsT, OutputT, RenderingT> renderWorkflowIn(
   workflow: Workflow<PropsT, OutputT, RenderingT>,
   scope: CoroutineScope,
@@ -133,21 +134,60 @@ public fun <PropsT, OutputT, RenderingT> renderWorkflowIn(
     }
   )
 
+  suspend fun <PropsT, OutputT, RenderingT> renderAndEmitOutput(
+    runner: WorkflowRunner<PropsT, OutputT, RenderingT>,
+    actionResult: ActionProcessingResult?,
+    onOutput: suspend (OutputT) -> Unit
+  ): RenderingAndSnapshot<RenderingT> {
+    val nextRenderAndSnapshot = runner.nextRendering()
+
+    when (actionResult) {
+      is WorkflowOutput<*> -> {
+        @Suppress("UNCHECKED_CAST")
+        (actionResult as? WorkflowOutput<OutputT>)?.let {
+          onOutput(it.value)
+        }
+      }
+      else -> {} // no -op
+    }
+
+    return nextRenderAndSnapshot
+  }
+
   scope.launch {
     while (isActive) {
-      // It might look weird to start by consuming the output before getting the rendering below,
+      lateinit var nextRenderAndSnapshot: RenderingAndSnapshot<RenderingT>
+      // It might look weird to start by processing an action before getting the rendering below,
       // but remember the first render pass already occurred above, before this coroutine was even
       // launched.
-      val output: WorkflowOutput<OutputT>? = runner.processAction()
+      var actionResult: ActionProcessingResult? = runner.processAction()
 
-      // After resuming from runner.nextOutput() our coroutine could now be cancelled, check so we
-      // don't surprise anyone with an unexpected rendering pass. Show's over, go home.
+      // After resuming from runner.processAction() our coroutine could now be cancelled, check so
+      // we don't surprise anyone with an unexpected rendering pass. Show's over, go home.
       if (!isActive) return@launch
 
-      // After receiving an output, the next render pass must be done before emitting that output,
-      // so that the workflow states appear consistent to observers of the outputs and renderings.
-      renderingsAndSnapshots.value = runner.nextRendering()
-      output?.let { onOutput(it.value) }
+      // If the action did produce an Output, we send it immediately after the render pass.
+      nextRenderAndSnapshot = renderAndEmitOutput(runner, actionResult, onOutput)
+
+      if (runtimeConfig == ConflateStaleRenderings) {
+        // With this runtime modification, we do not pass renderings we know to be stale. This
+        // means that we may be calling onOutput out of sync with the update of the UI. Output
+        // is an event though, and should always occur immediately - i.e. it cannot be stale.
+        while (actionResult != ActionsExhausted) {
+          // We have more actions we can process, so this rendering is stale.
+          actionResult = runner.processAction(waitForAnAction = false)
+
+          if (!isActive) return@launch
+
+          // If no actions processed, then no new rendering needed.
+          if (actionResult == ActionsExhausted) break
+
+          nextRenderAndSnapshot = renderAndEmitOutput(runner, actionResult, onOutput)
+        }
+      }
+
+      // Pass on to the UI.
+      renderingsAndSnapshots.value = nextRenderAndSnapshot
     }
   }
 

workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/RuntimeConfig.kt

@@ -25,6 +25,12 @@ public sealed interface RuntimeConfig {
    */
   public object RenderPerAction : RuntimeConfig
 
+  /**
+   * If we have more actions to process, do so before passing the rendering to the UI layer.
+   */
+  @WorkflowExperimentalRuntime
+  public object ConflateStaleRenderings : RuntimeConfig
+
   public companion object {
     public val DEFAULT_CONFIG: RuntimeConfig = RenderPerAction
   }

workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/internal/SubtreeManager.kt

@@ -133,11 +133,17 @@ internal class SubtreeManager<PropsT, StateT, OutputT>(
   /**
    * Uses [selector] to invoke [WorkflowNode.tick] for every running child workflow this instance
    * is managing.
+   *
+   * @return [Boolean] whether or not the children action queues are empty.
    */
-  fun tickChildren(selector: SelectBuilder<ActionProcessingResult?>) {
+  fun tickChildren(selector: SelectBuilder<ActionProcessingResult?>): Boolean {
+    var empty = true
     children.forEachActive { child ->
-      child.workflowNode.tick(selector)
+      // Do this separately so the compiler doesn't avoid it if empty is already false.
+      val childEmpty = child.workflowNode.tick(selector)
+      empty = childEmpty && empty
     }
+    return empty
   }
 
   fun createChildSnapshots(): Map<WorkflowNodeId, TreeSnapshot> {

workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/internal/WorkflowNode.kt

@@ -18,6 +18,7 @@ import kotlinx.coroutines.CancellationException
 import kotlinx.coroutines.CoroutineName
 import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.CoroutineStart.LAZY
+import kotlinx.coroutines.ExperimentalCoroutinesApi
 import kotlinx.coroutines.Job
 import kotlinx.coroutines.cancel
 import kotlinx.coroutines.channels.Channel
@@ -142,24 +143,32 @@ internal class WorkflowNode<PropsT, StateT, OutputT, RenderingT>(
   }
 
   /**
-   * Gets the next [output][OutputT] from the state machine.
+   * Gets the next [result][ActionProcessingResult] from the state machine. This will be an
+   * [OutputT] or null.
    *
    * Walk the tree of state machines, asking each one to wait for its next event. If something happen
    * that results in an output, that output is returned. Null means something happened that requires
    * a re-render, e.g. my state changed or a child state changed.
    *
    * It is an error to call this method after calling [cancel].
+   *
+   * @return [Boolean] whether or not the queues were empty for this node and its children at the
+   *    time of suspending.
    */
-  fun tick(selector: SelectBuilder<ActionProcessingResult?>) {
+  @OptIn(ExperimentalCoroutinesApi::class)
+  fun tick(selector: SelectBuilder<ActionProcessingResult?>): Boolean {
     // Listen for any child workflow updates.
-    subtreeManager.tickChildren(selector)
+    var empty = subtreeManager.tickChildren(selector)
+
+    empty = empty && (eventActionsChannel.isEmpty || eventActionsChannel.isClosedForReceive)
 
     // Listen for any events.
     with(selector) {
       eventActionsChannel.onReceive { action ->
         return@onReceive applyAction(action)
       }
     }
+    return empty
   }
 
   /**