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

Jump to bottom

988: Cache RenderContext per instance #1251

Merged
merged 1 commit into from
Jan 28, 2025
Merged

988: Cache RenderContext per instance #1251

merged 1 commit into from
Jan 28, 2025
+83 −12

Conversation

steve-the-edwards
Copy link
Contributor

@steve-the-edwards steve-the-edwards commented Jan 20, 2025 •
edited
Loading

Rather than re-creating the RenderContext on each render for an intercepted workflow instance, or for the StatefulWorkflow version of a StatelessWorkflow, we create it only once, the first time it is rendered, then cache and re-use it, until a new instance is passed in to render(), then we need to recreate it.

Fixes #988 .

@steve-the-edwards steve-the-edwards force-pushed the sedwards/cache-render-context branch 2 times, most recently from d54c92e to cf3f7c9 Compare January 21, 2025 16:11
@steve-the-edwards steve-the-edwards marked this pull request as ready for review January 21, 2025 16:13
@steve-the-edwards steve-the-edwards added the optimization Issues related to benchmarking and optimization label Jan 21, 2025
@steve-the-edwards
Copy link
Contributor Author

steve-the-edwards commented Jan 21, 2025 •
edited
Loading

Idempotency checker is failing with StatelessWorkflows since they don't take on new contexts but use the cached one. https://github.com/square/workflow-kotlin/actions/runs/12891078212/job/35942744500?pr=1251

Update: FIXED - our RenderTester relies on being able to update the RenderContext in separate calls to render() on the workflow - so we account for that in the caching.

@steve-the-edwards steve-the-edwards force-pushed the sedwards/cache-render-context branch from cf3f7c9 to d9ace4f Compare January 21, 2025 18:11
rjrjr
rjrjr approved these changes Jan 21, 2025
View reviewed changes
workflow-core/src/commonMain/kotlin/com/squareup/workflow1/StatelessWorkflow.kt Outdated
renderState: Unit,
context: RenderContext
): RenderingT {
if (cachedStatelessRenderContext == null || context != cachedStatefulRenderContext) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the bit you put in for ease of testing? Seems like it warrants a comment, esp. call out when / why the context might change.

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, this is the bit - I can add some comments here.

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.

zach-klippenstein
zach-klippenstein reviewed Jan 21, 2025
View reviewed changes
workflow-core/src/commonMain/kotlin/com/squareup/workflow1/StatelessWorkflow.kt Outdated
* This method is called a few times per instance, but we don't need to allocate a new
* [StatefulWorkflow] every time, so we store it in a private property.
* This is only called when the instance of the Workflow is created, so we can recreate this each
* time.
Copy link
Collaborator

Choose a reason for hiding this comment

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

"this" refers to both the function and… something else – "so it can create and return a new instance each time"?

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.

workflow-core/src/commonMain/kotlin/com/squareup/workflow1/StatelessWorkflow.kt Outdated
context: RenderContext
): RenderingT {
if (cachedRenderContext == null) {
cachedRenderContext = RenderContext(context, this@StatelessWorkflow)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We support rendering the same workflow instance in multiple places, right? Do all of those render calls also get the same RenderContext? If not, this cache would only work for a single use of this workflow object.

Copy link
Collaborator

Choose a reason for hiding this comment

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

With the new equality check, at least it will now fallback to the old behavior (create a new context each time), which is at least correct if still wasted work.

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, and I ended up using instance ineqaulity !==

@steve-the-edwards steve-the-edwards force-pushed the sedwards/cache-render-context branch from d9ace4f to 6f2d156 Compare January 23, 2025 14:43
@steve-the-edwards steve-the-edwards force-pushed the sedwards/cache-render-context branch from 6f2d156 to 74fb6fb Compare January 23, 2025 15:30
@steve-the-edwards steve-the-edwards force-pushed the sedwards/cache-render-context branch 3 times, most recently from 0a36563 to 717d484 Compare January 24, 2025 16:24
zach-klippenstein
zach-klippenstein approved these changes Jan 27, 2025
View reviewed changes
workflow-core/src/commonMain/kotlin/com/squareup/workflow1/StatelessWorkflow.kt Outdated
render = { props, _ -> render(props, RenderContext(this, this@StatelessWorkflow)) }
)
private val statefulWorkflow: StatefulWorkflow<PropsT, Unit, OutputT, RenderingT> =
object : StatefulWorkflow<PropsT, Unit, OutputT, RenderingT>() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Consider pulling this into a named inner class. Might be better for future debugging if it has a human-readable name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! Will do.

workflow-core/src/commonMain/kotlin/com/squareup/workflow1/StatelessWorkflow.kt Outdated
private val statefulWorkflow: StatefulWorkflow<PropsT, Unit, OutputT, RenderingT> =
object : StatefulWorkflow<PropsT, Unit, OutputT, RenderingT>() {
// We want to cache the render context so that we don't have to recreate it each time
// render() is called.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: These should all be kdoc.

@steve-the-edwards
988: Cache RenderContext per instance
646ebf7 
For the InterceptedRenderContext and for the StatelessWorkflow.RenderContext.
@steve-the-edwards steve-the-edwards force-pushed the sedwards/cache-render-context branch from 717d484 to 646ebf7 Compare January 28, 2025 15:03
Base automatically changed from sedwards/cache-it-all to main January 28, 2025 16:24
@steve-the-edwards steve-the-edwards merged commit be834d1 into main Jan 28, 2025
31 checks passed
@steve-the-edwards steve-the-edwards deleted the sedwards/cache-render-context branch January 28, 2025 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zach-klippenstein zach-klippenstein zach-klippenstein approved these changes

@rjrjr rjrjr Awaiting requested review from rjrjr

Assignees
No one assigned
Labels
optimization Issues related to benchmarking and optimization
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

RenderContext passed into StatefulWorkflow#render is new instance every time
3 participants
@steve-the-edwards @zach-klippenstein @rjrjr