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

Add Configurable Recursive Rendering to Performance Poetry #903

Merged
merged 1 commit into from
Dec 16, 2022

Conversation

steve-the-edwards
Copy link
Contributor

@steve-the-edwards steve-the-edwards commented Dec 10, 2022

Customizable via intent.

Depth is how many times to recursively render PerformancePoemsBrowserWorkflow in the tree.
Breadth is how many times to repeate rendering of PerformancePoemsBrowserWorkflow horizontally in the tree.

Only one path of the tree provides a rendering.

@steve-the-edwards steve-the-edwards force-pushed the sedwards/perf-experiments branch 4 times, most recently from 6003718 to 411044e Compare December 14, 2022 19:43
@steve-the-edwards steve-the-edwards changed the title Experimental Perf Branches Add Configurable Recursive Rendering to Performance Poetry Dec 14, 2022
@steve-the-edwards steve-the-edwards force-pushed the sedwards/perf-experiments branch 2 times, most recently from f8eb139 to b5e9831 Compare December 14, 2022 20:11
@steve-the-edwards steve-the-edwards marked this pull request as ready for review December 14, 2022 20:46
@rjrjr
Copy link
Contributor

rjrjr commented Dec 15, 2022

The commit message should be a comment in the code somewhere.

I don't understand this:

Only one path of the tree provides a rendering.

It looked like you were rendering children for breadth as well as depth, just that most of them ignored output. What do you actually mean by "provides a rendering"?

@steve-the-edwards
Copy link
Contributor Author

The commit message should be a comment in the code somewhere.

I don't understand this:

Only one path of the tree provides a rendering.

It looked like you were rendering children for breadth as well as depth, just that most of them ignored output. What do you actually mean by "provides a rendering"?

All of them are 'rendered' from the POV of Workflow runtime, but only 1 path of the tree (the (n-1)th child of each depth) actually returns the rendering from render() and so only one path passes a rendering to the UI layer. I will make that clearer in the comment I add.

@steve-the-edwards
Copy link
Contributor Author

steve-the-edwards commented Dec 15, 2022

  • Add comment for what this does.

@steve-the-edwards steve-the-edwards force-pushed the sedwards/perf-experiments branch from b5e9831 to 2809f01 Compare December 15, 2022 16:17
* simultaneously and tries to represent a scenario of multiple Workflows listening to the same
* action.
* [traceRenderingPasses], [traceFrameLatency], [traceEventLatency] are all flags to add
* instrumentation for different performance measurements.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rjrjr @RBusarow - I added a description for recursionGraph here and while I was added I described each of the other config levers.

@rjrjr
Copy link
Contributor

rjrjr commented Dec 15, 2022

All of them are 'rendered' from the POV of Workflow runtime, but only 1 path of the tree (the (n-1)th child of each depth) actually returns the rendering from render() and so only one path passes a rendering to the UI layer. I will make that clearer in the comment I add.

Do the other ones render null? Is that mimicking a pattern in our prod code?

@steve-the-edwards
Copy link
Contributor Author

All of them are 'rendered' from the POV of Workflow runtime, but only 1 path of the tree (the (n-1)th child of each depth) actually returns the rendering from render() and so only one path passes a rendering to the UI layer. I will make that clearer in the comment I add.

Do the other ones render null? Is that mimicking a pattern in our prod code?

They don't render null, we just don't do anything with their renderings.

@@ -50,9 +51,11 @@ class PerformancePoemsBrowserWorkflow(
private val isLoading: MutableStateFlow<Boolean>,
) :
PoemsBrowserWorkflow,
StatefulWorkflow<List<Poem>, State, Unit, OverviewDetailScreen>() {
StatefulWorkflow<Pair<Pair<Int, Int>, List<Poem>>, State, Unit, OverviewDetailScreen>() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Replacing Pair<Int, Int> with a data class or a typealias and some extension methods would make this a lot easier to read.

val nextProps = renderProps.copy(
first = renderProps.first.first - 1 to breadth
)
// ignore the siblings.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment confuses me. Is it about the noAction()? If so, how about put it there and say "Ignore sibling output."? Be good to say why we're ignoring it too.

Customizable via intent.

Depth is how many times to recursively render PerformancePoemsBrowserWorkflow in the tree.
Breadth is how many times to repeate rendering of PerformancePoemsBrowserWorkflow horizontally in the tree.

Only one path of the tree provides a rendering.
@steve-the-edwards steve-the-edwards force-pushed the sedwards/perf-experiments branch from 2809f01 to 96d5579 Compare December 16, 2022 15:54
@steve-the-edwards steve-the-edwards merged commit 665c4ce into main Dec 16, 2022
@steve-the-edwards steve-the-edwards deleted the sedwards/perf-experiments branch December 16, 2022 17:57
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

Successfully merging this pull request may close these issues.

2 participants