Make Html (VNode) cheap to clone #3431

cecton · 2023-09-30T10:32:34Z

Description

I tried to do this change a few times this year but couldn't make it works.
There was always something blocking me somewhere in the code that was just too
complicated to solve. But now, it seems that Yew has evolved so much that this
change is now easy to do! [edit: actually it wasn't 😓] I'm really happy about it.

I'm not gonna explain the whole thing again. Please refer to the discussion.

Related to #3022

Checklist

I have reviewed my own code
I have added tests

github-actions · 2023-09-30T10:40:21Z

Size Comparison

examples	master (KB)	pull request (KB)	diff (KB)	diff (%)
async_clock	104.058	103.356	-0.701	-0.674%
boids	176.885	177.007	+0.122	+0.069%
communication_child_to_parent	96.438	96.092	-0.347	-0.359%
communication_grandchild_with_grandparent	110.166	109.188	-0.978	-0.887%
communication_grandparent_to_grandchild	106.829	105.545	-1.284	-1.202%
communication_parent_to_child	93.911	93.432	-0.479	-0.511%
contexts	114.539	112.849	-1.690	-1.476%
counter	90.319	90.120	-0.199	-0.221%
counter_functional	91.052	90.788	-0.264	-0.290%
dyn_create_destroy_apps	93.488	93.336	-0.152	-0.163%
file_upload	104.776	104.507	-0.270	-0.257%
function_memory_game	175.780	175.767	-0.014	-0.008%
function_router	355.793	354.042	-1.751	-0.492%
function_todomvc	164.587	164.445	-0.142	-0.086%
futures	229.140	232.297	+3.157	+1.378%
game_of_life	113.382	113.299	-0.083	-0.073%
immutable	189.830	190.524	+0.694	+0.366%
inner_html	87.096	86.324	-0.771	-0.886%
js_callback	114.514	114.036	-0.478	-0.417%
keyed_list	202.075	202.916	+0.841	+0.416%
mount_point	90.317	89.444	-0.873	-0.967%
nested_list	116.955	117.034	+0.079	+0.068%
node_refs	97.415	97.145	-0.271	-0.278%
password_strength	1754.902	1754.218	-0.685	-0.039%
portals	99.504	98.315	-1.188	-1.194%
router	321.744	321.604	-0.140	-0.043%
simple_ssr	145.226	144.829	-0.396	-0.273%
ssr_router	393.559	392.061	-1.498	-0.381%
suspense	120.174	120.247	+0.073	+0.061%
timer	92.914	92.680	-0.234	-0.252%
timer_functional	101.622	101.456	-0.166	-0.163%
todomvc	144.908	144.585	-0.323	-0.223%
two_apps	91.026	90.246	-0.780	-0.857%
web_worker_fib	140.201	139.053	-1.148	-0.819%
web_worker_prime	191.812	189.807	-2.005	-1.045%
webgl	89.669	88.929	-0.740	-0.826%

⚠️ The following examples have changed their size significantly:

examples	master (KB)	pull request (KB)	diff (KB)	diff (%)
communication_grandparent_to_grandchild	106.829	105.545	-1.284	-1.202%
contexts	114.539	112.849	-1.690	-1.476%
futures	229.140	232.297	+3.157	+1.378%
portals	99.504	98.315	-1.188	-1.194%
web_worker_prime	191.812	189.807	-2.005	-1.045%

github-actions · 2023-09-30T10:42:05Z

Benchmark - SSR

Yew Master

Benchmark	Round	Min (ms)	Max (ms)	Mean (ms)	Standard Deviation
Baseline	10	356.463	360.993	358.455	1.310
Hello World	10	621.578	626.818	622.766	1.675
Function Router	10	2109.445	2119.982	2115.930	3.978
Concurrent Task	10	1007.389	1008.839	1008.203	0.476
Many Providers	10	1464.889	1483.306	1473.422	6.645

Pull Request

Benchmark	Round	Min (ms)	Max (ms)	Mean (ms)	Standard Deviation
Baseline	10	319.285	321.955	319.712	0.803
Hello World	10	649.651	658.244	652.310	2.337
Function Router	10	2130.727	2139.741	2135.265	3.006
Concurrent Task	10	1006.466	1008.981	1008.284	0.718
Many Providers	10	1466.062	1492.056	1473.267	7.778

github-actions · 2023-09-30T11:36:23Z

Visit the preview URL for this PR (updated for commit 594e3d4):

https://yew-rs-api--pr3431-html-cheap-to-clone-fnap4qt0.web.app

_{(expires Fri, 03 Nov 2023 13:58:49 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

futursolo

I think the current benchmark result does not reflect improvements in this pull request.
Would it be possible to add a benchmark that demonstrates the performance difference before and after this pull request?

cecton · 2023-10-02T12:42:53Z

Yes 100% agreed..... just need to figure out how xD I think a classic benchmark with criterion on different kind of VNode will work best

ranile

Nothing jumps out by skimming through the code. I agree with futursolo that it needs to benchmarked though

cecton · 2023-10-05T11:23:03Z

There is a new & shiny library 🙌 https://hachyderm.io/@nikolai/111179659061831881

ranile · 2023-10-07T11:27:33Z

There is a new & shiny library 🙌 https://hachyderm.io/@nikolai/111179659061831881

It doesn't run on WASM: nvzqz/divan#3

cecton · 2023-10-18T16:51:18Z

I'm struggling with this workflow xD @futursolo maybe you can help me? I think the commit befcaa2 would have work but the post thing doesn't start. I believe it is because the workflow does not exist yet on the main branch. So I changed it to use 2 steps instead of 2 workflows (e0711b1) and now I have a 403.

Note: This event will only trigger a workflow run if the workflow file is on the default branch.

(From the doc)

futursolo · 2023-10-22T10:16:05Z

I believe it is because the workflow does not exist yet on the main branch. So I changed it to use 2 steps instead of 2 workflows (e0711b1) and now I have a 403.

I think the original approach should work.

GitHub Actions no longer allows pull_request trigger to access protect resources, including posting comments.
You need a separate workflow with a different trigger to post comments.

The only way to figure out if it works is to merge the benchmark and create an additional pull request to test.

futursolo · 2023-10-22T10:19:30Z

packages/yew/src/virtual_dom/vnode.rs

@@ -16,25 +17,27 @@ use crate::AttrValue;
 #[must_use = "html does not do anything unless returned to Yew for rendering."]
 pub enum VNode {


May I ask why option 1 is selected by this pull request?

I feel other options would produce better public APIs.

I remember I couldn't do option 2. I got lost in the change and didn't manage to make it work.

Option 3 is actually used here. It's a mix of option 1 and option 3. In some cases, cloning a single object was cheaper so I added Rc in places it wasn't strictly necessary. (related to #3431 (comment))

Now if I recall I think there wasn't a strong interest of going for one or the other option so I did go wherever it worked.

I think in the end we need both option 1 and 3. Because option 1 is better for the user as they will pass html nodes (Html/VNode) through props so this need to be fast. But option 3 is important for the internals of Yew. Huh but it's really just theory.

Users may also want to pass VChild/VList directly for a better API to components (but those are already cheap to clone, so option 3 is already somewhat implemented)

cecton · 2023-10-24T08:59:27Z

The only way to figure out if it works is to merge the benchmark and create an additional pull request to test.

Ok I have put back the 2 workflows files and I think it's ready to merge.

I think this PR is very important and we should be absolutely sure that the implementation is right as we want it to be. So feel free to take your time to review again @hamza1311 @futursolo maybe others will have opinion on this.

From my point of view I don't see much differences between the different options but I do think having VNode (or Html) cheap-to-clone is a MUST have.

alexkazik · 2023-10-26T08:52:38Z

Out of curiosity: if VList is already cheap to clone (ImplicitClone) why is VNode::VList with Rc?

cecton · 2023-10-26T11:45:41Z

Out of curiosity: if VList is already cheap to clone (ImplicitClone) why is VNode::VList with Rc?

I think it was cheaper to clone 1 Rc instead of multiple ones? At least that's my theory. But let's see what the benchmark says...

With Rc wrapping VList:

vnode           fastest       │ slowest       │ median        │ mean          │ samples │ iters
╰─ vnode_clone  1.787 ns      │ 3.351 ns      │ 1.797 ns      │ 1.814 ns      │ 100     │ 1000000000

Without Rc wrapping VList:

vnode           fastest       │ slowest       │ median        │ mean          │ samples │ iters
╰─ vnode_clone  3.338 ns      │ 5.379 ns      │ 3.367 ns      │ 3.393 ns      │ 100     │ 1000000000

ranile · 2023-10-26T12:55:40Z

@cecton the best way to ensure the workflow works is rip it out of this PR and make another one with the benchmarks that's merged before this one. Then you can rebase this one and get the benchmark results

cecton · 2023-10-26T14:03:23Z

@cecton the best way to ensure the workflow works is rip it out of this PR and make another one with the benchmarks that's merged before this one. Then you can rebase this one and get the benchmark results

I don't know why I didn't think of that earlier xD

ranile · 2023-10-27T12:26:01Z

@cecton I merged #3487, can you rebase this?

github-actions · 2023-10-27T13:58:02Z

Benchmark - core

Yew Master

vnode           fastest       │ slowest       │ median        │ mean          │ samples │ iters
╰─ vnode_clone  78.23 ns      │ 86.75 ns      │ 80.8 ns       │ 80.9 ns       │ 100     │ 1000000000

Pull Request" >> comment.txt

vnode           fastest       │ slowest       │ median        │ mean          │ samples │ iters
╰─ vnode_clone  5.215 ns      │ 6.788 ns      │ 5.434 ns      │ 5.548 ns      │ 100     │ 1000000000

cecton · 2023-10-27T14:08:05Z

@ everyone dont hesitate to push commits on the PR to test things out!

ranile

I'm fine with merging it. There is more indirection in reading the data, not sure how that affects things. Cheap cloning is very important for Html as it's everywhere and cloned most of the time it's used

The benchmarks show a big improvement and no one has raised any issues with the implementation so I'm going to go ahead and merge this. If anything comes up, it can always be addressed later.

cecton added 2 commits September 30, 2023 12:15

Make VNode cheap to clone

fd1187f

Faster clone for list and portal

df2d8ef

cecton added 4 commits September 30, 2023 13:20

Fixes hopefully good

a7fd660

clippy

ea8b6a7

more fixes hopefully good

56eb9c8

rustfmt

a015177

cecton added 5 commits September 30, 2023 13:39

More fixes

9f29663

more fixes...

bcd68c0

more fixes

afdf83c

Update element-fail.stderr

7717bb0

Macro fixes...

5282d96

cecton marked this pull request as ready for review September 30, 2023 13:01

cecton requested review from WorldSEnder, futursolo, voidpumpkin and ranile September 30, 2023 13:01

github-actions bot previously approved these changes Sep 30, 2023

View reviewed changes

CLEANUP

95d1438

cecton dismissed GitHub Actions’s stale review via 95d1438 September 30, 2023 13:19

github-actions bot previously approved these changes Sep 30, 2023

View reviewed changes

futursolo reviewed Oct 2, 2023

View reviewed changes

ranile reviewed Oct 2, 2023

View reviewed changes

This was referenced Oct 6, 2023

Struggling with ways to convert someting into Html. #3443

Open

remove ToHtml trait #3453

Merged

futursolo reviewed Oct 22, 2023

View reviewed changes

Use the 2 workflows approach, will fix after merge if not working

8804b0f

cecton marked this pull request as ready for review October 24, 2023 08:54

github-actions bot previously approved these changes Oct 24, 2023

View reviewed changes

cecton mentioned this pull request Oct 26, 2023

Benchmark crate for core features #3487

Merged

2 tasks

cecton added 2 commits October 27, 2023 14:26

Merge commit 1b9d294 (no conflict)

ddfef36

CLEANUP

1e8c819

cecton dismissed GitHub Actions’s stale review via 1e8c819 October 27, 2023 12:28

github-actions bot previously approved these changes Oct 27, 2023

View reviewed changes

Merge commit 22fd855 (conflicts)

72225ee

cecton dismissed GitHub Actions’s stale review via 72225ee October 27, 2023 12:30

github-actions bot previously approved these changes Oct 27, 2023

View reviewed changes

can i push that change here pretty please

5bd102b

cecton dismissed GitHub Actions’s stale review via 5bd102b October 27, 2023 12:46

github-actions bot approved these changes Oct 27, 2023

View reviewed changes

cecton added 2 commits October 27, 2023 15:53

Trigger CI

c9fe705

Merge commit ddb77bd (no conflict)

594e3d4

ranile approved these changes Oct 27, 2023

View reviewed changes

ranile merged commit 7f45af3 into yewstack:master Oct 27, 2023
21 checks passed

cecton deleted the html-cheap-to-clone branch October 31, 2023 12:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Html (VNode) cheap to clone #3431

Make Html (VNode) cheap to clone #3431

cecton commented Sep 30, 2023 •

edited

Loading

github-actions bot commented Sep 30, 2023 •

edited

Loading

github-actions bot commented Sep 30, 2023 •

edited

Loading

github-actions bot commented Sep 30, 2023 •

edited

Loading

futursolo left a comment

cecton commented Oct 2, 2023

ranile left a comment

cecton commented Oct 5, 2023

ranile commented Oct 7, 2023

cecton commented Oct 18, 2023 •

edited

Loading

futursolo commented Oct 22, 2023

futursolo Oct 22, 2023

cecton Oct 24, 2023

cecton Oct 27, 2023 •

edited

Loading

ranile Oct 27, 2023 •

edited

Loading

cecton commented Oct 24, 2023

alexkazik commented Oct 26, 2023

cecton commented Oct 26, 2023

ranile commented Oct 26, 2023

cecton commented Oct 26, 2023

ranile commented Oct 27, 2023

github-actions bot commented Oct 27, 2023 •

edited

Loading

cecton commented Oct 27, 2023

ranile left a comment •

edited

Loading

		@@ -16,25 +17,27 @@ use crate::AttrValue;
		#[must_use = "html does not do anything unless returned to Yew for rendering."]
		pub enum VNode {

Make Html (VNode) cheap to clone #3431

Make Html (VNode) cheap to clone #3431

Conversation

cecton commented Sep 30, 2023 • edited Loading

Description

Checklist

github-actions bot commented Sep 30, 2023 • edited Loading

Size Comparison

github-actions bot commented Sep 30, 2023 • edited Loading

Benchmark - SSR

Yew Master

Pull Request

github-actions bot commented Sep 30, 2023 • edited Loading

futursolo left a comment

Choose a reason for hiding this comment

cecton commented Oct 2, 2023

ranile left a comment

Choose a reason for hiding this comment

cecton commented Oct 5, 2023

ranile commented Oct 7, 2023

cecton commented Oct 18, 2023 • edited Loading

futursolo commented Oct 22, 2023

futursolo Oct 22, 2023

Choose a reason for hiding this comment

cecton Oct 24, 2023

Choose a reason for hiding this comment

cecton Oct 27, 2023 • edited Loading

Choose a reason for hiding this comment

ranile Oct 27, 2023 • edited Loading

Choose a reason for hiding this comment

cecton commented Oct 24, 2023

alexkazik commented Oct 26, 2023

cecton commented Oct 26, 2023

ranile commented Oct 26, 2023

cecton commented Oct 26, 2023

ranile commented Oct 27, 2023

github-actions bot commented Oct 27, 2023 • edited Loading

Benchmark - core

Yew Master

Pull Request" >> comment.txt

cecton commented Oct 27, 2023

ranile left a comment • edited Loading

Choose a reason for hiding this comment

cecton commented Sep 30, 2023 •

edited

Loading

github-actions bot commented Sep 30, 2023 •

edited

Loading

github-actions bot commented Sep 30, 2023 •

edited

Loading

github-actions bot commented Sep 30, 2023 •

edited

Loading

cecton commented Oct 18, 2023 •

edited

Loading

cecton Oct 27, 2023 •

edited

Loading

ranile Oct 27, 2023 •

edited

Loading

github-actions bot commented Oct 27, 2023 •

edited

Loading

ranile left a comment •

edited

Loading