Introduce explicit internal datastructures modeling dom state #2330

WorldSEnder · 2022-01-06T00:21:09Z

Description

Prior to this change, reconciliation of the virtual DOM works by mutating the incoming virtual DOM internally, taking an optional ancestor parameter.
With this change, for each (most) type of virtual node, there is an explicit second structure modeling the state that is relevant only after the virtual DOM has been applied (reconciled). In effect, this slims down some of the vdom structures and gets rid of some unwraps by making illegal state unrepresentable.

The introduced structure is called "Bundle" throughout the code, coming from the mathematical sense, meaning here "living above the virtual dom".

Tries to continue tackling #758. Most of the vdom should be more easily removable now that it is rid of state logic.

Should make it easier to implement #2154 as "host logic" (listener registration) can more easily be added to the bundle structure.

As a side effect of a change to the list reconciler, fixes #2327 .

Might impact future considerations in #1154.

Please apply the performance label.

How to review this change

Most of the test cases have been moved into the new dom_bundle module, as it tests how the vdom is reconciled and rendered. There might be some test cases that only inspect the vdom structure, but I've honestly not looked too closely. It might make sense to split those off (back into virtual_dom).

The change should be invisible to most, if not all users of yew. Please take a few minutes to double check that I haven't accidentally removed trait impls from vdom structures. Further moved methods are all internal (pub(crate)) to yew. No pub methods should have been moved.

Obviously, some fields have been removed or renamed in the virtual_dom structures, but I don't think this is defined as a "stable" public interface of yew?

Checklist

I have run cargo make pr-flow
I have reviewed my own code
I have added tests

WorldSEnder · 2022-01-06T00:26:34Z

~~Test cases seem to spuriously fail due to bumpalo = "3.9.0" not compiling on older releases of rust.~~ Fixed now

See fitzgen/bumpalo@55b3fa5

packages/yew/src/dom_bundle/bportal.rs

voidpumpkin

To me all this looks good. Great work!

But i would like to see this PR benchmark results after we update the benchmarks in #2352

WorldSEnder · 2022-01-10T19:00:09Z

Seems there is a permissions issue with posting the results of benchmarking 😵‍💫 ready to rebase when that's fixed.

voidpumpkin · 2022-01-10T19:04:06Z

Seems there is a permissions issue with posting the results of benchmarking 😵‍💫 ready to rebase when that's fixed.

Working on it

voidpumpkin · 2022-01-10T21:04:07Z

dang that altert looks suspicious, let me re-run...

WorldSEnder · 2022-01-10T21:23:54Z

Might not be that suspicious after all, but I'll wait for some "normal" PRs to go through the benchmarking suit and see if they pass before I trust it. In any case, I'll have to check I understand what is measured and why it regresses

WorldSEnder · 2022-01-10T21:27:13Z

Hm, similar "regressions" detected in #2337 which means you are right, something is fishy.

the new bcomp is especially nice and lost a few unwraps owed to not having to reserve space for a scope before rendering. Note also that bsuspense has been slimmed a bit, storing the suspended flag implicitly in the state. some naming is not perfect yet and has to be adjusted still.

voidpumpkin · 2022-01-10T23:07:39Z

Yea sorry just the env is unstable it seems. We can't assume that there are regressions yet

WorldSEnder · 2022-01-31T03:08:43Z

I won't bother merging master all the time while the old virtual dom is changing as rapidly as it is now, if that means I have to port conflict after conflict because git doesn't seem to understand how to resolve them naturally.

voidpumpkin · 2022-01-31T07:51:52Z

I won't bother merging master all the time while the old virtual dom is changing as rapidly as it is now, if that means I have to port conflict after conflict because git doesn't seem to understand how to resolve them naturally.

Yeah its a weird situation. Originally i was not approving PR's to wait until yours gets merged, but then every time I would check up on this PR it was super WIP. Later you even stated its not mergable.

If you want we can slow down for a week or something to let you catch up and merge this in.
@hamza1311 would you be ok with that?

WorldSEnder · 2022-01-31T08:07:36Z

This week is a bit stressful for me because of university, so the "pause" doesn't have to happen immediately, I'd much rather prefer to not add pressure before my exam anyway :)

I suppose #2421 could be merged in by next week, so unless @futursolo has any other planned PRs upcoming, maybe a good point in time would be after that one.

hamza1311 · 2022-01-31T08:11:17Z

I was waiting that hoping @voidpumpkin or @siku2 would review this before me and I can keep their review in mind. I haven't been able to sit down and focus on any PRs for the last little bit (my own PR is sitting with failing CI)

I'd be fine with not merging anything else until this one is in.

voidpumpkin · 2022-01-31T09:33:28Z

This week is a bit stressful for me because of university, so the "pause" doesn't have to happen immediately, I'd much rather prefer to not add pressure before my exam anyway :)

I suppose #2421 could be merged in by next week, so unless @futursolo has any other planned PRs upcoming, maybe a good point in time would be after that one.

I will be gone for a month after 2 weeks. So github will probably slowdown a bit without me.
So you with this PR will be in @siku2 and @hamza1311 hands :)

hamza1311 · 2022-02-17T18:20:51Z

@WorldSEnder can you fix the conflicts? I'll make this PR the first thing I review after the conflicts resolved (I assumed it's mostly good because last time i reviewed it, it was fine)

WorldSEnder · 2022-02-18T02:09:28Z

One last assignment due for uni, I'll see if I can get this done over the weekend

WorldSEnder · 2022-02-22T23:03:33Z

Makefile.toml

@@ -82,7 +82,7 @@ dependencies = ["test"]
 [tasks.test]
 private = true
 command = "cargo"
-args = ["test", "--all-targets", "--workspace", "--exclude", "website-test"]
+args = ["test", "--all-targets"]


This change should fix #2403 . As I understand it, tasks are inherited and run for all workspace members anyway, unless overwritten.

hamza1311

Looks good. Sorry, it took such a long time to get reviewed

hamza1311 · 2022-03-02T15:56:01Z

packages/yew/src/dom_bundle/app_handle.rs

@@ -1,17 +1,16 @@
-//! This module contains the `App` struct, which is used to bootstrap


I'm fine with keeping it here. If we support any platform other than web, then it'll have to but that's for the future.

hamza1311 · 2022-03-02T16:21:33Z

packages/yew/src/html/component/lifecycle.rs

+pub struct CreateRunner<COMP: BaseComponent> {
+    pub initial_render_state: ComponentRenderState,
+    pub node_ref: NodeRef,
+    pub props: Rc<COMP::Properties>,


What is possible (and I think worth doing) is removing the Boxes in scheduler and storing and Rc<dyn smth> to the ComponentState<COMP> instead.

I agree that we should do this. Want to create an issue for this?

voidpumpkin

LGTM

hamza1311

Looks good! Finally we can merge this in

voidpumpkin added A-yew Area: The main yew crate performance labels Jan 6, 2022

hamza1311 mentioned this pull request Jan 6, 2022

Bump MSRV to 1.56 #2334

Merged

3 tasks

futursolo mentioned this pull request Jan 6, 2022

Server-side Rendering (without hydration) #2335

Merged

3 tasks

WorldSEnder force-pushed the dom-bundle branch from bcde660 to 545a45d Compare January 6, 2022 17:35

hamza1311 added this to the v0.20 milestone Jan 6, 2022

voidpumpkin reviewed Jan 10, 2022

View reviewed changes

packages/yew/src/dom_bundle/bportal.rs Show resolved Hide resolved

voidpumpkin reviewed Jan 10, 2022

View reviewed changes

WorldSEnder force-pushed the dom-bundle branch from 545a45d to dee8b51 Compare January 10, 2022 17:36

WorldSEnder force-pushed the dom-bundle branch 2 times, most recently from 243cf7f to 0d306ff Compare January 10, 2022 20:51

WorldSEnder added 10 commits January 10, 2022 23:32

detach destructures now

5f6e8ed

add failing keyed-list issue

b302a88

crude port to the new bundle infrastructure

994a6db

mass rename: apply -> reconcile

355e328

get rid of move_before in favor of shift

35d7c2e

generate id directly when creating a new scope

d9bd30a

bundle for text nodes

c843a2b

work on naming: ancestor -> bundle

f576c36

slightly optimize list reconciler, add doccomments

751fed2

WorldSEnder force-pushed the dom-bundle branch from 0d306ff to 751fed2 Compare January 10, 2022 22:32

WorldSEnder requested a review from voidpumpkin January 10, 2022 23:01

WorldSEnder force-pushed the dom-bundle branch from 29489bf to c713027 Compare January 29, 2022 03:58

WorldSEnder added 2 commits February 20, 2022 17:25

Merge remote-tracking branch 'upstream/master' into dom-bundle

5010f34

Merge remote-tracking branch 'upstream/master' into dom-bundle

02c7dd6

WorldSEnder commented Feb 22, 2022

View reviewed changes

futursolo mentioned this pull request Feb 23, 2022

SSR Hydration #2453

Closed

3 tasks

hamza1311 previously approved these changes Mar 2, 2022

View reviewed changes

hamza1311 requested a review from cecton March 2, 2022 16:32

WorldSEnder mentioned this pull request Mar 2, 2022

Push ComponentState into the scheduler instead of boxed runners #2485

Open

3 tasks

Merge branch 'master' into dom-bundle

c1a7ba9

WorldSEnder dismissed hamza1311’s stale review via c1a7ba9 March 5, 2022 18:50

WorldSEnder added 2 commits March 5, 2022 21:50

Merge remote-tracking branch 'upstream/master' into dom-bundle

0678829

fix naming of BNode variants

bea0708

voidpumpkin approved these changes Mar 5, 2022

View reviewed changes

WorldSEnder requested a review from hamza1311 March 5, 2022 22:38

hamza1311 approved these changes Mar 6, 2022

View reviewed changes

hamza1311 merged commit 78d4204 into yewstack:master Mar 6, 2022

hamza1311 mentioned this pull request Mar 6, 2022

Better Function Component in docs + Macro-based Hooks #2478

Merged

3 tasks

futursolo mentioned this pull request Mar 6, 2022

Remove cargo make benchmarks #2495

Merged

3 tasks

WorldSEnder deleted the dom-bundle branch March 6, 2022 13:40

WorldSEnder mentioned this pull request Mar 12, 2022

Element switching order #2462

Closed

3 tasks

WorldSEnder mentioned this pull request Dec 6, 2022

Fix top-level reconciliation in portals #3020

Merged

2 tasks

kaisalmon mentioned this pull request Dec 23, 2022

CONTRIBUTING.md refers to benchmarking script which no longer exists #3058

Closed

8 tasks

WorldSEnder mentioned this pull request May 11, 2023

Regression: elements are re-rendered even if they didn't change, when there's a list below them (worked in v0.19.3) #3262

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce explicit internal datastructures modeling dom state #2330

Introduce explicit internal datastructures modeling dom state #2330

WorldSEnder commented Jan 6, 2022 •

edited

WorldSEnder commented Jan 6, 2022 •

edited

voidpumpkin left a comment

WorldSEnder commented Jan 10, 2022

voidpumpkin commented Jan 10, 2022

voidpumpkin commented Jan 10, 2022

WorldSEnder commented Jan 10, 2022

WorldSEnder commented Jan 10, 2022

voidpumpkin commented Jan 10, 2022

WorldSEnder commented Jan 31, 2022

voidpumpkin commented Jan 31, 2022 •

edited

WorldSEnder commented Jan 31, 2022 •

edited

hamza1311 commented Jan 31, 2022 •

edited

voidpumpkin commented Jan 31, 2022

hamza1311 commented Feb 17, 2022

WorldSEnder commented Feb 18, 2022

WorldSEnder Feb 22, 2022

hamza1311 left a comment

hamza1311 Mar 2, 2022

hamza1311 Mar 2, 2022

voidpumpkin left a comment

hamza1311 left a comment

		@@ -1,17 +1,16 @@
		//! This module contains the `App` struct, which is used to bootstrap

Introduce explicit internal datastructures modeling dom state #2330

Introduce explicit internal datastructures modeling dom state #2330

Conversation

WorldSEnder commented Jan 6, 2022 • edited

Description

How to review this change

Checklist

WorldSEnder commented Jan 6, 2022 • edited

voidpumpkin left a comment

Choose a reason for hiding this comment

WorldSEnder commented Jan 10, 2022

voidpumpkin commented Jan 10, 2022

voidpumpkin commented Jan 10, 2022

WorldSEnder commented Jan 10, 2022

WorldSEnder commented Jan 10, 2022

voidpumpkin commented Jan 10, 2022

WorldSEnder commented Jan 31, 2022

voidpumpkin commented Jan 31, 2022 • edited

WorldSEnder commented Jan 31, 2022 • edited

hamza1311 commented Jan 31, 2022 • edited

voidpumpkin commented Jan 31, 2022

hamza1311 commented Feb 17, 2022

WorldSEnder commented Feb 18, 2022

WorldSEnder Feb 22, 2022

Choose a reason for hiding this comment

hamza1311 left a comment

Choose a reason for hiding this comment

hamza1311 Mar 2, 2022

Choose a reason for hiding this comment

hamza1311 Mar 2, 2022

Choose a reason for hiding this comment

voidpumpkin left a comment

Choose a reason for hiding this comment

hamza1311 left a comment

Choose a reason for hiding this comment

WorldSEnder commented Jan 6, 2022 •

edited

WorldSEnder commented Jan 6, 2022 •

edited

voidpumpkin commented Jan 31, 2022 •

edited

WorldSEnder commented Jan 31, 2022 •

edited

hamza1311 commented Jan 31, 2022 •

edited