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

Implement an internal DomSlot for positioning instead of NodeRef #3048

Merged
merged 22 commits into from
Jan 9, 2023

Conversation

WorldSEnder
Copy link
Member

@WorldSEnder WorldSEnder commented Dec 17, 2022

Description

A new for-internal-use data structure, DomPosition, is introduced to replace the uses of NodeRefs. This means the user-facing NodeRef does no longer have a link to a potentially long chain to follow.

A DomPosition can either point a Node (next sibling), None (the end of the children list), or a RetargetableDomPosition. The last of which is used to be able to transparently update the position "before" components, without having to explicitly know which kind of dom_bundle is its sibling.

Also introduces an explicit next_sibling for Fragment, to better track what comes after it during hydration.

Fixes #3043

Checklist

  • I have reviewed my own code
  • I have added tests

use instead of NodeRef, decoupling the two
fixes yewstack#3043
@github-actions
Copy link

github-actions bot commented Dec 17, 2022

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

https://yew-rs-api--pr3048-internal-noderefs-xjambtbj.web.app

(expires Mon, 16 Jan 2023 12:17:49 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

@github-actions
Copy link

github-actions bot commented Dec 17, 2022

Size Comparison

examples master (KB) pull request (KB) diff (KB) diff (%)
async_clock 106.552 106.551 -0.001 -0.001%
boids 170.592 170.720 +0.128 +0.075%
communication_child_to_parent 90.665 90.797 +0.132 +0.145%
communication_grandchild_with_grandparent 105.026 105.128 +0.102 +0.097%
communication_grandparent_to_grandchild 100.874 100.955 +0.081 +0.080%
communication_parent_to_child 87.760 87.895 +0.135 +0.154%
contexts 107.548 107.662 +0.114 +0.106%
counter 85.703 85.696 -0.007 -0.008%
counter_functional 86.015 86.005 -0.010 -0.011%
dyn_create_destroy_apps 88.540 88.503 -0.037 -0.042%
file_upload 100.171 100.180 +0.009 +0.009%
function_memory_game 164.676 164.850 +0.174 +0.106%
function_router 348.951 349.261 +0.310 +0.089%
function_todomvc 159.635 159.829 +0.194 +0.122%
futures 224.748 224.795 +0.047 +0.021%
game_of_life 106.443 106.457 +0.014 +0.013%
immutable 181.218 181.915 +0.697 +0.385%
inner_html 82.122 82.085 -0.037 -0.045%
js_callback 111.698 111.915 +0.217 +0.194%
keyed_list 196.127 196.262 +0.135 +0.069%
mount_point 85.265 85.321 +0.057 +0.066%
nested_list 113.266 113.357 +0.092 +0.081%
node_refs 93.183 93.336 +0.153 +0.165%
password_strength 1549.689 1549.872 +0.183 +0.012%
portals 96.470 96.523 +0.054 +0.056%
router 319.041 319.391 +0.350 +0.110%
simple_ssr 151.280 150.938 -0.343 -0.227%
ssr_router 393.427 394.442 +1.016 +0.258%
suspense 108.827 109.038 +0.211 +0.194%
timer 88.572 88.571 -0.001 -0.001%
todomvc 141.066 141.064 -0.002 -0.001%
two_apps 86.323 86.316 -0.007 -0.008%
web_worker_fib 151.872 151.860 -0.012 -0.008%
webgl 84.852 84.898 +0.047 +0.055%

✅ None of the examples has changed their size significantly.

@github-actions
Copy link

github-actions bot commented Dec 17, 2022

Benchmark - SSR

Yew Master

Benchmark Round Min (ms) Max (ms) Mean (ms) Standard Deviation
Baseline 10 376.755 377.692 377.048 0.269
Hello World 10 686.258 700.259 690.443 4.752
Function Router 10 2331.956 2355.051 2341.939 7.415
Concurrent Task 10 1007.780 1010.451 1008.539 0.764

Pull Request

Benchmark Round Min (ms) Max (ms) Mean (ms) Standard Deviation
Baseline 10 393.356 394.474 393.596 0.325
Hello World 10 678.964 683.269 679.744 1.256
Function Router 10 2342.381 2396.532 2380.700 14.266
Concurrent Task 10 1006.949 1010.569 1008.657 1.081

@WorldSEnder WorldSEnder marked this pull request as ready for review January 4, 2023 00:18
Copy link
Member

@futursolo futursolo left a comment

Choose a reason for hiding this comment

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

In general, I think this is a good change that it separates the "link" which is used internally and "RefCell-like" which is used externally.

I personally think we can eventually get away with "linking" completely if render is no longer pushed to scheduler and delayed at a later time.

packages/yew/src/dom_bundle/dom_position.rs Outdated Show resolved Hide resolved
packages/yew/src/dom_bundle/dom_position.rs Outdated Show resolved Hide resolved
packages/yew/src/dom_bundle/dom_position.rs Outdated Show resolved Hide resolved
@@ -109,14 +109,13 @@ impl std::fmt::Debug for NodeRef {
#[derive(PartialEq, Debug, Default, Clone)]
struct NodeRefInner {
node: Option<Node>,
Copy link
Member

Choose a reason for hiding this comment

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

Can we have NodeRef(Rc<RefCell<Option<Node>>>) and remove NodeRefInner?

Copy link
Member Author

Choose a reason for hiding this comment

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

With callback-refs, we would again need a NodeRefInner, so I left it, anyway.

packages/yew/src/dom_bundle/traits.rs Outdated Show resolved Hide resolved
Comment on lines 11 to 15
#[derive(Clone)]
enum DomPositionVariant {
Node(Option<Node>),
Chained(RetargetableDomPosition),
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#[derive(Clone)]
enum DomPositionVariant {
Node(Option<Node>),
Chained(RetargetableDomPosition),
}
#[derive(Clone)]
enum DomPositionVariant<'a> {
Node(Option<&'a Node>),
Chained(RetargetableDomPosition),
}

I think DomPosition is never stored so it doesn't need to live 'static.
It would be nice to have this as a reference as cloning a JsValue actually creates JavaScript calls to create a new spot on the ref array so it's relatively expensive to clone them.

Copy link
Member Author

Choose a reason for hiding this comment

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

The issue with this is that DomPosition is returned from the various ReconcileTarget::* trait methods, without a lifetime readily availabe. I'll see what can be done.

packages/yew/src/dom_bundle/dom_position.rs Outdated Show resolved Hide resolved
github-actions[bot]
github-actions bot previously approved these changes Jan 4, 2023
github-actions[bot]
github-actions bot previously approved these changes Jan 4, 2023
@WorldSEnder WorldSEnder changed the title Implement an internal DomPosition Implement an internal DomSlot for positioning instead of NodeRef Jan 7, 2023
packages/yew/src/dom_bundle/fragment.rs Outdated Show resolved Hide resolved
packages/yew/src/dom_bundle/dom_position.rs Outdated Show resolved Hide resolved
packages/yew/src/dom_bundle/dom_position.rs Outdated Show resolved Hide resolved
packages/yew/src/dom_bundle/dom_position.rs Outdated Show resolved Hide resolved
rename retarget -> reassign
github-actions[bot]
github-actions bot previously approved these changes Jan 9, 2023
futursolo
futursolo previously approved these changes Jan 9, 2023
Copy link
Member

@futursolo futursolo left a comment

Choose a reason for hiding this comment

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

Approving this as I do not think there are other issues other than naming, which is not blocking to this pull request.

packages/yew/src/dom_bundle/mod.rs Outdated Show resolved Hide resolved
@futursolo
Copy link
Member

LGTM.

@WorldSEnder WorldSEnder merged commit c5ffe60 into yewstack:master Jan 9, 2023
@WorldSEnder WorldSEnder deleted the internal-noderefs branch January 9, 2023 14:08
@WorldSEnder WorldSEnder added the A-yew Area: The main yew crate label Jan 9, 2023
WorldSEnder added a commit to WorldSEnder/yew that referenced this pull request Jan 10, 2023
…stack#3048)

use instead of NodeRef, decoupling the two
fixes yewstack#3043

* implement internal DomSlot
* move DomSlot into submodule of dom_bundle
* hide behind feature csr
* add test cases
* write get in continuation style, this saves a clone
* private DomSlot::get
WorldSEnder added a commit that referenced this pull request Jan 19, 2023
use instead of NodeRef, decoupling the two
fixes #3043

* implement internal DomSlot
* move DomSlot into submodule of dom_bundle
* hide behind feature csr
* add test cases
* write get in continuation style, this saves a clone
* private DomSlot::get
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-yew Area: The main yew crate bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Uncaught RangeError: Maximum call stack size exceeded
2 participants