From 72aa1c91f36a041515def1648f7464dd3e70d8bb Mon Sep 17 00:00:00 2001 From: Stephen Edwards Date: Mon, 20 Jan 2025 11:02:55 -0500 Subject: [PATCH 1/4] 1247: Partial Tree Rerendering If PARTIAL_TREE_RENDERING is in the RuntimeConfig then, Track whether or not the state (or state of a descendant) changed in the WorkflowNode. Pass lastRendering if its not. Also adds tests for this behavior and expands the test runtime matrix. Also adds shards for the new runtime for instrumentation tests. --- .github/workflows/kotlin.yml | 48 +++++- .../config/AndroidRuntimeConfigTools.kt | 10 ++ .../config/JvmTestRuntimeConfigTools.kt | 10 ++ workflow-core/api/workflow-core.api | 20 +++ .../com/squareup/workflow1/NullableInitBox.kt | 28 ++++ .../squareup/workflow1/NullableInitBoxTest.kt | 41 +++++ .../com/squareup/workflow1/WorkerTest.kt | 1 - .../workflow1/WorkflowIdentifierTest.kt | 1 - workflow-runtime/api/workflow-runtime.api | 1 + .../com/squareup/workflow1/RuntimeConfig.kt | 16 ++ .../workflow1/internal/WorkflowNode.kt | 68 +++++--- .../workflow1/RenderWorkflowInTest.kt | 145 +++++++++++++++++- .../workflow1/WorkflowOperatorsTest.kt | 20 ++- .../workflow1/internal/WorkflowRunnerTest.kt | 5 +- .../workflow1/WorkflowsLifecycleTests.kt | 5 +- 15 files changed, 391 insertions(+), 28 deletions(-) create mode 100644 workflow-core/src/commonMain/kotlin/com/squareup/workflow1/NullableInitBox.kt create mode 100644 workflow-core/src/commonTest/kotlin/com/squareup/workflow1/NullableInitBoxTest.kt diff --git a/.github/workflows/kotlin.yml b/.github/workflows/kotlin.yml index a1cc582d47..bb54529b77 100644 --- a/.github/workflows/kotlin.yml +++ b/.github/workflows/kotlin.yml @@ -222,6 +222,27 @@ jobs : with : report_paths : '**/build/test-results/test/TEST-*.xml' + jvm-partial-runtime-test: + name: Partial Tree Rendering Only Runtime JVM Tests + runs-on: ubuntu-latest + timeout-minutes: 20 + steps: + - name: Checkout + uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1 + + - name: Check with Gradle + uses: ./.github/actions/gradle-task + with: + task: jvmTest --continue -Pworkflow.runtime=baseline-partial + restore-cache-key: main-build-artifacts + + # Report as GitHub Pull Request Check. + - name: Publish Test Report + uses: mikepenz/action-junit-report@5f47764eec0e1c1f19f40c8e60a5ba47e47015c5 # v4 + if: always() # always run even if the previous step fails + with: + report_paths: '**/build/test-results/test/TEST-*.xml' + jvm-conflate-stateChange-runtime-test : name : Render On State Change Only and Conflate Stale Runtime JVM Tests runs-on : ubuntu-latest @@ -243,6 +264,27 @@ jobs : with : report_paths : '**/build/test-results/test/TEST-*.xml' + jvm-conflate-partial-runtime-test: + name: Render On State Change Only and Conflate Stale Runtime JVM Tests + runs-on: ubuntu-latest + timeout-minutes: 20 + steps: + - name: Checkout + uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1 + + - name: Check with Gradle + uses: ./.github/actions/gradle-task + with: + task: jvmTest --continue -Pworkflow.runtime=conflate-partial + restore-cache-key: main-build-artifacts + + # Report as GitHub Pull Request Check. + - name: Publish Test Report + uses: mikepenz/action-junit-report@5f47764eec0e1c1f19f40c8e60a5ba47e47015c5 # v4 + 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 @@ -349,7 +391,7 @@ jobs : ### shardNum: [ 1, 2, 3 ] ### - runtime : [ conflate, baseline-stateChange, conflate-stateChange ] + runtime : [ conflate, baseline-stateChange, conflate-stateChange, baseline-partial, conflate-partial ] steps : - name: Checkout uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1 @@ -378,8 +420,10 @@ jobs : - ios-tests - js-tests - jvm-conflate-runtime-test - - jvm-conflate-stateChange-runtime-test - jvm-stateChange-runtime-test + - jvm-partial-runtime-test + - jvm-conflate-stateChange-runtime-test + - jvm-conflate-partial-runtime-test - ktlint - performance-tests - runtime-instrumentation-tests diff --git a/workflow-config/config-android/src/main/java/com/squareup/workflow1/config/AndroidRuntimeConfigTools.kt b/workflow-config/config-android/src/main/java/com/squareup/workflow1/config/AndroidRuntimeConfigTools.kt index 93e69595e4..7df4791984 100644 --- a/workflow-config/config-android/src/main/java/com/squareup/workflow1/config/AndroidRuntimeConfigTools.kt +++ b/workflow-config/config-android/src/main/java/com/squareup/workflow1/config/AndroidRuntimeConfigTools.kt @@ -3,6 +3,7 @@ package com.squareup.workflow1.config import com.squareup.workflow1.RuntimeConfig import com.squareup.workflow1.RuntimeConfigOptions import com.squareup.workflow1.RuntimeConfigOptions.CONFLATE_STALE_RENDERINGS +import com.squareup.workflow1.RuntimeConfigOptions.PARTIAL_TREE_RENDERING import com.squareup.workflow1.RuntimeConfigOptions.RENDER_ONLY_WHEN_STATE_CHANGES import com.squareup.workflow1.WorkflowExperimentalRuntime @@ -27,6 +28,9 @@ public class AndroidRuntimeConfigTools { * Then, these can be combined (via '-') with: * "stateChange" : Only re-render when the state of some WorkflowNode has been changed by an * action cascade. + * "partial" : Which includes "stateChange" as well as partial tree rendering, which only + * re-renders each Workflow node if: 1) its state changed; or 2) one of its descendant's state + * changed. * * E.g., "baseline-stateChange" to turn on the stateChange option with the baseline runtime. * @@ -37,6 +41,12 @@ public class AndroidRuntimeConfigTools { "conflate" -> setOf(CONFLATE_STALE_RENDERINGS) "conflate-stateChange" -> setOf(CONFLATE_STALE_RENDERINGS, RENDER_ONLY_WHEN_STATE_CHANGES) "baseline-stateChange" -> setOf(RENDER_ONLY_WHEN_STATE_CHANGES) + "conflate-partial" -> setOf( + CONFLATE_STALE_RENDERINGS, + RENDER_ONLY_WHEN_STATE_CHANGES, + PARTIAL_TREE_RENDERING + ) + "baseline-partial" -> setOf(RENDER_ONLY_WHEN_STATE_CHANGES, PARTIAL_TREE_RENDERING) "", "baseline" -> RuntimeConfigOptions.RENDER_PER_ACTION else -> throw IllegalArgumentException("Unrecognized config \"${BuildConfig.WORKFLOW_RUNTIME}\"") diff --git a/workflow-config/config-jvm/src/main/java/com/squareup/workflow1/config/JvmTestRuntimeConfigTools.kt b/workflow-config/config-jvm/src/main/java/com/squareup/workflow1/config/JvmTestRuntimeConfigTools.kt index 98d3d8d2da..8d3358d352 100644 --- a/workflow-config/config-jvm/src/main/java/com/squareup/workflow1/config/JvmTestRuntimeConfigTools.kt +++ b/workflow-config/config-jvm/src/main/java/com/squareup/workflow1/config/JvmTestRuntimeConfigTools.kt @@ -3,6 +3,7 @@ package com.squareup.workflow1.config import com.squareup.workflow1.RuntimeConfig import com.squareup.workflow1.RuntimeConfigOptions import com.squareup.workflow1.RuntimeConfigOptions.CONFLATE_STALE_RENDERINGS +import com.squareup.workflow1.RuntimeConfigOptions.PARTIAL_TREE_RENDERING import com.squareup.workflow1.RuntimeConfigOptions.RENDER_ONLY_WHEN_STATE_CHANGES import com.squareup.workflow1.WorkflowExperimentalRuntime @@ -27,6 +28,9 @@ public class JvmTestRuntimeConfigTools { * Then, these can be combined (via '-') with: * "stateChange" : Only re-render when the state of some WorkflowNode has been changed by an * action cascade. + * "partial" : Which includes "stateChange" as well as partial tree rendering, which only + * re-renders each Workflow node if: 1) its state changed; or 2) one of its descendant's state + * changed. * * E.g., "baseline-stateChange" to turn on the stateChange option with the baseline runtime. * @@ -38,6 +42,12 @@ public class JvmTestRuntimeConfigTools { "conflate" -> setOf(CONFLATE_STALE_RENDERINGS) "conflate-stateChange" -> setOf(CONFLATE_STALE_RENDERINGS, RENDER_ONLY_WHEN_STATE_CHANGES) "baseline-stateChange" -> setOf(RENDER_ONLY_WHEN_STATE_CHANGES) + "conflate-partial" -> setOf( + CONFLATE_STALE_RENDERINGS, + RENDER_ONLY_WHEN_STATE_CHANGES, + PARTIAL_TREE_RENDERING + ) + "baseline-partial" -> setOf(RENDER_ONLY_WHEN_STATE_CHANGES, PARTIAL_TREE_RENDERING) "", "baseline" -> RuntimeConfigOptions.RENDER_PER_ACTION else -> throw IllegalArgumentException("Unrecognized config \"$runtimeConfig\"") diff --git a/workflow-core/api/workflow-core.api b/workflow-core/api/workflow-core.api index e7467b3a33..3ff8f29942 100644 --- a/workflow-core/api/workflow-core.api +++ b/workflow-core/api/workflow-core.api @@ -77,6 +77,26 @@ public abstract class com/squareup/workflow1/LifecycleWorker : com/squareup/work public final fun run ()Lkotlinx/coroutines/flow/Flow; } +public final class com/squareup/workflow1/NullableInitBox { + public static final synthetic fun box-impl (Ljava/lang/Object;)Lcom/squareup/workflow1/NullableInitBox; + public static fun constructor-impl (Ljava/lang/Object;)Ljava/lang/Object; + public static synthetic fun constructor-impl$default (Ljava/lang/Object;ILkotlin/jvm/internal/DefaultConstructorMarker;)Ljava/lang/Object; + public fun equals (Ljava/lang/Object;)Z + public static fun equals-impl (Ljava/lang/Object;Ljava/lang/Object;)Z + public static final fun equals-impl0 (Ljava/lang/Object;Ljava/lang/Object;)Z + public static final fun getOrThrow-impl (Ljava/lang/Object;)Ljava/lang/Object; + public fun hashCode ()I + public static fun hashCode-impl (Ljava/lang/Object;)I + public static final fun isInitialized-impl (Ljava/lang/Object;)Z + public fun toString ()Ljava/lang/String; + public static fun toString-impl (Ljava/lang/Object;)Ljava/lang/String; + public final synthetic fun unbox-impl ()Ljava/lang/Object; +} + +public final class com/squareup/workflow1/NullableInitBox$Uninitialized { + public static final field INSTANCE Lcom/squareup/workflow1/NullableInitBox$Uninitialized; +} + public final class com/squareup/workflow1/PropsUpdated : com/squareup/workflow1/ActionProcessingResult { public static final field INSTANCE Lcom/squareup/workflow1/PropsUpdated; } diff --git a/workflow-core/src/commonMain/kotlin/com/squareup/workflow1/NullableInitBox.kt b/workflow-core/src/commonMain/kotlin/com/squareup/workflow1/NullableInitBox.kt new file mode 100644 index 0000000000..ec6504e564 --- /dev/null +++ b/workflow-core/src/commonMain/kotlin/com/squareup/workflow1/NullableInitBox.kt @@ -0,0 +1,28 @@ +package com.squareup.workflow1 + +import kotlin.jvm.JvmInline + +/** + * Used to wrap immutable nullable values whose holder may not yet be initialized. + * Check [isInitialized] to see if the value has been assigned. + */ +@JvmInline +public value class NullableInitBox(private val _value: Any? = Uninitialized) { + /** + * Whether or not a value has been set for this [NullableInitBox] + */ + public val isInitialized: Boolean get() = _value !== Uninitialized + + /** + * Get the value this has been initialized with. + * + * @throws [IllegalStateException] if the value in the box has not been initialized. + */ + @Suppress("UNCHECKED_CAST") + public fun getOrThrow(): T { + check(isInitialized) { "NullableInitBox was fetched before it was initialized with a value." } + return _value as T + } + + public object Uninitialized +} diff --git a/workflow-core/src/commonTest/kotlin/com/squareup/workflow1/NullableInitBoxTest.kt b/workflow-core/src/commonTest/kotlin/com/squareup/workflow1/NullableInitBoxTest.kt new file mode 100644 index 0000000000..0b18c21b72 --- /dev/null +++ b/workflow-core/src/commonTest/kotlin/com/squareup/workflow1/NullableInitBoxTest.kt @@ -0,0 +1,41 @@ +package com.squareup.workflow1 + +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertFailsWith +import kotlin.test.assertFalse +import kotlin.test.assertTrue + +class NullableInitBoxTest { + + @Test fun reports_not_initialized() { + val box = NullableInitBox() + + assertFalse(box.isInitialized) + } + + @Test fun reports_initialized() { + val box = NullableInitBox("Hello") + + assertTrue(box.isInitialized) + } + + @Test fun returns_value() { + val box = NullableInitBox("Hello") + + assertEquals("Hello", box.getOrThrow()) + } + + @Test fun throws_exceptions() { + val box = NullableInitBox() + + val exception = assertFailsWith { + box.getOrThrow() + } + + assertEquals( + "NullableInitBox was fetched before it was initialized with a value.", + exception.message + ) + } +} diff --git a/workflow-core/src/commonTest/kotlin/com/squareup/workflow1/WorkerTest.kt b/workflow-core/src/commonTest/kotlin/com/squareup/workflow1/WorkerTest.kt index 25581e0b8d..8a4cdcff22 100644 --- a/workflow-core/src/commonTest/kotlin/com/squareup/workflow1/WorkerTest.kt +++ b/workflow-core/src/commonTest/kotlin/com/squareup/workflow1/WorkerTest.kt @@ -16,7 +16,6 @@ import kotlin.test.assertNotSame import kotlin.test.assertTrue @ExperimentalCoroutinesApi -@OptIn(ExperimentalStdlibApi::class) class WorkerTest { @Test fun timer_returns_equivalent_workers_keyed() { diff --git a/workflow-core/src/commonTest/kotlin/com/squareup/workflow1/WorkflowIdentifierTest.kt b/workflow-core/src/commonTest/kotlin/com/squareup/workflow1/WorkflowIdentifierTest.kt index 8ae6aa9a93..de3855ec9c 100644 --- a/workflow-core/src/commonTest/kotlin/com/squareup/workflow1/WorkflowIdentifierTest.kt +++ b/workflow-core/src/commonTest/kotlin/com/squareup/workflow1/WorkflowIdentifierTest.kt @@ -12,7 +12,6 @@ import kotlin.test.assertFailsWith import kotlin.test.assertNotEquals import kotlin.test.assertNull -@OptIn(ExperimentalStdlibApi::class) internal class WorkflowIdentifierTest { @Test fun restored_identifier_toString() { diff --git a/workflow-runtime/api/workflow-runtime.api b/workflow-runtime/api/workflow-runtime.api index 5591211f0a..cc40aaf0dc 100644 --- a/workflow-runtime/api/workflow-runtime.api +++ b/workflow-runtime/api/workflow-runtime.api @@ -25,6 +25,7 @@ public final class com/squareup/workflow1/RenderingAndSnapshot { public final class com/squareup/workflow1/RuntimeConfigOptions : java/lang/Enum { public static final field CONFLATE_STALE_RENDERINGS Lcom/squareup/workflow1/RuntimeConfigOptions; public static final field Companion Lcom/squareup/workflow1/RuntimeConfigOptions$Companion; + public static final field PARTIAL_TREE_RENDERING Lcom/squareup/workflow1/RuntimeConfigOptions; public static final field RENDER_ONLY_WHEN_STATE_CHANGES Lcom/squareup/workflow1/RuntimeConfigOptions; public static fun getEntries ()Lkotlin/enums/EnumEntries; public static fun valueOf (Ljava/lang/String;)Lcom/squareup/workflow1/RuntimeConfigOptions; diff --git a/workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/RuntimeConfig.kt b/workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/RuntimeConfig.kt index 52c445f86b..ff5787ae40 100644 --- a/workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/RuntimeConfig.kt +++ b/workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/RuntimeConfig.kt @@ -38,6 +38,22 @@ public enum class RuntimeConfigOptions { @WorkflowExperimentalRuntime RENDER_ONLY_WHEN_STATE_CHANGES, + /** + * Only re-render each active Workflow node if: + * 1. It's own state changed, OR + * 2. One of it's descendant's state has changed. + * + * Otherwise return the cached rendering (as there is no way it could have changed). + * + * Note however that you must be careful using this because there may be external + * state that your Workflow's draw in and re-render and if that is not explicitly + * tracked within that Workflow's state, then it will not re-render. In this case, + * make sure that the state is tracked within the Workflow's state (even through + * an artificial token) in some way. + */ + @WorkflowExperimentalRuntime + PARTIAL_TREE_RENDERING, + /** * If we have more actions to process, do so before passing the rendering to the UI layer. */ diff --git a/workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/internal/WorkflowNode.kt b/workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/internal/WorkflowNode.kt index 5a2672dff2..2ad74f470b 100644 --- a/workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/internal/WorkflowNode.kt +++ b/workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/internal/WorkflowNode.kt @@ -3,14 +3,17 @@ package com.squareup.workflow1.internal import com.squareup.workflow1.ActionApplied import com.squareup.workflow1.ActionProcessingResult import com.squareup.workflow1.NoopWorkflowInterceptor +import com.squareup.workflow1.NullableInitBox import com.squareup.workflow1.RenderContext import com.squareup.workflow1.RuntimeConfig import com.squareup.workflow1.RuntimeConfigOptions +import com.squareup.workflow1.RuntimeConfigOptions.PARTIAL_TREE_RENDERING import com.squareup.workflow1.StatefulWorkflow import com.squareup.workflow1.TreeSnapshot import com.squareup.workflow1.Workflow import com.squareup.workflow1.WorkflowAction import com.squareup.workflow1.WorkflowExperimentalApi +import com.squareup.workflow1.WorkflowExperimentalRuntime import com.squareup.workflow1.WorkflowIdentifier import com.squareup.workflow1.WorkflowInterceptor import com.squareup.workflow1.WorkflowInterceptor.WorkflowSession @@ -44,7 +47,7 @@ import kotlin.coroutines.CoroutineContext * hard-coded values added to worker contexts. It must not contain a [Job] element (it would violate * structured concurrency). */ -@OptIn(WorkflowExperimentalApi::class) +@OptIn(WorkflowExperimentalApi::class, WorkflowExperimentalRuntime::class) internal class WorkflowNode( val id: WorkflowNodeId, workflow: StatefulWorkflow, @@ -86,9 +89,11 @@ internal class WorkflowNode( ) private val sideEffects = ActiveStagingList() private var lastProps: PropsT = initialProps + private var lastRendering: NullableInitBox = NullableInitBox() private val eventActionsChannel = Channel>(capacity = UNLIMITED) private var state: StateT + private var subtreeStateDidChange: Boolean = true private val baseRenderContext = RealRenderContext( renderer = subtreeManager, @@ -135,7 +140,7 @@ internal class WorkflowNode( fun snapshot(workflow: StatefulWorkflow<*, *, *, *>): TreeSnapshot { @Suppress("UNCHECKED_CAST") val typedWorkflow = workflow as StatefulWorkflow - updateCachedWorkflowInstance(typedWorkflow) + maybeUpdateCachedWorkflowInstance(typedWorkflow) return interceptor.onSnapshotStateWithChildren({ val childSnapshots = subtreeManager.createChildSnapshots() val rootSnapshot = interceptedWorkflowInstance.snapshotState(state) @@ -209,7 +214,7 @@ internal class WorkflowNode( * Call this after we have been passed any workflow instance, in [render] or [snapshot]. It may * have changed and we should check to see if we need to update our cached instances. */ - private fun updateCachedWorkflowInstance( + private fun maybeUpdateCachedWorkflowInstance( workflow: StatefulWorkflow ) { if (workflow !== cachedWorkflowInstance) { @@ -227,31 +232,51 @@ internal class WorkflowNode( workflow: StatefulWorkflow, props: PropsT ): RenderingT { - updateCachedWorkflowInstance(workflow) - updatePropsAndState(props) + updatePropsAndState(props, workflow) - baseRenderContext.unfreeze() - val rendering = interceptedWorkflowInstance.render(props, state, context) - baseRenderContext.freeze() + if (!runtimeConfig.contains(PARTIAL_TREE_RENDERING) || + !lastRendering.isInitialized || + subtreeStateDidChange + ) { + // If we haven't already updated the cached instance, better do it now! + maybeUpdateCachedWorkflowInstance(workflow) - workflowTracer.trace("UpdateRuntimeTree") { - // Tear down workflows and workers that are obsolete. - subtreeManager.commitRenderedChildren() - // Side effect jobs are launched lazily, since they can send actions to the sink, and can only - // be started after context is frozen. - sideEffects.forEachStaging { it.job.start() } - sideEffects.commitStaging { it.job.cancel() } + baseRenderContext.unfreeze() + lastRendering = NullableInitBox(interceptedWorkflowInstance.render(props, state, context)) + baseRenderContext.freeze() + + workflowTracer.trace("UpdateRuntimeTree") { + // Tear down workflows and workers that are obsolete. + subtreeManager.commitRenderedChildren() + // Side effect jobs are launched lazily, since they can send actions to the sink, and can only + // be started after context is frozen. + sideEffects.forEachStaging { it.job.start() } + sideEffects.commitStaging { it.job.cancel() } + } + // After we have rendered this subtree, we need another action in order for us to be + // considered dirty again. + subtreeStateDidChange = false } - return rendering + return lastRendering.getOrThrow() } + /** + * Update props if they have changed. If that happens, then check to see if we need + * to update the cached workflow instance, then call [StatefulWorkflow.onPropsChanged] and + * update the state from that. We consider any change to props as [subtreeStateDidChange] because + * the props themselves are used in [StatefulWorkflow.render] (they are the 'external' part of + * the state) so we must re-render. + */ private fun updatePropsAndState( - newProps: PropsT + newProps: PropsT, + workflow: StatefulWorkflow, ) { if (newProps != lastProps) { + maybeUpdateCachedWorkflowInstance(workflow) val newState = interceptedWorkflowInstance.onPropsChanged(lastProps, newProps, state) state = newState + subtreeStateDidChange = true } lastProps = newProps } @@ -260,6 +285,7 @@ internal class WorkflowNode( * Applies [action] to this workflow's [state] and then passes the resulting [ActionApplied] * via [emitAppliedActionToParent] to the parent, with additional information as to whether or * not this action has changed the current node's state. + * */ private fun applyAction( action: WorkflowAction, @@ -272,7 +298,13 @@ internal class WorkflowNode( // Changing state is sticky, we pass it up if it ever changed. stateChanged = actionApplied.stateChanged || (childResult?.stateChanged ?: false) ) - return if (actionApplied.output != null) { + // Our state changed or one of our children's state changed. + subtreeStateDidChange = aggregateActionApplied.stateChanged + return if (actionApplied.output != null || + runtimeConfig.contains(PARTIAL_TREE_RENDERING) + ) { + // If we are using the optimization, always return to the parent, so we carry a path that + // notes that the subtree did change all the way to the root. emitAppliedActionToParent(aggregateActionApplied) } else { aggregateActionApplied diff --git a/workflow-runtime/src/commonTest/kotlin/com/squareup/workflow1/RenderWorkflowInTest.kt b/workflow-runtime/src/commonTest/kotlin/com/squareup/workflow1/RenderWorkflowInTest.kt index 50f48e7c4d..4a1df64f64 100644 --- a/workflow-runtime/src/commonTest/kotlin/com/squareup/workflow1/RenderWorkflowInTest.kt +++ b/workflow-runtime/src/commonTest/kotlin/com/squareup/workflow1/RenderWorkflowInTest.kt @@ -1,6 +1,7 @@ package com.squareup.workflow1 import com.squareup.workflow1.RuntimeConfigOptions.CONFLATE_STALE_RENDERINGS +import com.squareup.workflow1.RuntimeConfigOptions.PARTIAL_TREE_RENDERING import com.squareup.workflow1.RuntimeConfigOptions.RENDER_ONLY_WHEN_STATE_CHANGES import com.squareup.workflow1.internal.ParameterizedTestRunner import kotlinx.coroutines.CancellationException @@ -50,7 +51,9 @@ class RenderWorkflowInTest { RuntimeConfigOptions.RENDER_PER_ACTION, setOf(RENDER_ONLY_WHEN_STATE_CHANGES), setOf(CONFLATE_STALE_RENDERINGS), - setOf(CONFLATE_STALE_RENDERINGS, RENDER_ONLY_WHEN_STATE_CHANGES) + setOf(CONFLATE_STALE_RENDERINGS, RENDER_ONLY_WHEN_STATE_CHANGES), + setOf(RENDER_ONLY_WHEN_STATE_CHANGES, PARTIAL_TREE_RENDERING), + setOf(CONFLATE_STALE_RENDERINGS, RENDER_ONLY_WHEN_STATE_CHANGES, PARTIAL_TREE_RENDERING), ) private val tracerOptions = setOf( @@ -1182,6 +1185,146 @@ class RenderWorkflowInTest { } } + @Test + fun `for_partial_tree_rendering_we_do_not_render_nodes_if_state_not_changed_even_in_render_pass`() { + runtimeTestRunner.runParametrizedTest( + paramSource = runtimeOptions.filter { + it.first.contains(PARTIAL_TREE_RENDERING) + }, + before = ::setup, + ) { (runtimeConfig: RuntimeConfig, workflowTracer: WorkflowTracer?) -> + runTest(UnconfinedTestDispatcher()) { + check(runtimeConfig.contains(PARTIAL_TREE_RENDERING)) + + val trigger = MutableSharedFlow() + var childRenderCount = 0 + var parentRenderCount = 0 + + val childWorkflow = Workflow.stateful( + // Starts with "state 1" + initialState = "state 1", + render = { renderState -> + runningWorker( + trigger.asWorker() + ) { + action("") { + state = it // Change state, but no output for parent. + } + } + renderState.also { + childRenderCount++ + } + } + ) + + val workflow = Workflow.stateful( + initialState = "state 0", + render = { renderState -> + renderChild(childWorkflow) { childOutput -> + action("childHandler") { + state = childOutput + } + } + runningWorker( + trigger.asWorker() + ) { + action("") { + state = it + } + } + renderState.also { + parentRenderCount++ + } + } + ) + val props = MutableStateFlow(Unit) + renderWorkflowIn( + workflow = workflow, + scope = backgroundScope, + props = props, + runtimeConfig = runtimeConfig, + workflowTracer = workflowTracer, + ) {} + + trigger.emit("state 1") // same value as the child starts with. + advanceUntilIdle() + + assertEquals(2, parentRenderCount) + assertEquals(1, childRenderCount) + } + } + } + + @Test fun for_partial_tree_rendering_we_render_nodes_if_state_changed() { + runtimeTestRunner.runParametrizedTest( + paramSource = runtimeOptions.filter { + it.first.contains(PARTIAL_TREE_RENDERING) + }, + before = ::setup, + ) { (runtimeConfig: RuntimeConfig, workflowTracer: WorkflowTracer?) -> + runTest(UnconfinedTestDispatcher()) { + check(runtimeConfig.contains(PARTIAL_TREE_RENDERING)) + + val trigger = MutableSharedFlow() + var childRenderCount = 0 + var parentRenderCount = 0 + + val childWorkflow = Workflow.stateful( + // Starts with "state 0" + initialState = "state 0", + render = { renderState -> + runningWorker( + trigger.asWorker() + ) { + action("") { + state = it // Change state, but no output for parent. + } + } + renderState.also { + childRenderCount++ + } + } + ) + + val workflow = Workflow.stateful( + initialState = "state 0", + render = { renderState -> + renderChild(childWorkflow) { childOutput -> + action("childHandler") { + state = childOutput + } + } + runningWorker( + trigger.asWorker() + ) { + action("") { + state = it + } + } + renderState.also { + parentRenderCount++ + } + } + ) + val props = MutableStateFlow(Unit) + renderWorkflowIn( + workflow = workflow, + scope = backgroundScope, + props = props, + runtimeConfig = runtimeConfig, + workflowTracer = workflowTracer, + ) {} + + trigger.emit("state 1") // different value than the child starts with. + advanceUntilIdle() + + assertEquals(3, parentRenderCount) + // Parent needs to be rendered 3x, but child only 2x as the 3rd time its the same. + assertEquals(2, childRenderCount) + } + } + } + @Test fun for_render_on_change_only_and_conflate_we_drain_action_but_do_not_render_no_state_changed() { runtimeTestRunner.runParametrizedTest( diff --git a/workflow-runtime/src/commonTest/kotlin/com/squareup/workflow1/WorkflowOperatorsTest.kt b/workflow-runtime/src/commonTest/kotlin/com/squareup/workflow1/WorkflowOperatorsTest.kt index 735b966f3a..1bc5bc0c2f 100644 --- a/workflow-runtime/src/commonTest/kotlin/com/squareup/workflow1/WorkflowOperatorsTest.kt +++ b/workflow-runtime/src/commonTest/kotlin/com/squareup/workflow1/WorkflowOperatorsTest.kt @@ -207,23 +207,37 @@ class WorkflowOperatorsTest { private abstract class StateFlowWorkflow( val name: String, val flow: StateFlow - ) : StatelessWorkflow() { + ) : StatefulWorkflow() { var starts: Int = 0 private set + override fun initialState( + props: Unit, + snapshot: Snapshot? + ): T { + return flow.value + } + private val rerenderWorker = object : Worker { override fun run(): Flow = flow.onStart { starts++ } } override fun render( renderProps: Unit, + renderState: T, context: RenderContext ): T { // Listen to the flow to trigger a re-render when it updates. - context.runningWorker(rerenderWorker as Worker) { WorkflowAction.noAction() } - return flow.value + context.runningWorker(rerenderWorker) { output: T -> + action("rerenderUpdate") { + state = output + } + } + return renderState } + override fun snapshotState(state: T): Snapshot? = null + override fun toString(): String = "StateFlowWorkflow($name)" } } diff --git a/workflow-runtime/src/commonTest/kotlin/com/squareup/workflow1/internal/WorkflowRunnerTest.kt b/workflow-runtime/src/commonTest/kotlin/com/squareup/workflow1/internal/WorkflowRunnerTest.kt index 7efab5a762..18e657ce44 100644 --- a/workflow-runtime/src/commonTest/kotlin/com/squareup/workflow1/internal/WorkflowRunnerTest.kt +++ b/workflow-runtime/src/commonTest/kotlin/com/squareup/workflow1/internal/WorkflowRunnerTest.kt @@ -5,6 +5,7 @@ import com.squareup.workflow1.NoopWorkflowInterceptor import com.squareup.workflow1.RuntimeConfig import com.squareup.workflow1.RuntimeConfigOptions import com.squareup.workflow1.RuntimeConfigOptions.CONFLATE_STALE_RENDERINGS +import com.squareup.workflow1.RuntimeConfigOptions.PARTIAL_TREE_RENDERING import com.squareup.workflow1.RuntimeConfigOptions.RENDER_ONLY_WHEN_STATE_CHANGES import com.squareup.workflow1.Worker import com.squareup.workflow1.Workflow @@ -34,7 +35,9 @@ internal class WorkflowRunnerTest { RuntimeConfigOptions.RENDER_PER_ACTION, setOf(RENDER_ONLY_WHEN_STATE_CHANGES), setOf(CONFLATE_STALE_RENDERINGS), - setOf(CONFLATE_STALE_RENDERINGS, RENDER_ONLY_WHEN_STATE_CHANGES) + setOf(CONFLATE_STALE_RENDERINGS, RENDER_ONLY_WHEN_STATE_CHANGES), + setOf(RENDER_ONLY_WHEN_STATE_CHANGES, PARTIAL_TREE_RENDERING), + setOf(CONFLATE_STALE_RENDERINGS, RENDER_ONLY_WHEN_STATE_CHANGES, PARTIAL_TREE_RENDERING), ).asSequence() private fun setup() { diff --git a/workflow-testing/src/test/java/com/squareup/workflow1/WorkflowsLifecycleTests.kt b/workflow-testing/src/test/java/com/squareup/workflow1/WorkflowsLifecycleTests.kt index 15112aedf2..51a1b648bc 100644 --- a/workflow-testing/src/test/java/com/squareup/workflow1/WorkflowsLifecycleTests.kt +++ b/workflow-testing/src/test/java/com/squareup/workflow1/WorkflowsLifecycleTests.kt @@ -1,6 +1,7 @@ package com.squareup.workflow1 import com.squareup.workflow1.RuntimeConfigOptions.CONFLATE_STALE_RENDERINGS +import com.squareup.workflow1.RuntimeConfigOptions.PARTIAL_TREE_RENDERING import com.squareup.workflow1.RuntimeConfigOptions.RENDER_ONLY_WHEN_STATE_CHANGES import com.squareup.workflow1.testing.headlessIntegrationTest import kotlinx.coroutines.Job @@ -19,7 +20,9 @@ class WorkflowsLifecycleTests { RuntimeConfigOptions.RENDER_PER_ACTION, setOf(RENDER_ONLY_WHEN_STATE_CHANGES), setOf(CONFLATE_STALE_RENDERINGS), - setOf(CONFLATE_STALE_RENDERINGS, RENDER_ONLY_WHEN_STATE_CHANGES) + setOf(CONFLATE_STALE_RENDERINGS, RENDER_ONLY_WHEN_STATE_CHANGES), + setOf(RENDER_ONLY_WHEN_STATE_CHANGES, PARTIAL_TREE_RENDERING), + setOf(CONFLATE_STALE_RENDERINGS, RENDER_ONLY_WHEN_STATE_CHANGES, PARTIAL_TREE_RENDERING), ).asSequence() private val runtimeTestRunner = ParameterizedTestRunner() From c14c54d0297b691659dfade24bf6b7b1f7e94c91 Mon Sep 17 00:00:00 2001 From: Stephen Edwards Date: Thu, 6 Mar 2025 09:57:05 -0500 Subject: [PATCH 2/4] Update workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/RuntimeConfig.kt Co-authored-by: Ray Ryan --- .../commonMain/kotlin/com/squareup/workflow1/RuntimeConfig.kt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/RuntimeConfig.kt b/workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/RuntimeConfig.kt index ff5787ae40..f16335a2bf 100644 --- a/workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/RuntimeConfig.kt +++ b/workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/RuntimeConfig.kt @@ -40,8 +40,8 @@ public enum class RuntimeConfigOptions { /** * Only re-render each active Workflow node if: - * 1. It's own state changed, OR - * 2. One of it's descendant's state has changed. + * 1. Its own state changed, OR + * 2. One of its descendant's state has changed. * * Otherwise return the cached rendering (as there is no way it could have changed). * From 8e5d20a93c086920a3b6683c1589b6799546ff63 Mon Sep 17 00:00:00 2001 From: Stephen Edwards Date: Thu, 6 Mar 2025 09:57:13 -0500 Subject: [PATCH 3/4] Update workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/RuntimeConfig.kt Co-authored-by: Ray Ryan --- .../kotlin/com/squareup/workflow1/RuntimeConfig.kt | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/RuntimeConfig.kt b/workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/RuntimeConfig.kt index f16335a2bf..5bbc237b19 100644 --- a/workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/RuntimeConfig.kt +++ b/workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/RuntimeConfig.kt @@ -46,10 +46,10 @@ public enum class RuntimeConfigOptions { * Otherwise return the cached rendering (as there is no way it could have changed). * * Note however that you must be careful using this because there may be external - * state that your Workflow's draw in and re-render and if that is not explicitly - * tracked within that Workflow's state, then it will not re-render. In this case, - * make sure that the state is tracked within the Workflow's state (even through - * an artificial token) in some way. + * state that your Workflow draws in and re-renders, and if that is not explicitly + * tracked within that Workflow's state then the Workflow will not re-render. + * In this case make sure that the implicit state is tracked within the Workflow's + * `StateT` in some way, even if only via a hash token. */ @WorkflowExperimentalRuntime PARTIAL_TREE_RENDERING, From 7d3a6fe552c87524036fd48c6f20cdd3fac8de57 Mon Sep 17 00:00:00 2001 From: Stephen Edwards Date: Thu, 6 Mar 2025 09:57:53 -0500 Subject: [PATCH 4/4] Update workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/internal/WorkflowNode.kt Co-authored-by: Ray Ryan --- .../com/squareup/workflow1/internal/WorkflowNode.kt | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/internal/WorkflowNode.kt b/workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/internal/WorkflowNode.kt index 2ad74f470b..ea4b25299a 100644 --- a/workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/internal/WorkflowNode.kt +++ b/workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/internal/WorkflowNode.kt @@ -305,6 +305,14 @@ internal class WorkflowNode( ) { // If we are using the optimization, always return to the parent, so we carry a path that // notes that the subtree did change all the way to the root. + // + // We don't need that without the optimization because there is nothing + // to output from the root of the runtime -- the output has propagated + // as far as it needs to causing all corresponding state changes. + // + // However, the root and the path down to the changed nodes must always + // re-render now, so this is the implementation detail of how we get + // subtreeStateDidChange = true on that entire path to the root. emitAppliedActionToParent(aggregateActionApplied) } else { aggregateActionApplied