Use WorkStealingDispatcher in runtime, behind a flag. #1365

zach-klippenstein · 2025-07-09T00:08:41Z

Installs the WorkStealingDispatcher introduced in #1364 into the workflow runtime behind a new runtime flag.

rjrjr · 2025-07-09T00:33:05Z

So if this works, use it instead of adding the new Compose dependency?

zach-klippenstein · 2025-07-09T19:47:23Z

So if this works, use it instead of adding the new Compose dependency?

I would still like to get away from Unconfined if there's an opportunity to do so, but if this lets us move faster without shaving that yak, then awesome.

zach-klippenstein · 2025-07-09T19:55:49Z

Changed to draft while I write tests.

zach-klippenstein · 2025-07-10T00:36:57Z

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

@@ -161,6 +169,8 @@ public enum class RuntimeConfigOptions {
      //     DRAIN_EXCLUSIVE_ACTIONS,
      //   )
      // ),
+
+      ALL(RuntimeConfigOptions.entries.toSet())


It seems to me like it would be a good idea to have a config that turns on all the flags (for testing, but also for green-field uses of the runtime), but if you'd rather I add explicit configs for this new flag I can do that too.

Added a comment indicating the difference in intention between this and other enum entries that might happen to also contain all entries when they're introduced.

zach-klippenstein · 2025-07-10T00:37:31Z

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

@@ -1494,7 +1501,9 @@ class RenderWorkflowInTest(

  @Test
  fun for_conflate_we_do_not_conflate_stacked_actions_into_one_rendering_if_output() {
-    if (runtimeConfig.contains(CONFLATE_STALE_RENDERINGS)) {
+    if (CONFLATE_STALE_RENDERINGS in runtimeConfig &&
+      WORK_STEALING_DISPATCHER !in runtimeConfig


This tested behavior intentionally changes with WSD on.

zach-klippenstein · 2025-07-10T00:42:36Z

I added a single test for the new ordering behavior. I don't know if we need more than that since WorkStealingDispatcher itself is tested extensively in isolation. Happy to add more tests if anyone can think up any other interesting edge cases though.

steve-the-edwards

I realize the latest version of it is not merged yet - but where I would really like to test this is with the tests in AndroidDispatchersRenderWorkflowInTest workflow-runtime-android/src/androidTest/java/com/squareup/workflow1/android/AndroidDispatchersRenderWorkflowInTest.kt

Currently in the branch from this PR - #1355

We could merge your WSD and then I could rebase and test it with that I guess?

zach-klippenstein · 2025-07-14T14:49:37Z

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

+          // We advance the dispatcher first to allow any coroutines that were launched by the last
+          // render pass to start up and potentially enqueue actions.
+          dispatcher?.let {
+            workflowTracer.trace("AdvancingWorkflowDispatcher") {


@steve-the-edwards I just added this. I figure since this is intended to improve performance, might as well add a trace section to measure the new work preemptively.

zach-klippenstein · 2025-07-14T19:56:03Z

Ownership ended with @zach-klippenstein, @steve-the-edwards is this PR's owner now.

# Conflicts: # workflow-core/src/commonMain/kotlin/com/squareup/workflow1/RuntimeConfig.kt # workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/RenderWorkflow.kt # workflow-runtime/src/commonTest/kotlin/com/squareup/workflow1/RenderWorkflowInTest.kt

steve-the-edwards · 2025-07-23T14:23:49Z

.github/workflows/kotlin.yml

@@ -239,7 +239,7 @@ jobs:
        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'
+          report_paths: '**/build/test-results/jvmTest/TEST-*.xml'


This was just a bug with the reporting paths that I found while looking at this.

What is report_paths?

what files it tries to pick up test results from to show in the PR directly. (this was just not working before).

steve-the-edwards · 2025-07-23T14:24:46Z

.github/workflows/kotlin.yml

+      - name: Check with Gradle
+        uses: ./.github/actions/gradle-task
+        with:
+          task: jvmTest --continue -Pworkflow.runtime=all


I don't test the work stealing dispatcher combinations separately, just adding the 'all' config here for the jvm tests.

Which should be fine b/c we plan to ship that way?

'eventually', yes

but good point that we should make sure each combination we roll out with has been tested here.

steve-the-edwards · 2025-07-23T14:25:02Z

.github/workflows/kotlin.yml

@@ -578,7 +599,7 @@ jobs:
        ### <start-connected-check-shards>
        shardNum: [ 1, 2, 3 ]
        ### <end-connected-check-shards>
-        runtime: [ conflate, stateChange, drainExclusive, conflate-stateChange, partial, conflate-partial, stable, conflate-drainExclusive, stateChange-drainExclusive, partial-drainExclusive, conflate-partial-drainExclusive ]
+        runtime: [ conflate, stateChange, drainExclusive, conflate-stateChange, partial, conflate-partial, stable, conflate-drainExclusive, stateChange-drainExclusive, partial-drainExclusive, conflate-partial-drainExclusive, all ]


Add 'all' the runtime test shard.

steve-the-edwards · 2025-07-23T14:25:37Z

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

+   * Note that this is *not effective* when using an immediate or unconfined dispatcher as the
+   * handling of an action happens synchronously after the coroutine sending it into the sink
+   * is resumed. This means it never dispatches multiple coroutines that send actions into the sink
+   * before handling the first, so there is never multiple actions to apply!


Added notes here about how CSR and DEA won't work with an immediate or unconfined dispatcher.

But WORK_STEALING_DISPATCHER fixes that, right? If so should say so. If not, what's our actual plan? Seems like we should suggest to the reader whatever it is that we're planning to do in POS.

No, it does not actually fix that. We thought it might until we realized that immediate dispatchers prevent the dispatch of the coroutines for the other actions from happening, so there is nothing to drain after processing just the first action.

The way to fix that would be to use a non immediate dispatcher - e.g. Dispatchers.Main - along with a frame clock listener that explicitly calls advanceUntilIdle() for each frame so that we can maintain the guarantee of stably updating the runtime each frame.

I implemented that and tested it here with these tests, but in discussion with @zach-klippenstein we decided that it was not helpful.

If you are using Compose, then you should use AndroidUiDispatcher.Main for this, otherwise the timing of the two frame callbacks might not be right.

If you are not using Compose, we don't really know this and we are OK with just not being able to use CSR or DEA.

I updated it to say this:

* Note further that [WORK_STEALING_DISPATCHER] does not fix this for immediate dispatchers. In * order to use this optimization, a non-immediate dispatcher must be used. If using a non- * immediate dispatcher, we recommend that you ensure that the Workflow runtime completes all * known updates before the next 'frame.' This can be done using a dispatcher that is integrated * with the frame clock on your platform. E.g., on Android we recommend using Compose UI's * `AndroidUiDispatcher.Main` in order to access these optimizations.

Makes me think we should just drop CSR then, since it's already pretty redundant with what's built into ComposeView et al.

I thought so at first too, but if the view runner work is not cancellable it does save view runner work by producing fewer renderings when they are stale.

rjrjr · 2025-07-23T14:30:32Z

.github/workflows/kotlin.yml

@@ -239,7 +239,7 @@ jobs:
        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'
+          report_paths: '**/build/test-results/jvmTest/TEST-*.xml'


What is report_paths?

rjrjr · 2025-07-23T14:31:21Z

.github/workflows/kotlin.yml

+      - name: Check with Gradle
+        uses: ./.github/actions/gradle-task
+        with:
+          task: jvmTest --continue -Pworkflow.runtime=all


Which should be fine b/c we plan to ship that way?

rjrjr · 2025-07-23T14:36:25Z

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

+   * Note that this is *not effective* when using an immediate or unconfined dispatcher as the
+   * handling of an action happens synchronously after the coroutine sending it into the sink
+   * is resumed. This means it never dispatches multiple coroutines that send actions into the sink
+   * before handling the first, so there is never multiple actions to apply!


But WORK_STEALING_DISPATCHER fixes that, right? If so should say so. If not, what's our actual plan? Seems like we should suggest to the reader whatever it is that we're planning to do in POS.

rjrjr · 2025-07-23T14:37:19Z

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

@@ -98,69 +115,349 @@ public enum class RuntimeConfigOptions {
    enum class RuntimeOptions(
      val runtimeConfig: RuntimeConfig
    ) {
-      DEFAULT(RENDER_PER_ACTION),
+      NONE(RENDER_PER_ACTION),


Holy combinatorial explosion.

zach-klippenstein force-pushed the zachklipp/dispatcher branch from 4df1469 to b71978b Compare July 9, 2025 00:45

zach-klippenstein force-pushed the zachklipp/dispatcher-runtime branch from 65a8c7f to c150bc0 Compare July 9, 2025 00:49

zach-klippenstein marked this pull request as ready for review July 9, 2025 00:52

zach-klippenstein requested a review from a team as a code owner July 9, 2025 00:52

workflow-pr-fixer bot requested a review from a team as a code owner July 9, 2025 00:53

zach-klippenstein force-pushed the zachklipp/dispatcher branch from 5adab40 to b81c18d Compare July 9, 2025 01:00

zach-klippenstein force-pushed the zachklipp/dispatcher-runtime branch from 9c0f379 to 26aab12 Compare July 9, 2025 01:00

zach-klippenstein force-pushed the zachklipp/dispatcher branch from b81c18d to 484d9c5 Compare July 9, 2025 18:00

zach-klippenstein force-pushed the zachklipp/dispatcher-runtime branch from a2fe965 to ab23b7b Compare July 9, 2025 18:00

zach-klippenstein force-pushed the zachklipp/dispatcher branch 4 times, most recently from e06b394 to 20e7b6b Compare July 9, 2025 18:59

zach-klippenstein force-pushed the zachklipp/dispatcher-runtime branch from f6e81e9 to 468cef1 Compare July 9, 2025 19:40

zach-klippenstein marked this pull request as draft July 9, 2025 19:55

zach-klippenstein force-pushed the zachklipp/dispatcher-runtime branch from c652b31 to 2547ad1 Compare July 10, 2025 00:29

zach-klippenstein force-pushed the zachklipp/dispatcher branch from 20e7b6b to e56709d Compare July 10, 2025 00:32

zach-klippenstein force-pushed the zachklipp/dispatcher-runtime branch from 2547ad1 to 68c7b69 Compare July 10, 2025 00:33

zach-klippenstein mentioned this pull request Jul 10, 2025

Introduce WorkStealingDispatcher. #1364

Merged

zach-klippenstein commented Jul 10, 2025

View reviewed changes

zach-klippenstein marked this pull request as ready for review July 10, 2025 00:42

steve-the-edwards approved these changes Jul 10, 2025

View reviewed changes

zach-klippenstein force-pushed the zachklipp/dispatcher branch 3 times, most recently from 7568824 to c660617 Compare July 11, 2025 19:03

zach-klippenstein force-pushed the zachklipp/dispatcher branch 2 times, most recently from 446b1d8 to 9cb6cfe Compare July 14, 2025 14:01

zach-klippenstein force-pushed the zachklipp/dispatcher-runtime branch from 68c7b69 to 13b4104 Compare July 14, 2025 14:47

zach-klippenstein commented Jul 14, 2025

View reviewed changes

zach-klippenstein force-pushed the zachklipp/dispatcher-runtime branch from 13b4104 to 96b655a Compare July 14, 2025 14:51

zach-klippenstein force-pushed the zachklipp/dispatcher branch from 9cb6cfe to 1be3f23 Compare July 14, 2025 15:48

zach-klippenstein force-pushed the zachklipp/dispatcher-runtime branch from 96b655a to 095c5a1 Compare July 14, 2025 15:48

zach-klippenstein force-pushed the zachklipp/dispatcher branch from 1be3f23 to cab0e63 Compare July 14, 2025 15:59

zach-klippenstein force-pushed the zachklipp/dispatcher-runtime branch from 095c5a1 to d6512f6 Compare July 14, 2025 15:59

steve-the-edwards force-pushed the zachklipp/dispatcher branch from cab0e63 to a158414 Compare July 14, 2025 17:32

Base automatically changed from zachklipp/dispatcher to main July 14, 2025 17:56

steve-the-edwards force-pushed the zachklipp/dispatcher-runtime branch 2 times, most recently from 1f49159 to 94d3855 Compare July 22, 2025 20:11

steve-the-edwards force-pushed the zachklipp/dispatcher-runtime branch from 94d3855 to 28db854 Compare July 23, 2025 14:13

steve-the-edwards reviewed Jul 23, 2025

View reviewed changes

rjrjr approved these changes Jul 23, 2025

View reviewed changes

Update WSD Use, Further Tests, Add to CI

d9bbcff

steve-the-edwards force-pushed the zachklipp/dispatcher-runtime branch from 28db854 to d9bbcff Compare July 23, 2025 15:13

steve-the-edwards merged commit 96997a2 into main Jul 23, 2025
130 of 134 checks passed

steve-the-edwards deleted the zachklipp/dispatcher-runtime branch July 23, 2025 16:58

Use WorkStealingDispatcher in runtime, behind a flag. #1365

Use WorkStealingDispatcher in runtime, behind a flag. #1365

Uh oh!

Conversation

zach-klippenstein commented Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rjrjr commented Jul 9, 2025

Uh oh!

zach-klippenstein commented Jul 9, 2025

Uh oh!

zach-klippenstein commented Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zach-klippenstein Jul 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zach-klippenstein commented Jul 10, 2025

Uh oh!

steve-the-edwards left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zach-klippenstein commented Jul 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zach-klippenstein commented Jul 9, 2025 •

edited

Loading

zach-klippenstein commented Jul 9, 2025 •

edited

Loading

zach-klippenstein Jul 10, 2025 •

edited

Loading