-
Notifications
You must be signed in to change notification settings - Fork 112
DRAIN_EXCLUSIVE_ACTIONS implementation #1355
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
Conversation
b276a5b to
eefc6d8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the interaction between conflation and drainExclusive. It looks like we run the drainExclusive loop first, then render, then conflate, drainExclusive, and render, etc. Wouldn't we want to continuously interleave drainExclusive and conflation passes? Or is that what we're doing and I just can't read code?
| ): Unit = Unit | ||
|
|
||
| public sealed interface RuntimeLoopOutcome | ||
| public sealed interface RuntimeUpdate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the risk of over-optimizing, this could be a value class to avoid allocating RenderingProduced objects every pass even when they're ignored.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice. will take a look at doing this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd have to talk more with you about what you are thinking. I am not sure we even need to report the rendering itself to the interceptor. I can start by just removing that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, that object is already allocated (probably why I was not worried originally). So we could start by just making RenderingProduced a value class I think I was confused originally because this comment was on the RuntimeUpdate sealed interface
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made RenderingProduced a value class now. The others are just objects, so this should all be quite fast / have minimal allocations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no point in making a subclass of this a value class, since it will be boxed when referred to as a parent type. We'd need to replace the entire sealed class/interface hierarchy with a single value class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, interesting!
| } | ||
|
|
||
| @Test | ||
| fun `for_drain_exclusive_and_render_only_when_state_changes_we_handle_multiple_actions_in_one_render_but_we_do_pass_rendering_if_state_changed_earlier`() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: You shouldn't need the backticks here, you're not using spaces or other illegal characters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's actually because this is KMP code that runs on js as well, IIRC. For some reason the kotlin-js compiler can't handle long names that don't have ticks? Or maybe that's why I had to add the _. Now I can't remember. But it was something with kotlin-js.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out I was just remembering wrong or had copied this forward when not needed. Removed now!
e6b2344 to
1551901
Compare
f116b2d to
f38b27a
Compare
5b45b9a to
44397aa
Compare
9a60fe3 to
0acd80d
Compare
4f4977a to
a3bf0ad
Compare
0acd80d to
4d41e92
Compare
the main loop (after the first synchronous render) is:
No interleaving is necessary from what I can tell. The exclusive actions for DEA are just a subset of the actions that can be applied without rendering at all. |
b0dfc9c to
3635e7d
Compare
| if (!runtimeConfig.contains(PARTIAL_TREE_RENDERING) || | ||
| !lastRendering.isInitialized || | ||
| subtreeStateDidChange | ||
| subtreeStateDirty |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we be checking the selfStateDirty here as well?
is there in which the self state might be dirty but the subtree isnt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no. if self is dirty, subtree is also dirty. Not true the other way around though, subtree may be dirty without the self being dirty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Subtree includes self, basically.
It'd be good to capture this in a comment in the code. |
Why would exclusive actions only be available after (1) and not after (3)? |
We chatted offline and realized I need to make a few things much more clear in the code:
|
83c12df to
176b598
Compare
176b598 to
e48d80e
Compare
Add an option to get the next action that is mutually exclusive to the dirty nodes. Then we can apply as many of those actions as possible.
Add tests for this as well and ensure existing tests can run with this runtime config on.