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

Fix leak in core because of bump allocated Vec #1364

Merged
merged 3 commits into from Aug 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion docs/guide/examples/readme_expanded.rs
Expand Up @@ -89,7 +89,8 @@ fn app(cx: Scope) -> Element {
key: None,
// The static template this node will use. The template is stored in a Cell so it can be replaced with a new template when hot rsx reloading is enabled
template: std::cell::Cell::new(TEMPLATE),
root_ids: Default::default(),
root_ids: dioxus::core::exports::bumpalo::collections::Vec::new_in(__cx.bump())
.into(),
dynamic_nodes: __cx.bump().alloc([
// The dynamic count text node (dynamic node id 0)
__cx.text_node(format_args!("High-Five counter: {0}", count)),
Expand Down
8 changes: 6 additions & 2 deletions packages/core/src/create.rs
Expand Up @@ -87,8 +87,12 @@ impl<'b> VirtualDom {
}
}

// Intialize the root nodes slice
*node.root_ids.borrow_mut() = vec![ElementId(0); node.template.get().roots.len()];
// Initialize the root nodes slice
{
let mut nodes_mut = node.root_ids.borrow_mut();
let len = node.template.get().roots.len();
nodes_mut.resize(len, ElementId::default());
};

// The best renderers will have templates prehydrated and registered
// Just in case, let's create the template using instructions anyways
Expand Down
8 changes: 7 additions & 1 deletion packages/core/src/diff.rs
Expand Up @@ -126,7 +126,13 @@ impl<'b> VirtualDom {
});

// Make sure the roots get transferred over while we're here
*right_template.root_ids.borrow_mut() = left_template.root_ids.borrow().clone();
{
let mut right = right_template.root_ids.borrow_mut();
right.clear();
for &element in left_template.root_ids.borrow().iter() {
right.push(element);
}
}

let root_ids = right_template.root_ids.borrow();

Expand Down
14 changes: 7 additions & 7 deletions packages/core/src/nodes.rs
Expand Up @@ -54,7 +54,7 @@ pub struct VNode<'a> {

/// The IDs for the roots of this template - to be used when moving the template around and removing it from
/// the actual Dom
pub root_ids: RefCell<Vec<ElementId>>,
pub root_ids: RefCell<bumpalo::collections::Vec<'a, ElementId>>,

/// The dynamic parts of the template
pub dynamic_nodes: &'a [DynamicNode<'a>],
Expand All @@ -65,11 +65,11 @@ pub struct VNode<'a> {

impl<'a> VNode<'a> {
/// Create a template with no nodes that will be skipped over during diffing
pub fn empty() -> Element<'a> {
pub fn empty(cx: &'a ScopeState) -> Element<'a> {
Some(VNode {
key: None,
parent: None,
root_ids: Default::default(),
root_ids: RefCell::new(bumpalo::collections::Vec::new_in(cx.bump())),
dynamic_nodes: &[],
dynamic_attrs: &[],
template: Cell::new(Template {
Expand Down Expand Up @@ -698,7 +698,7 @@ impl<'a, 'b> IntoDynNode<'a> for LazyNodes<'a, 'b> {
impl<'a, 'b> IntoDynNode<'b> for &'a str {
fn into_vnode(self, cx: &'b ScopeState) -> DynamicNode<'b> {
DynamicNode::Text(VText {
value: bumpalo::collections::String::from_str_in(self, cx.bump()).into_bump_str(),
value: cx.bump().alloc_str(self),
id: Default::default(),
})
}
Expand Down Expand Up @@ -741,10 +741,10 @@ impl<'a> IntoTemplate<'a> for VNode<'a> {
}
}
impl<'a> IntoTemplate<'a> for Element<'a> {
fn into_template(self, _cx: &'a ScopeState) -> VNode<'a> {
fn into_template(self, cx: &'a ScopeState) -> VNode<'a> {
match self {
Some(val) => val.into_template(_cx),
_ => VNode::empty().unwrap(),
Some(val) => val.into_template(cx),
_ => VNode::empty(cx).unwrap(),
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions packages/core/tests/fuzzing.rs
Expand Up @@ -179,7 +179,7 @@ fn create_random_dynamic_node(cx: &ScopeState, depth: usize) -> DynamicNode {
node_paths: &[&[0]],
attr_paths: &[],
}),
root_ids: Default::default(),
root_ids: bumpalo::collections::Vec::new_in(cx.bump()).into(),
dynamic_nodes: cx.bump().alloc([cx.component(
create_random_element,
DepthProps { depth, root: false },
Expand Down Expand Up @@ -276,7 +276,7 @@ fn create_random_element(cx: Scope<DepthProps>) -> Element {
key: None,
parent: None,
template: Cell::new(template),
root_ids: Default::default(),
root_ids: bumpalo::collections::Vec::new_in(cx.bump()).into(),
dynamic_nodes: {
let dynamic_nodes: Vec<_> = dynamic_node_types
.iter()
Expand Down
5 changes: 3 additions & 2 deletions packages/native-core/tests/fuzzing.rs
Expand Up @@ -187,7 +187,7 @@ fn create_random_dynamic_node(cx: &ScopeState, depth: usize) -> DynamicNode {
node_paths: &[&[0]],
attr_paths: &[],
}),
root_ids: Default::default(),
root_ids: dioxus::core::exports::bumpalo::collections::Vec::new_in(cx.bump()).into(),
dynamic_nodes: cx.bump().alloc([cx.component(
create_random_element,
DepthProps { depth, root: false },
Expand Down Expand Up @@ -257,7 +257,8 @@ fn create_random_element(cx: Scope<DepthProps>) -> Element {
key: None,
parent: None,
template: Cell::new(template),
root_ids: Default::default(),
root_ids: dioxus::core::exports::bumpalo::collections::Vec::new_in(cx.bump())
.into(),
dynamic_nodes: {
let dynamic_nodes: Vec<_> = dynamic_node_types
.iter()
Expand Down
3 changes: 2 additions & 1 deletion packages/rsx/src/lib.rs
Expand Up @@ -231,6 +231,7 @@ impl<'a> ToTokens for TemplateRenderer<'a> {

// Render and release the mutable borrow on context
let roots = quote! { #( #root_printer ),* };
let root_count = self.roots.len();
let node_printer = &context.dynamic_nodes;
let dyn_attr_printer = &context.dynamic_attributes;
let node_paths = context.node_paths.iter().map(|it| quote!(&[#(#it),*]));
Expand All @@ -247,7 +248,7 @@ impl<'a> ToTokens for TemplateRenderer<'a> {
parent: None,
key: #key_tokens,
template: std::cell::Cell::new(TEMPLATE),
root_ids: Default::default(),
root_ids: dioxus::core::exports::bumpalo::collections::Vec::with_capacity_in(#root_count, __cx.bump()).into(),
dynamic_nodes: __cx.bump().alloc([ #( #node_printer ),* ]),
dynamic_attrs: __cx.bump().alloc([ #( #dyn_attr_printer ),* ]),
}
Expand Down