Make VList's children Rc'ed #3050
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Visit the preview URL for this PR (updated for commit f36a4f1): https://yew-rs-api--pr3050-rc-vlist-5ng46tcw.web.app (expires Sun, 09 Apr 2023 08:51:49 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 |
Benchmark - SSRYew Master
Pull Request
|
Size Comparison
|
|
Looks like function_router is not a good example for benchmarking bundle size for this particular pull request. |
This reverts commit 3ca55be.
|
Twiggy: The size increase is due to that rustc can no longer determine whether This pull request is optimised for applications that actually uses I have added a Many Providers test which does not show too much of a difference in this pull request, But with slight modification with the benchmark adapted for #3042, the performance would increase about 20%. I think the performance increase might worth the bundle size tradeoff in this case? |
|
Would work for me, if PR gets updated. |
| @@ -14,7 +15,7 @@ enum FullyKeyedState { | |||
| #[derive(Clone, Debug)] | |||
| pub struct VList { | |||
| /// The list of child [VNode]s | |||
| pub(crate) children: Vec<VNode>, | |||
| pub(crate) children: Option<Rc<Vec<VNode>>>, | |||
There was a problem hiding this comment.
Maybe that's not relevant but I was thinking of using IArray would help here?
I see 2 possible values but I have not investigated in details:
- IArray uses
Rc<[T]>instead ofRc<Vec<T>>. I guess that would save an allocation somewhere. - IArray has a variant
Static(&'static [T]), it can maybe used in place ofNonehere
There was a problem hiding this comment.
(The only issue is the hard requirement of VNode to implement ImplicitClone but I think we can allow that because in the end this would be desirable, see discussion #3022)
There was a problem hiding this comment.
I think VList implements DerefMut<Target = Vec<VNode>> which allows manipulation without cloning the entire array when the reference count is 1 in which we may not be able to keep this behaviour with IArray if it uses slices.
There was a problem hiding this comment.
Now that you mention it, I think I had issues when converting the code because of that haha
| match self.children { | ||
| Some(ref m) => m, | ||
| None => { | ||
| // This is mutable because the Vec<VNode> is not Sync | ||
| static mut EMPTY: Vec<VNode> = Vec::new(); | ||
| // SAFETY: The EMPTY value is always read-only | ||
| unsafe { &EMPTY } | ||
| } | ||
| } |
There was a problem hiding this comment.
This looks a bit tricky overall. Is it possible to achieve this while using safety?
I'm also not sure I understand why we need an Option around the children type. Probably it is related to this whole thing.
Can't we have a default static value like this?
static EMPTY: Rc<Vec<VNode>> = Rc::new(Vec::new())
(Maybe a thread-local one but you see the idea)
There was a problem hiding this comment.
Since VNode is !Send, this makes a global static EMPTY: &Vec<VNode>= &Vec::new() inaccessible from all threads. (Rust will not compile this anyways...)
If we use thread_local!, this variable will only live for the period of the thread, not the period of the program, hence not 'static. This means that Rust cannot guarantee the reference will outlive the ownership. (Which is not quite right, since !Send types cannot live longer than the thread it resides on. So this is technically 'static for them.) In this case, I think the only way is to teach the Rust compiler a lesson with some additional knowledge.
However, we can still do it with safe Rust by leaking the memory.
If we don't have SSR, this might be the preferred method over unsafe since there is only 1 thread. But we also have SSR these days which will cause leaked memory for each thread ever created.
thread_local! {
static EMPTY: &'static Vec<VNode> = Box::leak(Box::default());
}
EMPTY.with(|m| *m)If there is a way to achieve this with safe Rust, I am also very curious to know.
There was a problem hiding this comment.
Maybe a stupid question but... do we actually need to impl Deref and DerefMut on VList??
The only use in its own crate is for this and it's kinda non-brainer to work around:
diff --git a/packages/yew/src/virtual_dom/vlist.rs b/packages/yew/src/virtual_dom/vlist.rs
index fc211ce9..40c94d5d 100644
--- a/packages/yew/src/virtual_dom/vlist.rs
+++ b/packages/yew/src/virtual_dom/vlist.rs
@@ -43,6 +43,7 @@ impl Default for VList {
}
}
+/*
impl Deref for VList {
type Target = Vec<VNode>;
@@ -58,13 +59,16 @@ impl Deref for VList {
}
}
}
+*/
+/*
impl DerefMut for VList {
fn deref_mut(&mut self) -> &mut Self::Target {
self.fully_keyed = FullyKeyedState::Unknown;
self.children_mut()
}
}
+*/
impl VList {
/// Creates a new empty [VList] instance.
@@ -135,7 +139,7 @@ impl VList {
match self.fully_keyed {
FullyKeyedState::KnownFullyKeyed => true,
FullyKeyedState::KnownMissingKeys => false,
- FullyKeyedState::Unknown => self.iter().all(|c| c.has_key()),
+ FullyKeyedState::Unknown => self.children.iter().flat_map(|x| x.iter()).all(|c| c.has_key()),
}
}
}There was a problem hiding this comment.
I guess the original intention for implementing VList with dereferencing to Vec<VNode> is to allow users use all methods available to a vector to CURD VLists.
E.g.: In bounce, I used VList as a Vec to recursively read all available html to be used with the <head /> element.
https://github.com/bounce-rs/bounce/blob/master/crates/bounce/src/helmet/comp.rs#L27
I didn't use any mutable operations in bounce helmet, but I can imagine a use case where manipulates VList would exist.
| pub const fn new() -> Self { | ||
| Self { | ||
| children: Vec::new(), | ||
| children: None, |
There was a problem hiding this comment.
(To answer my question earlier: why do we need an option around it? It's because we want to make a const fn constructor.)
There was a problem hiding this comment.
In addition, Rc::new() also results in an allocation, even if the vector is empty. :)
Description
Separated from #3042 to address bundle size issue.
This pull request makes VList to store children with
Option<Rc<Vec<VNode>>>.This increases the performance of
{self.children.clone()}, which increases for about ~5% when running the benchmark locally with SSR Benchmark based on thefunction_routerexample. However, since we do not have a benchmark that depends heavily on cloningchildren, the performance increase might not be fully demonstrated.But since
ChildrenRenderer<VNode>stores VNode in aVec<VNode>, it still involves an allocation every time children is passed.This will be addressed in #3042, which should give a further performance increase.
This is also not a breaking change.
Checklist