Skip to content

Conversation

@steve-the-edwards
Copy link
Contributor

@steve-the-edwards steve-the-edwards commented Jun 19, 2025

Note this also includes some adjustments to the runtime loop. After the first suspension to wait and apply an action, with CONFLATE_STALE_RENDERINGS we loop without suspending - applying any actions that are already queued up on the action queues.

If no dispatcher is specified on Android, use Compose's AndroidUiDispatcher.Main for the runtime (requiring Compose).

As it turns out, Compose's AndroidUiDispatcher.Main and its greedy behaviour to drain all dispatches before the next Android Choreographer frame gives us exactly the performant behaviour we want.

In fact, when dispatching with AndroidUiDispatcher.Main all the coroutines that would be resumed to apply an action will be greedily run before the next Choreographer frame. This ticks the "full" runtime loop for each one, so the inner loop for CONFLATE_STALE_RENDERINGS is not really necessary. The greediness means that the updates all happen before the rendering can be consumed by view code (as well as all happen before the next frame). Because this is not an immediate dispatcher, when we pass a new rendering to the stateflow to be consumed by the view code, it is not immediately picked up (it has to be dispatched, even though the AndroidUiDispatcher.Main is going to do that greedily and before the next frame). This changes ordering.

We keep the inner loop for CONFLATE_STALE_RENDERINGS however, as it is still useful for other dispatchers.

Still the code used to drain immediately available actions can be used by DRAIN_EXCLUSIVE_ACTIONS to avoid render passes.

Fixes #1311

This should also take care of #1257 if we start using the Compose AndroidUiDispatcher.

@CLAassistant
Copy link

CLAassistant commented Jun 19, 2025

CLA assistant check
All committers have signed the CLA.

@zach-klippenstein
Copy link
Collaborator

In fact, CONFLATE_STALE_RENDERINGS is basically a no-op when using the AndroidUiDispatcher.Main to dispatch the runtime and consume the renderings.

On Android context where we use this dispatcher, we can likely just not use CONFLATE_STALE_RENDERINGS at all. however, for other dispatchers, it could still have an effect.

Is it basically a no-op, or actually a no-op? Because if it's not identical, we shouldn't treat it as optional. We already have so many confusing workflow flags, if we start requiring various configurations with caveats about things like dispatchers, the messiness just explodes.

Copy link
Collaborator

@zach-klippenstein zach-klippenstein left a comment

Choose a reason for hiding this comment

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

Ok so after reading, I'm still not sure I fully understand what the significant behavior difference is between using Compose's main dispatcher and Main.immediate.

Numbering these assertions/questions so you can respond more easily.

  1. When the conflation flag is off and multiple actions are available immediately – then with Compose's dispatcher they may result in multiple emissions to the UI layer with redundant updates, but all will still happen before the frame is drawn. Main.immediate will process them all immediately but also emit to the UI immediately, so effectively the same as far as the UI is concerned. But that was existing behavior.
  2. When no actions are available, but another action is sent before the next Choreographer frame, then the action is still be guaranteed to be processed before the frame is drawn (either by Main.immediate's immediacy or by compose's pre-frame drain). Also existing behavior.
  3. When the flag is on, then we avoid suspending when multiple actions are available, which is theoretically a performance win internally since not suspending is faster than suspending, even with Main.immediate?
    1. Externally, the behavior is the same (only emit last conflated rendering to UI) even with Main.immediate because we already weren't emitting on the renderings flow until after the (previously suspending) conflation loop?
  4. If all these are true, then this change only affects behavior for multiple immediate actions when the conflation flag is off? Otherwise the we don't suspend in the conflation loop, so the choice of dispatcher doesn't matter.

workflow = workflow,
scope = backgroundScope +
Dispatchers.Main.immediate,
AndroidUiDispatcher.Main,
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this is the only dispatcher we're testing, is it now a hard requirement that apps using workflow UI on Android must use Compose? If so we should probably communicate that very clearly and update the docs too. If not, then can we test both?

Copy link
Collaborator

Choose a reason for hiding this comment

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

And furthermore, does this imply the behavior broke for other dispatchers? If not, can we just add a parameter to the parameterization that specifies the dispatcher to use, so we can continue to validate it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The general logic against both StandardTestDispatcher and UnconfinedTestDispatcher is tested in the workflow-runtime module tests themselves. This is all just for Android. Those tests are all still there. This is testing optimizations with the Android dispatchers.

AndroidUiDispatcher.Main,
props = props,
runtimeConfig = setOf(CONFLATE_STALE_RENDERINGS),
runtimeConfig = runtime.runtimeConfig,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah cool, so this change is saying that the behavior this tests validates now happens for all configs, as long as we're using that dispatcher?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes.

): StateFlow<RenderingT> {
val restoredSnap = savedStateHandle?.get<PickledTreesnapshot>(KEY)?.snapshot

// Add in Compose's AndroidUiDispatcher.Main by default if none is specified.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add this new magical behavior to the kdoc as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do!

actionResult = actionResult,
)
) {
if (shouldShortCircuitForUnchangedState(actionResult = actionResult)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Do we really need this parameter to be named here? When the value passed in has the same name, it doesn't really add any readability.

val result = subtreeManager.applyNextAvailableChildAction()

if (result == ActionsExhausted) {
return eventActionsChannel.tryReceive().getOrNull()?.let { action ->
Copy link
Collaborator

@zach-klippenstein zach-klippenstein Jun 23, 2025

Choose a reason for hiding this comment

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

Please confirm if i understand this right: If we hit this branch, it means either no children had actions to run, or some child had an action but it didn't produce an output?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, if a child had an action it would return ActionApplied even if there was no Output. This is only if there were no child actions.

/**
* Will try to apply any immediately available actions in this action queue or any of our
* children's.
*
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
*
*
* Contrast this to [onNextAction], which is used to await the next action when none is available right now.
* Both call [applyAction] when one is available.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

   * Contrast this to [registerTreeActionSelectors] which will add select clauses that will await
   * the next action. That will also end up with [applyAction] being called when the clauses is
   * selected.

* 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].
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* It is an error to call this method after calling [cancel].
* It is an error to call this method after calling [cancel].
*
* Contrast this to [applyNextAvailableAction], which is used to check for an action
* that is already available without waiting, and then _immediately_ apply it.
* Both call [applyAction] when one is available.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

   * Contrast this to [applyNextAvailableAction], which is used to check for an action
   * that is already available without waiting, and then _immediately_ apply it.
   * The select clauses added here also call [applyAction] when one of them is selected.
   */
  fun registerTreeActionSelectors(selector: SelectBuilder<ActionProcessingResult>) {

*/
@OptIn(WorkflowExperimentalRuntime::class)
suspend fun processAction(waitForAnAction: Boolean = true): ActionProcessingResult {
suspend fun waitAndProcessAction(): ActionProcessingResult {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Since you're renaming this anyway, "await" is the more idiomatic term for "listen while suspended". In contrast, "wait" canonically implies blocking a thread.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

great point.

ActionsExhausted
}
}
rootNode.onNextAction(this)
Copy link
Collaborator

Choose a reason for hiding this comment

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

(Follow-up to nit): Renaming this method would also make this code pretty much self-documenting and the above line comment redundant.

* none immediately available.
*/
fun applyNextAvailableAction(): ActionProcessingResult {
return rootNode.applyNextAvailableAction()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Maybe name this applyNextAvailableSubtreeAction to make it more clear when reading callsites that this actually walks the tree? It took me a few read-throughs of the subtree/WN code to remember these were actually depth-first visitors methods (this and onNextAction).

@steve-the-edwards steve-the-edwards force-pushed the sedwards/move-render-2 branch from 95bf5b8 to 90af5aa Compare July 2, 2025 20:31
@steve-the-edwards steve-the-edwards requested a review from a team as a code owner July 2, 2025 20:31
@steve-the-edwards steve-the-edwards force-pushed the sedwards/move-render-2 branch from 90af5aa to 225a91f Compare July 2, 2025 20:42
Base automatically changed from sedwards/move-render-2 to main July 2, 2025 21:04
@steve-the-edwards steve-the-edwards force-pushed the sedwards/compose-dispatcher branch 2 times, most recently from 20ca4ac to c1555f2 Compare July 3, 2025 13:47
@steve-the-edwards
Copy link
Contributor Author

In fact, when dispatching with AndroidUiDispatcher.Main all the coroutines that would be resumed to apply an action will be greedily run before the next Choreographer frame. This ticks the "full" runtime loop for each one, so the inner loop for CONFLATE_STALE_RENDERINGS is not really necessary.

I updated the PR description:

In fact, when dispatching with AndroidUiDispatcher.Main all the coroutines that would be resumed to apply an action will be greedily run before the next Choreographer frame. This ticks the "full" runtime loop for each one, so the inner loop for CONFLATE_STALE_RENDERINGS is not really necessary.

@steve-the-edwards
Copy link
Contributor Author

Numbering these assertions/questions so you can respond more easily.

  1. When the conflation flag is off and multiple actions are available immediately – then with Compose's dispatcher they may result in multiple emissions to the UI layer with redundant updates, but all will still happen before the frame is drawn. Main.immediate will process them all immediately but also emit to the UI immediately, so effectively the same as far as the UI is concerned. But that was existing behavior.

  2. When no actions are available, but another action is sent before the next Choreographer frame, then the action is still be guaranteed to be processed before the frame is drawn (either by Main.immediate's immediacy or by compose's pre-frame drain). Also existing behavior.

  3. When the flag is on, then we avoid suspending when multiple actions are available, which is theoretically a performance win internally since not suspending is faster than suspending, even with Main.immediate?

    1. Externally, the behavior is the same (only emit last conflated rendering to UI) even with Main.immediate because we already weren't emitting on the renderings flow until after the (previously suspending) conflation loop?
  4. If all these are true, then this change only affects behavior for multiple immediate actions when the conflation flag is off? Otherwise the we don't suspend in the conflation loop, so the choice of dispatcher doesn't matter.

  1. yes. we should consume on AndroidUiDispatcher.Main as well to get optimal behavior.
  2. Yes
  3. Yes
  4. No. when CONFLATE_STALE_RENDERINGS is on for an immediate dispatcher, then the view code responding to a new rendering emission would still be run for each action because there is no way to stop the dispatcher from continuously running that code.

In other words:

  1. With an immediate dispatcher, there is no way to CONFLATE_STALE_RENDERINGS
  2. With Compose's AndroidUiDispatcher.Main we get the "all updates before Choreographer frame" behaviour and we get conflation due to the ordering of execution of the coroutines (actions before the view code) but it's still slightly more optimized when CONFLATE_STALE_RENDERINGS does it without suspending.
  3. With other non-immediate dispatchers, CONFLATE_STALE_RENDERINGS will work but its efficacy depends on the dispatcher behaviour and how many actions can get queued.

@rjrjr
Copy link
Collaborator

rjrjr commented Jul 3, 2025

If Compose's AndroidUiDispatcher.Main is not being used on Android, use it for the runtime whenever on Android (requiring Compose).

Misleading, makes it sound like we will clobber the dispatcher they provide when what you really mean is we'll use it if no dispatcher is provided.

// And otherwise there are *5* attempts to produce a new rendering.
val expected = if (runtime.runtimeConfig.contains(CONFLATE_STALE_RENDERINGS)) 2 else 5
assertEquals(
expected = if (runtime.runtimeConfig.contains(CONFLATE_STALE_RENDERINGS)) 2 else 5,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
expected = if (runtime.runtimeConfig.contains(CONFLATE_STALE_RENDERINGS)) 2 else 5,
expected = expected,

assertEquals(
expected = if (runtime.runtimeConfig.contains(CONFLATE_STALE_RENDERINGS)) 2 else 5,
actual = renderingsProduced,
message = "Expected $expected renderings to be produced (passed signal to interceptor)."
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
message = "Expected $expected renderings to be produced (passed signal to interceptor)."
message = "Expected $expected renderings to be produced with config ${runtime.runtimeConfig} (passed signal to interceptor)."

Comment on lines 283 to 287
val expected = if (runtime.runtimeConfig.contains(CONFLATE_STALE_RENDERINGS)) 2 else 3
assertEquals(
2,
renderingsPassed,
"Expected only 2 renderings passed to interceptor when conflating actions."
expected = if (runtime.runtimeConfig.contains(CONFLATE_STALE_RENDERINGS)) 2 else 3,
actual = renderingsProduced,
message = "Expected $expected renderings to be produced (passed signal to interceptor)."
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same changes as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. made all these changes manually


// 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.
// After resuming from runner.waitAndProcessAction() our coroutine could now be cancelled,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think there is any such method. Did you mean awaitAndApplyAction()?

// Make sure the runtime has not been cancelled from runner.processAction()
if (!isActive) return@launch

// Render pass for the updated state from the action applied.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like you're confident that applyNextAvailabletreeAction() cannot make us inactive, how come?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because it does not suspend so there is no way for the scope to be cancelled.

…ntime

If on Android there is *no* dispatcher specified, then use it for the runtime whenever on Android (requiring Compose)
@steve-the-edwards steve-the-edwards force-pushed the sedwards/compose-dispatcher branch from c1555f2 to 5f14c66 Compare July 3, 2025 19:18
@steve-the-edwards steve-the-edwards merged commit 4ee6901 into main Jul 3, 2025
43 checks passed
@steve-the-edwards steve-the-edwards deleted the sedwards/compose-dispatcher branch July 3, 2025 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

On Android, process all updates from actions from an event before the next frame (Choreographer.doFrame)

5 participants