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

Offer config option to skip rendering when new state == old #985

Closed
rjrjr opened this issue Apr 7, 2023 · 3 comments
Closed

Offer config option to skip rendering when new state == old #985

rjrjr opened this issue Apr 7, 2023 · 3 comments
Assignees

Comments

@rjrjr
Copy link
Contributor

rjrjr commented Apr 7, 2023

We have been afraid to simply make this change because it will inevitably break so many existing workflows. But everyone writes code assuming this is how things work, and that causes our worst performance problems. Let's at least offer the option to turn that behavior on, and consider gradually moving to make that the default.

Also not that if an output is emitted but no state is changed, there is still no reason to re-render. It seems to me we can consider posting output and changing state as orthogonal things.

@steve-the-edwards
Copy link
Contributor

So, with #992 we will add exactly this. See that PR for explanation and pertinent points. Tests are passing and results looking good re: the # of render passes.

A further step beyond this could actually be to record a path from the the root of the Workflow tree (R) to the node where the state changed started from the action (lets call this N).

I believe we only need to re-render the path of nodes from R-N as they are the only ones that can be affected by this state change, within the next render pass. That state change of course could have knock on effects for any and all of the other nodes but those would need to be applied in separate actions.

The path R-N could start as naive (render all nodes in the path and all the children of all those nodes), then we could optimize it even further where in the path R-N we only render a node A's child if that child is on the path R-N or if the state change to A has modify the props passed to the child.

I'm not sure how we would manage knitting together the cached renderings w/ new renderings in order to build the full rendering.

@rjrjr
Copy link
Contributor Author

rjrjr commented Apr 27, 2023

I'm not sure how we would manage knitting together the cached renderings w/ new renderings in order to build the full rendering.

I think you had a pretty clear idea on that this morning: we always render everything beneath a dirty node, and everything above it; but can use the cached renderings for the dirty node's peers.

@steve-the-edwards
Copy link
Contributor

Fixed by #992

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

No branches or pull requests

2 participants