1247: Partial Tree Rerendering #1248

steve-the-edwards · 2025-01-20T16:10:56Z

Re-render the node only if:

The state (including props) of this node or props has changed.
The state of one of this node's descendants has changed.

In order to do this, we have to propagate the action cascade all the way up to the root to record a single path of changed nodes.

Note we currently do not re-render if the workflow instance (the behavior definition instance, not the node) changed (unless the state also changed!), assuming the new workflow is still compatible with the old rendering because its identifier did not change.

Closes #1247.

steve-the-edwards · 2025-01-24T19:50:10Z

workflow-runtime/src/commonTest/kotlin/com/squareup/workflow1/WorkflowOperatorsTest.kt

-      return flow.value
+      context.runningWorker(rerenderWorker) { output: T ->
+        action("rerenderUpdate") {
+          state = output


prior to this state was not actually changing - so this would not work in the optimized runtime.

zach-klippenstein · 2025-03-04T23:44:40Z

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

@@ -211,12 +216,14 @@ internal class WorkflowNode<PropsT, StateT, OutputT, RenderingT>(
   */
  private fun updateCachedWorkflowInstance(
    workflow: StatefulWorkflow<PropsT, StateT, OutputT, RenderingT>
-  ) {
+  ): Boolean {


Nit: Add a note to the kdoc explaining this new return value

No longer returning as we just call this relying on its idempotency.

zach-klippenstein · 2025-03-04T23:49:36Z

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

+      !lastRendering.isInitialized ||
+      subtreeStateDidChange
+    ) {
+      if (!didUpdateCachedInstance) {


Would it be simpler to just call updateCachedWorkflowInstance unconditionally here? It's idempotent, right?

good point, we check equality there, so we could call it again

I just call it again now.

zach-klippenstein · 2025-03-04T23:52:55Z

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

@@ -289,4 +322,17 @@ internal class WorkflowNode<PropsT, StateT, OutputT, RenderingT>(
      SideEffectNode(key, job)
    }
  }
+
+  @JvmInline
+  internal value class Box<T>(private val _value: Any? = Uninitialized) {


Nit: This seems like a super generic and potentially useful helper, I'd pull it out.

Extracted into workflow-core as NullableInitBox with its own tests.

zach-klippenstein · 2025-03-04T23:53:08Z

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

+
+    @Suppress("UNCHECKED_CAST")
+    fun getOrThrow(): T {
+      check(isInitialized)


Nit: Give this a descriptive error message.

zach-klippenstein · 2025-03-04T23:54:47Z

workflow-testing/src/test/java/com/squareup/workflow1/WorkflowsLifecycleTests.kt

-    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()


Not in scope but… why isn't this just sequenceOf(…)?

i'm not sure tbh. the way it was written originally is likely just a mistake?

zach-klippenstein · 2025-03-04T23:58:40Z

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

+      // 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.


Thanks for this comment, but I'm missing the middle reason why the optimization requires propagating change status all the way up but we don't need that without the optimization.

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.

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

rjrjr · 2025-03-05T22:29:27Z

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

@@ -260,6 +285,7 @@ internal class WorkflowNode<PropsT, StateT, OutputT, RenderingT>(
   * 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.
+   *


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

rjrjr · 2025-03-05T22:33:22Z

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

+ * Check [isInitialized] to see if the value has been assigned.
+ */
+@JvmInline
+public value class NullableInitBox<T>(private val _value: Any? = Uninitialized) {


I didn't take in at first that this is a value class. Cool!

That was a @zach-klippenstein special when we were noodling on this.

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.

…RuntimeConfig.kt Co-authored-by: Ray Ryan <[email protected]>

…internal/WorkflowNode.kt Co-authored-by: Ray Ryan <[email protected]>

steve-the-edwards force-pushed the sedwards/partial-tree branch from a393b93 to 0514919 Compare January 21, 2025 19:08

steve-the-edwards changed the base branch from main to sedwards/cache-render-context January 21, 2025 20:32

steve-the-edwards force-pushed the sedwards/cache-render-context branch 2 times, most recently from 6f2d156 to 74fb6fb Compare January 23, 2025 15:30

steve-the-edwards force-pushed the sedwards/partial-tree branch from 0514919 to 7c724a3 Compare January 23, 2025 15:59

steve-the-edwards closed this Jan 23, 2025

steve-the-edwards reopened this Jan 23, 2025

steve-the-edwards force-pushed the sedwards/cache-render-context branch from 74fb6fb to c7d7c15 Compare January 23, 2025 20:22

steve-the-edwards force-pushed the sedwards/partial-tree branch from 06bf903 to e1f220c Compare January 23, 2025 20:35

steve-the-edwards force-pushed the sedwards/cache-render-context branch 2 times, most recently from 0a36563 to 717d484 Compare January 24, 2025 16:24

steve-the-edwards force-pushed the sedwards/partial-tree branch 3 times, most recently from 7d7098f to b6e677c Compare January 24, 2025 19:49

steve-the-edwards commented Jan 24, 2025

View reviewed changes

steve-the-edwards force-pushed the sedwards/partial-tree branch 2 times, most recently from 8647d68 to e99c6b6 Compare January 27, 2025 14:55

steve-the-edwards force-pushed the sedwards/cache-render-context branch from 717d484 to 646ebf7 Compare January 28, 2025 15:03

Base automatically changed from sedwards/cache-render-context to main January 28, 2025 16:24

steve-the-edwards force-pushed the sedwards/partial-tree branch 5 times, most recently from 3a5f35a to f888f1d Compare January 30, 2025 16:24

steve-the-edwards force-pushed the sedwards/partial-tree branch from f888f1d to f04cb3d Compare February 12, 2025 16:08

steve-the-edwards force-pushed the sedwards/partial-tree branch 2 times, most recently from 2a074ea to b40368b Compare February 20, 2025 16:30

steve-the-edwards changed the base branch from main to sedwards/fix-conflate February 20, 2025 16:30

Base automatically changed from sedwards/fix-conflate to main February 21, 2025 21:53

steve-the-edwards force-pushed the sedwards/partial-tree branch from b40368b to a3d67b6 Compare March 4, 2025 19:40

steve-the-edwards changed the title ~~1247: WIP: Partial Tree Rerendering~~ 1247: Partial Tree Rerendering Mar 4, 2025

steve-the-edwards marked this pull request as ready for review March 4, 2025 19:43

steve-the-edwards requested review from zach-klippenstein and a team as code owners March 4, 2025 19:43

steve-the-edwards force-pushed the sedwards/partial-tree branch from 2773b21 to 00eb7a6 Compare March 4, 2025 21:25

zach-klippenstein reviewed Mar 4, 2025

View reviewed changes

steve-the-edwards force-pushed the sedwards/partial-tree branch from c1623df to 8f303c5 Compare March 5, 2025 15:16

steve-the-edwards requested a review from zach-klippenstein March 5, 2025 15:17

rjrjr approved these changes Mar 5, 2025

View reviewed changes

rjrjr reviewed Mar 5, 2025

View reviewed changes

steve-the-edwards and others added 4 commits March 6, 2025 10:18

Update workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/…

c14c54d

…RuntimeConfig.kt Co-authored-by: Ray Ryan <[email protected]>

Update workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/…

8e5d20a

…RuntimeConfig.kt Co-authored-by: Ray Ryan <[email protected]>

Update workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/…

7d3a6fe

…internal/WorkflowNode.kt Co-authored-by: Ray Ryan <[email protected]>

steve-the-edwards force-pushed the sedwards/partial-tree branch from 6384fa7 to 7d3a6fe Compare March 6, 2025 15:18

steve-the-edwards merged commit 073d468 into main Mar 6, 2025
39 checks passed

steve-the-edwards deleted the sedwards/partial-tree branch March 6, 2025 19:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1247: Partial Tree Rerendering #1248

1247: Partial Tree Rerendering #1248

steve-the-edwards commented Jan 20, 2025 •

edited

Loading

steve-the-edwards Jan 24, 2025

zach-klippenstein Mar 4, 2025

steve-the-edwards Mar 5, 2025

zach-klippenstein Mar 4, 2025

steve-the-edwards Mar 5, 2025

steve-the-edwards Mar 5, 2025

zach-klippenstein Mar 4, 2025

steve-the-edwards Mar 5, 2025

zach-klippenstein Mar 4, 2025

zach-klippenstein Mar 4, 2025

steve-the-edwards Mar 5, 2025

zach-klippenstein Mar 4, 2025

steve-the-edwards Mar 5, 2025

rjrjr Mar 5, 2025

rjrjr Mar 5, 2025

steve-the-edwards Mar 6, 2025

		// 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.

1247: Partial Tree Rerendering #1248

1247: Partial Tree Rerendering #1248

Conversation

steve-the-edwards commented Jan 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steve-the-edwards commented Jan 20, 2025 •

edited

Loading