From ba40f643ba83679e4513bd8c822dcee1ca583a7f Mon Sep 17 00:00:00 2001 From: David Husicka Date: Wed, 7 Aug 2024 16:46:50 +0200 Subject: [PATCH 1/4] Reuse anonymous blocks for complex inline layouts This gets rid of a memory leak where the anonymous block would be created in a loop and never cleared. --- packages/dom/src/document.rs | 16 +++++++++++++++- packages/dom/src/layout/construct.rs | 13 +++++++++++++ packages/dom/src/node.rs | 4 ++++ 3 files changed, 32 insertions(+), 1 deletion(-) diff --git a/packages/dom/src/document.rs b/packages/dom/src/document.rs index 0f54c177c..b57c756ee 100644 --- a/packages/dom/src/document.rs +++ b/packages/dom/src/document.rs @@ -383,6 +383,7 @@ impl Document { if let Some(Node { mut child_idx, parent: Some(parent_id), + anonymous_block_id, .. }) = node { @@ -399,7 +400,20 @@ impl Document { child_idx += 1; } - self.nodes[parent_id].children = children; + // remove unneeded anonymous blocks + if let Some(anonymous_block_id) = anonymous_block_id { + let is_block_used = !children.iter().all(|c| { + let child = self.get_node(*c); + matches!(child, Some(child) if child.anonymous_block_id != Some(anonymous_block_id)) + }); + + self.nodes[parent_id].children = children; + if !is_block_used { + self.remove_node(anonymous_block_id); + } + } else { + self.nodes[parent_id].children = children; + } } node diff --git a/packages/dom/src/layout/construct.rs b/packages/dom/src/layout/construct.rs index bf62b0a3a..63897c93f 100644 --- a/packages/dom/src/layout/construct.rs +++ b/packages/dom/src/layout/construct.rs @@ -235,6 +235,13 @@ fn collect_complex_layout_children( else if needs_wrap(child_node_kind, display_outside) { use style::selector_parser::PseudoElement; + // TODO: once let chaining lends in stable, make nicer + if anonymous_block_id.is_none() { + if let Some(child) = doc.get_node(child_id) { + *anonymous_block_id = child.anonymous_block_id; + } + } + if anonymous_block_id.is_none() { const NAME: QualName = QualName { prefix: None, @@ -246,6 +253,9 @@ fn collect_complex_layout_children( Vec::new(), ))); + let child = doc.get_node_mut(child_id).unwrap(); + child.anonymous_block_id = Some(node_id); + // Set style data let parent_style = doc.nodes[container_node_id].primary_styles().unwrap(); let read_guard = doc.guard.read(); @@ -262,6 +272,9 @@ fn collect_complex_layout_children( layout_children.push(node_id); *anonymous_block_id = Some(node_id); + + #[cfg(feature = "tracing")] + tracing::info!("Created anonymous block container with id {}", node_id); } doc.nodes[anonymous_block_id.unwrap()] diff --git a/packages/dom/src/node.rs b/packages/dom/src/node.rs index 36c657b46..c431e392d 100644 --- a/packages/dom/src/node.rs +++ b/packages/dom/src/node.rs @@ -53,6 +53,9 @@ pub struct Node { pub child_idx: usize, // What are our children? pub children: Vec, + // What anonymous block are we a part of + // Changing order of nodes requires unsetting + pub anonymous_block_id: Option, /// A separate child list that includes anonymous collections of inline elements pub layout_children: RefCell>>, @@ -90,6 +93,7 @@ impl Node { id, parent: None, children: vec![], + anonymous_block_id: None, layout_children: RefCell::new(None), child_idx: 0, From 88fb76283a75aa4f7933f393b3959506a0a7b634 Mon Sep 17 00:00:00 2001 From: David Husicka Date: Wed, 7 Aug 2024 21:11:14 +0200 Subject: [PATCH 2/4] Invalidate anonymous blocks when document changes --- packages/dom/src/document.rs | 18 ++++++++++++++++++ packages/dom/src/node.rs | 1 + 2 files changed, 19 insertions(+) diff --git a/packages/dom/src/document.rs b/packages/dom/src/document.rs index b57c756ee..b482bdeb9 100644 --- a/packages/dom/src/document.rs +++ b/packages/dom/src/document.rs @@ -310,11 +310,27 @@ impl Document { new_node_id } + /// Used for cleaning invalid anonymous blocks + fn clean_anonymous_blocks(&mut self, node_id: usize) { + let children = &mut self.nodes[node_id].children.clone(); + for child_id in children.iter() { + self.clean_anonymous_blocks(*child_id); + } + + let node = &mut self.nodes[node_id]; + node.anonymous_block_id = None; + if let Some(anonymous_block_id) = node.anonymous_block_id { + // Removing does pattern matching so removing the same node multiple times is ok + self.remove_node(anonymous_block_id); + } + } + pub fn insert_before(&mut self, node_id: usize, inserted_node_ids: &[usize]) { // let count = inserted_node_ids.len(); // self.print_tree(); + self.clean_anonymous_blocks(node_id); let node = &self.nodes[node_id]; let node_child_idx = node.child_idx; @@ -344,6 +360,8 @@ impl Document { } pub fn append(&mut self, node_id: usize, appended_node_ids: &[usize]) { + self.clean_anonymous_blocks(node_id); + let node = &self.nodes[node_id]; // let node_child_idx = node.child_idx; let parent_id = node.parent.unwrap(); diff --git a/packages/dom/src/node.rs b/packages/dom/src/node.rs index c431e392d..fc56ea5a7 100644 --- a/packages/dom/src/node.rs +++ b/packages/dom/src/node.rs @@ -55,6 +55,7 @@ pub struct Node { pub children: Vec, // What anonymous block are we a part of // Changing order of nodes requires unsetting + // using the "clean_anonymous_blocks" method with parent_id pub anonymous_block_id: Option, /// A separate child list that includes anonymous collections of inline elements pub layout_children: RefCell>>, From 9bef52c05d1ee7c48fcfbff9c5e5c999d8abadb6 Mon Sep 17 00:00:00 2001 From: David Husicka Date: Wed, 7 Aug 2024 21:17:29 +0200 Subject: [PATCH 3/4] Change name of a single letter variable to better represent it's meaning and make the predicate function more clear --- packages/dom/src/document.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/dom/src/document.rs b/packages/dom/src/document.rs index b482bdeb9..f9afaf217 100644 --- a/packages/dom/src/document.rs +++ b/packages/dom/src/document.rs @@ -420,9 +420,9 @@ impl Document { // remove unneeded anonymous blocks if let Some(anonymous_block_id) = anonymous_block_id { - let is_block_used = !children.iter().all(|c| { - let child = self.get_node(*c); - matches!(child, Some(child) if child.anonymous_block_id != Some(anonymous_block_id)) + let is_block_used = children.iter().any(|child_id| { + let child = self.get_node(*child_id); + matches!(child, Some(child) if child.anonymous_block_id == Some(anonymous_block_id)) }); self.nodes[parent_id].children = children; From a50744b991d23d36861d5bac3dbee73961cb80ee Mon Sep 17 00:00:00 2001 From: David Husicka Date: Wed, 7 Aug 2024 21:55:15 +0200 Subject: [PATCH 4/4] Use parent_id as stated in the comment regarding anonymous block unsetting --- packages/dom/src/document.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/dom/src/document.rs b/packages/dom/src/document.rs index f9afaf217..e0cf88b7f 100644 --- a/packages/dom/src/document.rs +++ b/packages/dom/src/document.rs @@ -330,11 +330,11 @@ impl Document { // self.print_tree(); - self.clean_anonymous_blocks(node_id); let node = &self.nodes[node_id]; let node_child_idx = node.child_idx; let parent_id = node.parent.unwrap(); + self.clean_anonymous_blocks(parent_id); let parent = &mut self.nodes[parent_id]; // Mark the node's parent as changed. @@ -360,11 +360,10 @@ impl Document { } pub fn append(&mut self, node_id: usize, appended_node_ids: &[usize]) { - self.clean_anonymous_blocks(node_id); - let node = &self.nodes[node_id]; // let node_child_idx = node.child_idx; let parent_id = node.parent.unwrap(); + self.clean_anonymous_blocks(parent_id); let parent = &mut self.nodes[parent_id]; let mut children = std::mem::take(&mut parent.children);