Skip to content

Commit

Permalink
Merge pull request #1364 from Demonthos/fix-core-leak
Browse files Browse the repository at this point in the history
Fix leak in core because of bump allocated Vec
  • Loading branch information
jkelleyrtp authored Aug 15, 2023
2 parents fee206a + 6876d2d commit a2df9c2
Show file tree
Hide file tree
Showing 7 changed files with 29 additions and 16 deletions.
3 changes: 2 additions & 1 deletion docs/guide/examples/readme_expanded.rs
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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

0 comments on commit a2df9c2

Please sign in to comment.