Skip to content
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.

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

RA-64264 Make WorkflowLayout sample based on the frame clock #1257

Open
rjrjr opened this issue Jan 24, 2025 · 6 comments
Open

RA-64264 Make WorkflowLayout sample based on the frame clock #1257

rjrjr opened this issue Jan 24, 2025 · 6 comments
Labels
optimization Issues related to benchmarking and optimization ui Related to UI integration

Comments

@rjrjr
Copy link
Contributor

rjrjr commented Jan 24, 2025

The renderings: Flow<Screen> provided to WorkflowLayout.take() can fire a lot more often than is useful, especially in large apps that don't use initialState / StateFlow.value effectively. Can take buffer what it receives, keeping only the latest, and take care to call [show] either once or never for frame clock tick?

Remember that it is absolutely fine to drop renderings; each time a new rendering appears it means that the combined workflow state has changed, and the previous ones are now invalid. We're not an animation tool.

@rjrjr rjrjr added optimization Issues related to benchmarking and optimization ui Related to UI integration labels Jan 24, 2025
@rjrjr rjrjr changed the title Make WorkflowLayout throttle itself based on the frame clock Make WorkflowLayout sample based on the frame clock Jan 24, 2025
@zach-klippenstein
Copy link
Collaborator

zach-klippenstein commented Jan 24, 2025

That would be the default behavior if WorkflowLayout were compose (since it would presumably collectAsState or collectAsStateWithLifecycle). I think for a View we could do something like this:

suspend fun <T> Flow<T>.collectAtFrame(choreographer: Choreographer, collector: () -> Unit) {
  var frameScheduled = false
  var mostRecentValue: T? = null
  val frameCallback = FrameCallback {
    // Not worried about races since everything's on the main thread, otherwise we'd need
    // a lock probably.
    frameScheduled = false
    collector(mostRecentValue)
  }

  try {
    collect { value ->
      mostRecentValue = value
      if (!frameScheduled) {
        frameScheduled = true
        // This is a fire-once callback: It automatically gets removed after firing.
        choreographer.postFrameCallback(frameCallback)
      }
    }
  } finally {
    // Noop if no callback currently posted.
    choreographer.removeFrameCallback(frameCallback)
  }
}

Caveat is I'm not sure how this will sync with Compose's frame callback. Compose also does postFrameCallback, so it would end up being a race and if collectAtFrame lost then the new renderings wouldn't show in Compose until the next frame, I think.

@zach-klippenstein
Copy link
Collaborator

We're not an animation tool.

If we were, we'd still want to only operate on frames 😂

@zach-klippenstein
Copy link
Collaborator

zach-klippenstein commented Jan 24, 2025

Actually I think we should probably just make show debounce itself until frame boundary, not put the logic in take.

Image

@zach-klippenstein
Copy link
Collaborator

zach-klippenstein commented Jan 24, 2025

I haven't thought about nesting. If show is called as a result of a parent WorkflowLayout propagating the renderings down the view tree, then it shouldn't defer ever. (Compose also automatically handles this.) I'm not sure if this is a case we need to explicitly handle for views.

Ok so maybe take is the right place to put this debouncing. Assuming, IIRC, that when propagating down the view tree we call show directly and don't use take.

@rjrjr
Copy link
Contributor Author

rjrjr commented Jan 24, 2025

In the few instances where we have nested WorkflowLayout it's due to a separate workflow runtime being spun up by a legacy presenter object and feeding it directly. There is no recursion built into WorkflowLayout itself; and I can't see a reason for feature code to ever do that beause WorkflowViewStub and @Composable WorkflowRendering() exist and are attractive.

@rjrjr
Copy link
Contributor Author

rjrjr commented Jan 24, 2025

Did you just make an argument for us to get WorkflowLayout out of the top of our apps and do this instead? https://github.com/square/workflow-kotlin/blob/main/samples/compose-samples/src/main/java/com/squareup/sample/compose/hellocompose/HelloComposeActivity.kt

@rjrjr rjrjr changed the title Make WorkflowLayout sample based on the frame clock RA-64264 Make WorkflowLayout sample based on the frame clock Feb 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
optimization Issues related to benchmarking and optimization ui Related to UI integration
Projects
None yet
Development

No branches or pull requests

2 participants