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.

Sign up for GitHub

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

Jump to bottom

yew/vlist: optimize diffing and patching #1555

Merged
merged 20 commits into from Jul 18, 2021
Merged

yew/vlist: optimize diffing and patching #1555

merged 20 commits into from Jul 18, 2021

Conversation

bakape
Copy link
Contributor

@bakape bakape commented Sep 13, 2020 •
edited

Description

  • Optimized keyed VList patching for node addition or removal from either end of the list and localized changes in the middle
  • Changed unkeyed VList patching algorithm to much cheaper reverse vector itteration
  • Added opt-in more verbose test logging
  • Improved Debug output of some types

Checklist:

  • I have run ./ci/run_stable_checks.sh
  • I have reviewed my own code
  • I have added tests

@bakape
Copy link
Contributor Author

bakape commented Sep 18, 2020

yew-functional CI tests seem to have failed due to chromedriver.

@jstarry
Copy link
Member

jstarry commented Sep 19, 2020

@bakape perf looks good for 7b00d04e but not for 0c8a2715. Do you mind taking a look?

7b00d04e

result yew-05_swap1k.json min 49.149 max 52.446 mean 50.693000000000005 median 50.484 stddev 1.6584067655433616
result yew-baseline-05_swap1k.json min 49.25 max 52.606 mean 51.334 median 52.146 stddev 1.8193933054730091

0c8a2715

result yew-05_swap1k.json min 411.713 max 443.661 mean 429.0826666666667 median 431.874 stddev 16.155876093029836
result yew-baseline-05_swap1k.json min 51.105 max 60.054 mean 56.18866666666667 median 57.407 stddev 4.597216802950819

@jstarry
Copy link
Member

jstarry commented Sep 19, 2020

Also, I fixed the macro tests on master fyi

@bakape
Copy link
Contributor Author

bakape commented Oct 5, 2020

The regression was introduced in e6c965b, which is very odd. I don't see anything there that could have caused it. I'll rebase and merge master anew.

@bakape
Copy link
Contributor Author

bakape commented Nov 19, 2020

@jstarry Sorry, I've been busy with other matters and projects for a while. I've fixed the swap issue and added a few other optimizations.

Screenshot_20201119_033133

@jstarry
Copy link
Member

jstarry commented Nov 22, 2020

No worries, I've been busy as well. Will review soon

@bakape
Copy link
Contributor Author

bakape commented May 23, 2021

@siku2
Merged master and reran benchmarks using bakape/js-framework-benchmark
Screenshot_20210523_205748

@jstarry
yewstack/js-framework-benchmark is currently broken, so the action will fail. My PR should fix that.

@bakape bakape mentioned this pull request Jul 8, 2021
Merged
3 tasks
@cecton
Copy link
Member

cecton commented Jul 12, 2021

I'm looking forward to see this merged \o/

@bakape
Copy link
Contributor Author

bakape commented Jul 12, 2021

It's been forgotten, it seems. A lot to review, so understandable finding time might be hard.

teymour-aldridge
teymour-aldridge reviewed Jul 12, 2021
View reviewed changes
Copy link
Contributor

@teymour-aldridge teymour-aldridge left a comment

Choose a reason for hiding this comment

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

Just some thoughts

packages/yew/src/utils.rs Outdated Show resolved Hide resolved
packages/yew/src/virtual_dom/vlist.rs
) -> NodeRef {
use std::mem::{transmute, MaybeUninit};

macro_rules! map_keys {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure about the benefit of using a macro here (especially with an eye towards compile times).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It saves us from copy-pasting and I think the compile costs are negligible. Especially so, with how much the html! procedural macro does.

packages/yew/src/virtual_dom/vnode.rs Outdated Show resolved Hide resolved
@bakape @teymour-aldridge @cecton
Apply suggestions from code review
7b8761a 
Co-authored-by: Teymour Aldridge <notifications@reasoning.page>
Co-authored-by: Cecile Tonglet <cecile.tonglet@cecton.com>
hamza1311
hamza1311 reviewed Jul 18, 2021
View reviewed changes
Copy link
Member

@hamza1311 hamza1311 left a comment

Choose a reason for hiding this comment

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

Apart from this comment, this looks good to me.

packages/yew/src/virtual_dom/vtext.rs
"VText {{ text: \"{}\", reference: {} }}",
self.text,
match &self.reference {
Some(_) => "Some(...)",
Copy link
Member

Choose a reason for hiding this comment

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

Nit: maybe also print the VText's content? Or is that not possible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was intentional, as it made the debug log output much more unreadable. Especially so in already verbose log output, like the one I added to the layout tests.

packages/yew/src/virtual_dom/mod.rs Outdated Show resolved Hide resolved
@siku2 siku2 merged commit f2504f4 into yewstack:master Jul 18, 2021
@voidpumpkin voidpumpkin added the A-yew Area: The main yew crate label Nov 26, 2021
@WorldSEnder WorldSEnder mentioned this pull request Jan 4, 2022
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cecton cecton cecton left review comments

@teymour-aldridge teymour-aldridge teymour-aldridge left review comments

@hamza1311 hamza1311 hamza1311 left review comments

Assignees
No one assigned
Labels
A-yew Area: The main yew crate performance
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@bakape @jstarry @cecton @teymour-aldridge @hamza1311 @siku2 @voidpumpkin