-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Make VList's children Rc'ed #3050
Changes from 9 commits
50afc9d
71cf2a1
1f737f2
f372e00
e7783c8
ac1d9fd
311284a
3ca55be
128ed42
d21b9d1
f3d4c7f
6226508
98f9909
4555f15
a0a1b58
f36a4f1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
//! This module contains fragments implementation. | ||
use std::ops::{Deref, DerefMut}; | ||
use std::rc::Rc; | ||
|
||
use super::{Key, VNode}; | ||
|
||
|
@@ -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>>>, | ||
|
||
/// All [VNode]s in the VList have keys | ||
fully_keyed: FullyKeyedState, | ||
|
@@ -24,7 +25,15 @@ pub struct VList { | |
|
||
impl PartialEq for VList { | ||
fn eq(&self, other: &Self) -> bool { | ||
self.children == other.children && self.key == other.key | ||
self.key == other.key | ||
&& match (self.children.as_ref(), other.children.as_ref()) { | ||
// We try to use ptr_eq if both are behind Rc, | ||
// Somehow VNode is not Eq? | ||
(Some(l), Some(r)) if Rc::ptr_eq(l, r) => true, | ||
// We fallback to PartialEq if left and right didn't point to the same memory | ||
// address. | ||
(l, r) => l == r, | ||
} | ||
} | ||
} | ||
|
||
|
@@ -38,22 +47,30 @@ impl Deref for VList { | |
type Target = Vec<VNode>; | ||
|
||
fn deref(&self) -> &Self::Target { | ||
&self.children | ||
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 } | ||
} | ||
} | ||
Comment on lines
+50
to
+58
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
(Maybe a thread-local one but you see the idea) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since VNode is If we use However, we can still do it with safe Rust by leaking the memory. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess the original intention for implementing VList with dereferencing to E.g.: In bounce, I used VList as a Vec to recursively read all available html to be used with the |
||
} | ||
} | ||
|
||
impl DerefMut for VList { | ||
fn deref_mut(&mut self) -> &mut Self::Target { | ||
self.fully_keyed = FullyKeyedState::Unknown; | ||
&mut self.children | ||
self.children_mut() | ||
} | ||
} | ||
|
||
impl VList { | ||
/// Creates a new empty [VList] instance. | ||
pub const fn new() -> Self { | ||
Self { | ||
children: Vec::new(), | ||
children: None, | ||
Comment on lines
71
to
+73
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In addition, |
||
key: None, | ||
fully_keyed: FullyKeyedState::KnownFullyKeyed, | ||
} | ||
|
@@ -63,7 +80,7 @@ impl VList { | |
pub fn with_children(children: Vec<VNode>, key: Option<Key>) -> Self { | ||
let mut vlist = VList { | ||
fully_keyed: FullyKeyedState::Unknown, | ||
children, | ||
children: Some(Rc::new(children)), | ||
key, | ||
}; | ||
vlist.fully_keyed = if vlist.fully_keyed() { | ||
|
@@ -74,19 +91,31 @@ impl VList { | |
vlist | ||
} | ||
|
||
#[inline(never)] | ||
pub fn children_mut(&mut self) -> &mut Vec<VNode> { | ||
loop { | ||
match self.children { | ||
Some(ref mut m) => return Rc::make_mut(m), | ||
None => { | ||
self.children = Some(Rc::new(Vec::new())); | ||
} | ||
} | ||
} | ||
} | ||
|
||
/// Add [VNode] child. | ||
pub fn add_child(&mut self, child: VNode) { | ||
if self.fully_keyed == FullyKeyedState::KnownFullyKeyed && !child.has_key() { | ||
self.fully_keyed = FullyKeyedState::KnownMissingKeys; | ||
} | ||
self.children.push(child); | ||
self.children_mut().push(child); | ||
} | ||
|
||
/// Add multiple [VNode] children. | ||
pub fn add_children(&mut self, children: impl IntoIterator<Item = VNode>) { | ||
let it = children.into_iter(); | ||
let bound = it.size_hint(); | ||
self.children.reserve(bound.1.unwrap_or(bound.0)); | ||
self.children_mut().reserve(bound.1.unwrap_or(bound.0)); | ||
for ch in it { | ||
self.add_child(ch); | ||
} | ||
|
@@ -108,7 +137,7 @@ impl VList { | |
match self.fully_keyed { | ||
FullyKeyedState::KnownFullyKeyed => true, | ||
FullyKeyedState::KnownMissingKeys => false, | ||
FullyKeyedState::Unknown => self.children.iter().all(|c| c.has_key()), | ||
FullyKeyedState::Unknown => self.iter().all(|c| c.has_key()), | ||
} | ||
} | ||
} | ||
|
@@ -173,7 +202,7 @@ mod feat_ssr { | |
parent_scope: &AnyScope, | ||
hydratable: bool, | ||
) { | ||
match &self.children[..] { | ||
match &self[..] { | ||
[] => {} | ||
[child] => { | ||
child.render_into_stream(w, parent_scope, hydratable).await; | ||
|
@@ -237,7 +266,7 @@ mod feat_ssr { | |
} | ||
} | ||
|
||
let children = self.children.iter(); | ||
let children = self.iter(); | ||
render_child_iter(children, w, parent_scope, hydratable).await; | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
Rc<[T]>
instead ofRc<Vec<T>>
. I guess that would save an allocation somewhere.Static(&'static [T])
, it can maybe used in place ofNone
hereThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think
VList
implementsDerefMut<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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that you mention it, I think I had issues when converting the code because of that haha