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

Allow runningWorker / renderChild handlers to return null and avoid re-render. #915

Closed
rjrjr opened this issue Feb 1, 2023 · 4 comments
Closed
Labels
good first issue Good for newcomers optimization Issues related to benchmarking and optimization

Comments

@rjrjr
Copy link
Contributor

rjrjr commented Feb 1, 2023

The only way a handler can no-op is by returning noAction() or equivalent, which means every time any worker or child workflow fires a render pass must happen. We've talked about noticing when state and output are not changed, and avoiding re-render at that point, but defining "changed" is daunting, and it's very late in the game for such a semantic shift. But allowing an explicit null return should be easy and safe.

Should deprecate noAction() at the same time.

@rjrjr rjrjr added optimization Issues related to benchmarking and optimization good first issue Good for newcomers labels Feb 1, 2023
@rjrjr rjrjr changed the title Allow Worker handlers to return null and avoid re-render. Allow runningWorker / renderChild handlers to return null and avoid re-render. Apr 5, 2023
@rjrjr
Copy link
Contributor Author

rjrjr commented Apr 5, 2023

I wonder if we can go deeper in this direction. E.g., can we instrument WorkflowAction.Updater.state such that if state = is not called; and the parent consuming setOutput returns a null action; we short circuit in that case too.

That's riskier. Existing code might be inadvertently relying on rendering direct changes to mutable state, or to external state. But that seems like a pretty narrow space.

cc @steve-the-edwards, WDYT?

@steve-the-edwards
Copy link
Contributor

steve-the-edwards commented Apr 10, 2023

Quick note so we don't lose context on this. @jamieQ and I paired on this and given currently how WorkerWorkflow routes output via the actionSink even having an output handler for runningWorker that could return null wouldn't avoid a re-render as we need to execute that Output handler deeper in internal code for WorkerWorkflow in order to prevent an action from hitting the sink (and therefore re-rendering).

@steve-the-edwards
Copy link
Contributor

Not needed due to solution for #985

@steve-the-edwards
Copy link
Contributor

Covered by #985 and fixed by #992

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers optimization Issues related to benchmarking and optimization
Projects
None yet
Development

No branches or pull requests

2 participants