Open rjrjr opened 3 years ago
Both initialState
and onPropsChanged
are similar to the regular action
minus emitting an output. Making both methods return WorkflowAction
instead of WorkflowState
will enable codebase unification (when initial state isn't a special case) and will reduce the described ugliness.
One of usecases where it could be valuable is the case when the workflow is outputting some function of its state (extremely, workflow output is the same model as a state). It's a legit usecase, especially for dumb child workflows that delegate most of handling to the parent workflow. Right now consumers have to implement the ugly hack with one-shot worker, but it's not elegant and could be racey or error-prone.
@zach-klippenstein I'm going to first play around with the simplest flavor of this I can think of: simply suppress emitting a rendering while the actionSink is not empty. I expect to find out that it's not simple at all, but at least it's a place to start digging.
I'm less excited about emitting output from initialState
, if only because of the unspeakable migration nightmare it would be. I guess my hope is that I can find some low hanging fruit to reduce the penalty when people hack up their own mechanisms to achieve that effect.
I think it's well worth doing because I'm finding it's so hard to avoid an intial render storm as a deep workflow tree is set up, and all kinds of async workers are subscribed to at set up. It's hard to optimize things to avoid an initial redundant update from such workers (if you can even be confident they're redundant), and why should people have to fret about that in the first place?
Could you give a more specific example of a workflow that emits output as a function of state @0legg?
@zach-klippenstein Please take a look at AddressWorkflow
at internal repo, this is exactly the case of state and output being the same.
@rjrjr I'm not so sure that the migration effort would be so tedious if we can find a way to map the WorkflowState
to WorkflowAction
using just static analysis of the Workflow file. We could do something similar to the WF1 migrator.
This would be a lot easier to try out if we had a single eventChannel
for the entire tree rather than one per WorkflowNode
. Have to wonder if that would make render passes a little faster too.
Also seems like we could be caching sub tree renderings. If no actions are queued for a tree and its props don't change, there is no reason to re-render it. Could have a big impact for split views and layers.
This would be easier if we preserve the channel-per-workflow, I think.
@rjrjr I'm starting to parse out the different streams of work/thoughts here:
I think 3 is about convenience for a 'smell' where these Outputs should likely be folded into the Rendering of the child. Its not performance related per se but we could mitigate the negative performance impact of the smell by changing the API.
1 and 2 are very different areas to work on the same kind of optimizations - possibly could both be used.
What I am trying to decide (and want to talk with you and @RBusarow about more) is how we want to setup a demo app that we can use to measure the impact of these changes.
I am thinking of extending poetry to have more complicated event interactions - clicking each stanza/row/letter will impact the List Rendering as well as the Detail Rendering - i.e. showing 'current' someway in the 'overview pane' and doing it with more layers (perhaps down the letter).
I am also thinking of adding in asynchronous Workers that will do a hash on the poem content to simulate 'real world' work/calculations based on the selections.
What I can't decide is if I should make this app include the commonly seen 'smells' that we have encountered in order to see what kind of optimizations will impact those as well. E.g. - should these layers have 'InitializingStates' that are waiting for that hash to run to give their first Output back to their parent - thus churning the render passes more than needed? Should we have some layers that emit Output immediately to their parent (a la Observable.just(Unit).asWorker())?
If you can't beat em, join em?
This would have the added effect of having our performance demo app be a 'bad' actor until we force it not to be (via API changes etc.). WDYT?
tl;dr: I think #2 is the most promising. Square Flow has always worked that way (though its plumbing is much simpler of course), and it worked very well.
I think #1 was just a bad attempt at articulating #2.
I've gone very cold on #3, seems like most times I dig into that it's a sign that people are using OuputT for info that can more easily be passed via RenderingT
I don't see this as a ui-1.0 blocker, moving it out of that milestone. @steve-the-edwards if you disagree, maybe want to see us hold on to the @WorkflowUiExperimentalApi
annotation a bit longer for more wiggle room, please sing out.
The issue with a naive implementation of (2) - "holding off on render() until action queues are empty" is that we would need to pump all the individual node's actions into one shared queue in order to be able to quickly check if it is empty. I naively implemented queue checking and it resulted in much slower performance when there wasn't an action storm.
I think we can take a hint from (1) regarding frame rate and use that as one of the conditions for re-rendering.
When we process actions:
First wait for an event to happen or props to update. If the event caused Output or there were new Props, go and render(). After this continue to loop on selecting our tick() and end on 1 of 3 conditions:
Distilling slack monologue thread:
Observable.just(Unit).asWorker {...}
on first render and have it update state / emit output.