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.

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

1249: Cache Intercepted Workflow Instance #1250

Merged
merged 1 commit into from
Jan 28, 2025
Merged

1249: Cache Intercepted Workflow Instance #1250

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 25 additions & 10 deletions workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/internal/WorkflowNode.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,8 @@ internal class WorkflowNode<PropsT, StateT, OutputT, RenderingT>(
override val identifier: WorkflowIdentifier get() = id.identifier
override val renderKey: String get() = id.name
override val sessionId: Long = idCounter.createId()
private var cachedWorkflowInstance: StatefulWorkflow<PropsT, StateT, OutputT, RenderingT>
private var interceptedWorkflowInstance: StatefulWorkflow<PropsT, StateT, OutputT, RenderingT>

private val subtreeManager = SubtreeManager(
snapshotCache = snapshot?.childTreeSnapshots,
Expand Down Expand Up @@ -99,8 +101,9 @@ internal class WorkflowNode<PropsT, StateT, OutputT, RenderingT>(
init {
interceptor.onSessionStarted(this, this)

state = interceptor.intercept(workflow, this)
.initialState(initialProps, snapshot?.workflowSnapshot, this)
cachedWorkflowInstance = workflow
interceptedWorkflowInstance = interceptor.intercept(cachedWorkflowInstance, this)
state = interceptedWorkflowInstance.initialState(initialProps, snapshot?.workflowSnapshot, this)
}

override fun toString(): String {
Expand Down Expand Up @@ -132,10 +135,10 @@ internal class WorkflowNode<PropsT, StateT, OutputT, RenderingT>(
fun snapshot(workflow: StatefulWorkflow<*, *, *, *>): TreeSnapshot {
@Suppress("UNCHECKED_CAST")
val typedWorkflow = workflow as StatefulWorkflow<PropsT, StateT, OutputT, RenderingT>
updateCachedWorkflowInstance(typedWorkflow)
return interceptor.onSnapshotStateWithChildren({
val childSnapshots = subtreeManager.createChildSnapshots()
val rootSnapshot = interceptor.intercept(typedWorkflow, this)
.snapshotState(state)
val rootSnapshot = interceptedWorkflowInstance.snapshotState(state)
TreeSnapshot(
workflowSnapshot = rootSnapshot,
// Create the snapshots eagerly since subtreeManager is mutable.
Expand Down Expand Up @@ -202,6 +205,20 @@ internal class WorkflowNode<PropsT, StateT, OutputT, RenderingT>(
coroutineContext.cancel(cause)
}

/**
* Call this after we have been passed any workflow instance, in [render] or [snapshot]. It may
* have changed and we should check to see if we need to update our cached instances.
*/
private fun updateCachedWorkflowInstance(
workflow: StatefulWorkflow<PropsT, StateT, OutputT, RenderingT>
) {
if (workflow !== cachedWorkflowInstance) {
// The instance has changed.
cachedWorkflowInstance = workflow
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do you update cachedWorkflowInstance first in init, but second here? Is there some case where intercept needs the cached workflow to not be updated yet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope. Just a coincidence. I can unify the order so its not confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

interceptedWorkflowInstance = interceptor.intercept(cachedWorkflowInstance, this)
}
}

/**
* Contains the actual logic for [render], after we've casted the passed-in [Workflow]'s
* state type to our `StateT`.
Expand All @@ -210,11 +227,11 @@ internal class WorkflowNode<PropsT, StateT, OutputT, RenderingT>(
workflow: StatefulWorkflow<PropsT, StateT, OutputT, RenderingT>,
props: PropsT
): RenderingT {
updatePropsAndState(workflow, props)
updateCachedWorkflowInstance(workflow)
updatePropsAndState(props)

baseRenderContext.unfreeze()
val rendering = interceptor.intercept(workflow, this)
.render(props, state, context)
val rendering = interceptedWorkflowInstance.render(props, state, context)
baseRenderContext.freeze()

workflowTracer.trace("UpdateRuntimeTree") {
Expand All @@ -230,12 +247,10 @@ internal class WorkflowNode<PropsT, StateT, OutputT, RenderingT>(
}

private fun updatePropsAndState(
workflow: StatefulWorkflow<PropsT, StateT, OutputT, RenderingT>,
newProps: PropsT
) {
if (newProps != lastProps) {
val newState = interceptor.intercept(workflow, this)
.onPropsChanged(lastProps, newProps, state)
val newState = interceptedWorkflowInstance.onPropsChanged(lastProps, newProps, state)
state = newState
}
lastProps = newProps
Expand Down
Loading