From c65cd1eadd635ec4cfd10cefb4536c200295d079 Mon Sep 17 00:00:00 2001 From: Jonathan Schwender <55576758+jschwe@users.noreply.github.com> Date: Sat, 5 Jul 2025 11:45:58 +0800 Subject: [PATCH] Revert "layout: Add incremental box tree construction for inline floats and abspos" (#37888) This reverts commit 19ceccc8eb983bd87e3f8fca97d61c6dde87b4e7 due to a significant increase in resident memory usage (See https://github.com/servo/servo/issues/37887). Testing: This is a revert due to a regression Signed-off-by: Jonathan Schwender --- components/layout/dom.rs | 14 ------ components/layout/flow/construct.rs | 28 +++++------- components/layout/flow/inline/construct.rs | 52 ++++++++-------------- 3 files changed, 28 insertions(+), 66 deletions(-) diff --git a/components/layout/dom.rs b/components/layout/dom.rs index 124bc9269b6..40023439982 100644 --- a/components/layout/dom.rs +++ b/components/layout/dom.rs @@ -136,20 +136,6 @@ impl LayoutBox { .repair_style(context, node, new_style), } } - - /// If this [`LayoutBox`] represents an unsplit (due to inline-block splits) inline - /// level item, unwrap and return it. If not, return `None`. - pub(crate) fn unsplit_inline_level_layout_box(self) -> Option> { - let LayoutBox::InlineLevel(inline_level_boxes) = self else { - return None; - }; - // If this element box has been subject to inline-block splitting, ignore it. It's - // not useful currently for incremental box tree construction. - if inline_level_boxes.len() != 1 { - return None; - } - inline_level_boxes.into_iter().next() - } } /// A wrapper for [`InnerDOMLayoutData`]. This is necessary to give the entire data diff --git a/components/layout/flow/construct.rs b/components/layout/flow/construct.rs index 024122823f3..48493eceade 100644 --- a/components/layout/flow/construct.rs +++ b/components/layout/flow/construct.rs @@ -604,17 +604,13 @@ impl<'dom> BlockContainerBuilder<'dom, '_> { ) { if let Some(builder) = self.inline_formatting_context_builder.as_mut() { if !builder.is_empty() { - let constructor = || { - ArcRefCell::new(AbsolutelyPositionedBox::construct( + let inline_level_box = + builder.push_absolutely_positioned_box(AbsolutelyPositionedBox::construct( self.context, info, display_inside, contents, - )) - }; - let old_layout_box = box_slot.take_layout_box_if_undamaged(info.damage); - let inline_level_box = - builder.push_absolutely_positioned_box(constructor, old_layout_box); + )); box_slot.set(LayoutBox::InlineLevel(vec![inline_level_box])); return; } @@ -641,17 +637,13 @@ impl<'dom> BlockContainerBuilder<'dom, '_> { ) { if let Some(builder) = self.inline_formatting_context_builder.as_mut() { if !builder.is_empty() { - let constructor = || { - ArcRefCell::new(FloatBox::construct( - self.context, - info, - display_inside, - contents, - self.propagated_data, - )) - }; - let old_layout_box = box_slot.take_layout_box_if_undamaged(info.damage); - let inline_level_box = builder.push_float_box(constructor, old_layout_box); + let inline_level_box = builder.push_float_box(FloatBox::construct( + self.context, + info, + display_inside, + contents, + self.propagated_data, + )); box_slot.set(LayoutBox::InlineLevel(vec![inline_level_box])); return; } diff --git a/components/layout/flow/inline/construct.rs b/components/layout/flow/inline/construct.rs index b7c124819b8..1b4fe869416 100644 --- a/components/layout/flow/inline/construct.rs +++ b/components/layout/flow/inline/construct.rs @@ -160,10 +160,19 @@ impl InlineFormattingContextBuilder { ) -> ArcRefCell { // If there is an existing undamaged layout box that's compatible, use that. let independent_formatting_context = old_layout_box - .and_then(LayoutBox::unsplit_inline_level_layout_box) - .and_then(|inline_item| match &*inline_item.borrow() { - InlineItem::Atomic(atomic, ..) => Some(atomic.clone()), - _ => None, + .and_then(|layout_box| { + let LayoutBox::InlineLevel(inline_level_boxes) = layout_box else { + return None; + }; + + // If there's an existing box, it should be a compatible atomic inline and should + // not have been subject to inline-block splitting. + assert_eq!(inline_level_boxes.len(), 1); + let first_box = inline_level_boxes.into_iter().next()?; + match &*first_box.borrow() { + InlineItem::Atomic(atomic, ..) => Some(atomic.clone()), + _ => None, + } }) .unwrap_or_else(independent_formatting_context_creator); @@ -186,20 +195,9 @@ impl InlineFormattingContextBuilder { pub(crate) fn push_absolutely_positioned_box( &mut self, - absolutely_positioned_box_creator: impl FnOnce() -> ArcRefCell, - old_layout_box: Option, + absolutely_positioned_box: AbsolutelyPositionedBox, ) -> ArcRefCell { - let absolutely_positioned_box = old_layout_box - .and_then(LayoutBox::unsplit_inline_level_layout_box) - .and_then(|inline_item| match &*inline_item.borrow() { - InlineItem::OutOfFlowAbsolutelyPositionedBox(positioned_box, ..) => { - Some(positioned_box.clone()) - }, - _ => None, - }) - .unwrap_or_else(absolutely_positioned_box_creator); - - // We cannot just reuse the old inline item, because the `current_text_offset` may have changed. + let absolutely_positioned_box = ArcRefCell::new(absolutely_positioned_box); let inline_level_box = ArcRefCell::new(InlineItem::OutOfFlowAbsolutelyPositionedBox( absolutely_positioned_box, self.current_text_offset, @@ -209,23 +207,9 @@ impl InlineFormattingContextBuilder { inline_level_box } - pub(crate) fn push_float_box( - &mut self, - float_box_creator: impl FnOnce() -> ArcRefCell, - old_layout_box: Option, - ) -> ArcRefCell { - let inline_level_box = old_layout_box - .and_then(LayoutBox::unsplit_inline_level_layout_box) - .unwrap_or_else(|| ArcRefCell::new(InlineItem::OutOfFlowFloatBox(float_box_creator()))); - - debug_assert!( - matches!( - &*inline_level_box.borrow(), - InlineItem::OutOfFlowFloatBox(..), - ), - "Created float box with incompatible `old_layout_box`" - ); - + pub(crate) fn push_float_box(&mut self, float_box: FloatBox) -> ArcRefCell { + let inline_level_box = + ArcRefCell::new(InlineItem::OutOfFlowFloatBox(ArcRefCell::new(float_box))); self.inline_items.push(inline_level_box.clone()); self.contains_floats = true; inline_level_box