Skip to content

Use WorkStealingDispatcher in runtime, behind a flag. #1365

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 2 commits into
base: zachklipp/dispatcher
Choose a base branch
from
Open

Use WorkStealingDispatcher in runtime, behind a flag. #1365

wants to merge 2 commits into from

Conversation

zach-klippenstein
Copy link
Collaborator

@zach-klippenstein zach-klippenstein commented Jul 9, 2025 •
edited
Loading

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

@rjrjr
Copy link
Contributor

rjrjr commented Jul 9, 2025

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

@zach-klippenstein zach-klippenstein force-pushed the zachklipp/dispatcher-runtime branch from 65a8c7f to c150bc0 Compare July 9, 2025 00:49
@zach-klippenstein zach-klippenstein marked this pull request as ready for review July 9, 2025 00:52
@zach-klippenstein zach-klippenstein requested a review from a team as a code owner July 9, 2025 00:52
@workflow-pr-fixer workflow-pr-fixer bot requested a review from a team as a code owner July 9, 2025 00:53
@zach-klippenstein zach-klippenstein force-pushed the zachklipp/dispatcher-runtime branch from 9c0f379 to 26aab12 Compare July 9, 2025 01:00
@zach-klippenstein zach-klippenstein force-pushed the zachklipp/dispatcher-runtime branch from a2fe965 to ab23b7b Compare July 9, 2025 18:00
@zach-klippenstein zach-klippenstein force-pushed the zachklipp/dispatcher branch 4 times, most recently from e06b394 to 20e7b6b Compare July 9, 2025 18:59
@zach-klippenstein zach-klippenstein force-pushed the zachklipp/dispatcher-runtime branch from f6e81e9 to 468cef1 Compare July 9, 2025 19:40
@zach-klippenstein
Copy link
Collaborator Author

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 zach-klippenstein marked this pull request as draft July 9, 2025 19:55
@zach-klippenstein
Copy link
Collaborator Author

zach-klippenstein commented Jul 9, 2025 •
edited
Loading

Changed to draft while I write tests.

@zach-klippenstein zach-klippenstein force-pushed the zachklipp/dispatcher-runtime branch from c652b31 to 2547ad1 Compare July 10, 2025 00:29
@zach-klippenstein zach-klippenstein force-pushed the zachklipp/dispatcher-runtime branch from 2547ad1 to 68c7b69 Compare July 10, 2025 00:33
@zach-klippenstein zach-klippenstein mentioned this pull request Jul 10, 2025
Open
zach-klippenstein
zach-klippenstein commented Jul 10, 2025
View reviewed changes
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())
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

zach-klippenstein
zach-klippenstein commented Jul 10, 2025
View reviewed changes
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
Copy link
Collaborator Author

@zach-klippenstein zach-klippenstein Jul 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This tested behavior intentionally changes with WSD on.

@zach-klippenstein
Copy link
Collaborator Author

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.

@zach-klippenstein zach-klippenstein marked this pull request as ready for review July 10, 2025 00:42
steve-the-edwards
steve-the-edwards approved these changes Jul 10, 2025
View reviewed changes
Copy link
Contributor

@steve-the-edwards steve-the-edwards left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 zach-klippenstein force-pushed the zachklipp/dispatcher branch 3 times, most recently from c660617 to 446b1d8 Compare July 11, 2025 19:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@steve-the-edwards steve-the-edwards steve-the-edwards approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@zach-klippenstein @rjrjr @steve-the-edwards