From ad7e6878feb0c22bdee2dc329a588d2a8f8893ef Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Sat, 27 Jan 2024 08:38:27 +0100 Subject: [PATCH] LibWeb: Allocate CSS::ComputedValues objects on the heap This makes them cheap to move around, since we can store them in a NonnullOwnPtr instead of memcopying 2584(!) bytes. Drastically reduces the chance of stack overflow while building the layout tree for deeply nested DOMs (since tree building was putting these things on the stack). This change also exposed a completely unnecessary ComputedValues deep copy in block layout. --- .../Libraries/LibWeb/CSS/ComputedValues.h | 12 +++++++++--- .../LibWeb/Layout/BlockContainer.cpp | 2 +- .../Libraries/LibWeb/Layout/BlockContainer.h | 2 +- .../LibWeb/Layout/BlockFormattingContext.cpp | 2 +- Userland/Libraries/LibWeb/Layout/Box.cpp | 2 +- Userland/Libraries/LibWeb/Layout/Box.h | 2 +- Userland/Libraries/LibWeb/Layout/Node.cpp | 19 ++++++++++--------- Userland/Libraries/LibWeb/Layout/Node.h | 10 +++++----- .../Libraries/LibWeb/Layout/TableWrapper.cpp | 2 +- .../Libraries/LibWeb/Layout/TableWrapper.h | 2 +- .../Libraries/LibWeb/Layout/TreeBuilder.cpp | 18 +++++++++--------- 11 files changed, 40 insertions(+), 33 deletions(-) diff --git a/Userland/Libraries/LibWeb/CSS/ComputedValues.h b/Userland/Libraries/LibWeb/CSS/ComputedValues.h index 27b32a30ae..29fadd3734 100644 --- a/Userland/Libraries/LibWeb/CSS/ComputedValues.h +++ b/Userland/Libraries/LibWeb/CSS/ComputedValues.h @@ -285,7 +285,13 @@ inline Gfx::Painter::ScalingMode to_gfx_scaling_mode(CSS::ImageRendering css_val } class ComputedValues { + AK_MAKE_NONCOPYABLE(ComputedValues); + AK_MAKE_NONMOVABLE(ComputedValues); + public: + ComputedValues() = default; + ~ComputedValues() = default; + AspectRatio aspect_ratio() const { return m_noninherited.aspect_ratio; } CSS::Float float_() const { return m_noninherited.float_; } CSS::Length border_spacing_horizontal() const { return m_inherited.border_spacing_horizontal; } @@ -411,10 +417,10 @@ public: CSS::MathStyle math_style() const { return m_inherited.math_style; } int math_depth() const { return m_inherited.math_depth; } - ComputedValues clone_inherited_values() const + NonnullOwnPtr clone_inherited_values() const { - ComputedValues clone; - clone.m_inherited = m_inherited; + auto clone = make(); + clone->m_inherited = m_inherited; return clone; } diff --git a/Userland/Libraries/LibWeb/Layout/BlockContainer.cpp b/Userland/Libraries/LibWeb/Layout/BlockContainer.cpp index dc30bfb928..72bc09f379 100644 --- a/Userland/Libraries/LibWeb/Layout/BlockContainer.cpp +++ b/Userland/Libraries/LibWeb/Layout/BlockContainer.cpp @@ -14,7 +14,7 @@ BlockContainer::BlockContainer(DOM::Document& document, DOM::Node* node, Nonnull { } -BlockContainer::BlockContainer(DOM::Document& document, DOM::Node* node, CSS::ComputedValues computed_values) +BlockContainer::BlockContainer(DOM::Document& document, DOM::Node* node, NonnullOwnPtr computed_values) : Box(document, node, move(computed_values)) { } diff --git a/Userland/Libraries/LibWeb/Layout/BlockContainer.h b/Userland/Libraries/LibWeb/Layout/BlockContainer.h index 58d43a24a4..6145722313 100644 --- a/Userland/Libraries/LibWeb/Layout/BlockContainer.h +++ b/Userland/Libraries/LibWeb/Layout/BlockContainer.h @@ -17,7 +17,7 @@ class BlockContainer : public Box { public: BlockContainer(DOM::Document&, DOM::Node*, NonnullRefPtr); - BlockContainer(DOM::Document&, DOM::Node*, CSS::ComputedValues); + BlockContainer(DOM::Document&, DOM::Node*, NonnullOwnPtr); virtual ~BlockContainer() override; Painting::PaintableWithLines const* paintable_with_lines() const; diff --git a/Userland/Libraries/LibWeb/Layout/BlockFormattingContext.cpp b/Userland/Libraries/LibWeb/Layout/BlockFormattingContext.cpp index 37f752107b..387e0b34ec 100644 --- a/Userland/Libraries/LibWeb/Layout/BlockFormattingContext.cpp +++ b/Userland/Libraries/LibWeb/Layout/BlockFormattingContext.cpp @@ -755,7 +755,7 @@ void BlockFormattingContext::layout_block_level_children(BlockContainer const& b auto& block_container_state = m_state.get_mutable(block_container); if (!block_container_state.has_definite_width()) { auto width = greatest_child_width(block_container); - auto computed_values = block_container.computed_values(); + auto const& computed_values = block_container.computed_values(); // NOTE: Min and max constraints are not applied to a box that is being sized as intrinsic because // according to css-sizing-3 spec: // The min-content size of a box in each axis is the size it would have if it was a float given an diff --git a/Userland/Libraries/LibWeb/Layout/Box.cpp b/Userland/Libraries/LibWeb/Layout/Box.cpp index 9b5041c161..bf48595e9c 100644 --- a/Userland/Libraries/LibWeb/Layout/Box.cpp +++ b/Userland/Libraries/LibWeb/Layout/Box.cpp @@ -19,7 +19,7 @@ Box::Box(DOM::Document& document, DOM::Node* node, NonnullRefPtr computed_values) : NodeWithStyleAndBoxModelMetrics(document, node, move(computed_values)) { } diff --git a/Userland/Libraries/LibWeb/Layout/Box.h b/Userland/Libraries/LibWeb/Layout/Box.h index 69298e271c..7bfedc0d96 100644 --- a/Userland/Libraries/LibWeb/Layout/Box.h +++ b/Userland/Libraries/LibWeb/Layout/Box.h @@ -56,7 +56,7 @@ public: protected: Box(DOM::Document&, DOM::Node*, NonnullRefPtr); - Box(DOM::Document&, DOM::Node*, CSS::ComputedValues); + Box(DOM::Document&, DOM::Node*, NonnullOwnPtr); private: virtual bool is_box() const final { return true; } diff --git a/Userland/Libraries/LibWeb/Layout/Node.cpp b/Userland/Libraries/LibWeb/Layout/Node.cpp index 3ed407bbe9..524178da4d 100644 --- a/Userland/Libraries/LibWeb/Layout/Node.cpp +++ b/Userland/Libraries/LibWeb/Layout/Node.cpp @@ -259,12 +259,13 @@ bool Node::is_fixed_position() const NodeWithStyle::NodeWithStyle(DOM::Document& document, DOM::Node* node, NonnullRefPtr computed_style) : Node(document, node) + , m_computed_values(make()) { m_has_style = true; apply_style(*computed_style); } -NodeWithStyle::NodeWithStyle(DOM::Document& document, DOM::Node* node, CSS::ComputedValues computed_values) +NodeWithStyle::NodeWithStyle(DOM::Document& document, DOM::Node* node, NonnullOwnPtr computed_values) : Node(document, node) , m_computed_values(move(computed_values)) { @@ -274,7 +275,7 @@ NodeWithStyle::NodeWithStyle(DOM::Document& document, DOM::Node* node, CSS::Comp void NodeWithStyle::visit_edges(Visitor& visitor) { Base::visit_edges(visitor); - for (auto& layer : m_computed_values.background_layers()) { + for (auto& layer : computed_values().background_layers()) { if (layer.background_image && layer.background_image->is_image()) layer.background_image->as_image().visit_edges(visitor); } @@ -306,7 +307,7 @@ static CSSPixels snap_a_length_as_a_border_width(double device_pixels_per_css_pi void NodeWithStyle::apply_style(const CSS::StyleProperties& computed_style) { - auto& computed_values = static_cast(m_computed_values); + auto& computed_values = mutable_computed_values(); // NOTE: color must be set first to ensure currentColor can be resolved in other properties (e.g. background-color). computed_values.set_color(computed_style.color_or_fallback(CSS::PropertyID::Color, *this, CSS::InitialValues::color())); @@ -838,15 +839,15 @@ void NodeWithStyle::propagate_style_to_anonymous_wrappers() // the parent inherits style from *this* node, not the other way around. if (display().is_table_inside() && is(parent())) { auto& table_wrapper = *static_cast(parent()); - static_cast(static_cast(const_cast(table_wrapper.computed_values()))).inherit_from(m_computed_values); - transfer_table_box_computed_values_to_wrapper_computed_values(table_wrapper.m_computed_values); + static_cast(static_cast(const_cast(table_wrapper.computed_values()))).inherit_from(computed_values()); + transfer_table_box_computed_values_to_wrapper_computed_values(table_wrapper.mutable_computed_values()); } // Propagate style to all anonymous children (except table wrappers!) for_each_child_of_type([&](NodeWithStyle& child) { if (child.is_anonymous() && !is(child)) { auto& child_computed_values = static_cast(static_cast(const_cast(child.computed_values()))); - child_computed_values.inherit_from(m_computed_values); + child_computed_values.inherit_from(computed_values()); } }); } @@ -906,8 +907,8 @@ bool Node::is_inline_table() const JS::NonnullGCPtr NodeWithStyle::create_anonymous_wrapper() const { - auto wrapper = heap().allocate_without_realm(const_cast(document()), nullptr, m_computed_values.clone_inherited_values()); - static_cast(wrapper->m_computed_values).set_display(CSS::Display(CSS::DisplayOutside::Block, CSS::DisplayInside::Flow)); + auto wrapper = heap().allocate_without_realm(const_cast(document()), nullptr, computed_values().clone_inherited_values()); + wrapper->mutable_computed_values().set_display(CSS::Display(CSS::DisplayOutside::Block, CSS::DisplayInside::Flow)); return *wrapper; } @@ -915,7 +916,7 @@ void NodeWithStyle::reset_table_box_computed_values_used_by_wrapper_to_init_valu { VERIFY(this->display().is_table_inside()); - CSS::MutableComputedValues& mutable_computed_values = static_cast(m_computed_values); + auto& mutable_computed_values = this->mutable_computed_values(); mutable_computed_values.set_position(CSS::InitialValues::position()); mutable_computed_values.set_float(CSS::InitialValues::float_()); mutable_computed_values.set_clear(CSS::InitialValues::clear()); diff --git a/Userland/Libraries/LibWeb/Layout/Node.h b/Userland/Libraries/LibWeb/Layout/Node.h index d13482e5db..fe6c37bc7d 100644 --- a/Userland/Libraries/LibWeb/Layout/Node.h +++ b/Userland/Libraries/LibWeb/Layout/Node.h @@ -206,8 +206,8 @@ class NodeWithStyle : public Node { public: virtual ~NodeWithStyle() override = default; - const CSS::ImmutableComputedValues& computed_values() const { return static_cast(m_computed_values); } - CSS::MutableComputedValues& mutable_computed_values() { return static_cast(m_computed_values); } + CSS::ImmutableComputedValues const& computed_values() const { return static_cast(*m_computed_values); } + CSS::MutableComputedValues& mutable_computed_values() { return static_cast(*m_computed_values); } void apply_style(const CSS::StyleProperties&); @@ -223,13 +223,13 @@ public: protected: NodeWithStyle(DOM::Document&, DOM::Node*, NonnullRefPtr); - NodeWithStyle(DOM::Document&, DOM::Node*, CSS::ComputedValues); + NodeWithStyle(DOM::Document&, DOM::Node*, NonnullOwnPtr); private: void reset_table_box_computed_values_used_by_wrapper_to_init_values(); void propagate_style_to_anonymous_wrappers(); - CSS::ComputedValues m_computed_values; + NonnullOwnPtr m_computed_values; RefPtr m_list_style_image; }; @@ -246,7 +246,7 @@ protected: { } - NodeWithStyleAndBoxModelMetrics(DOM::Document& document, DOM::Node* node, CSS::ComputedValues computed_values) + NodeWithStyleAndBoxModelMetrics(DOM::Document& document, DOM::Node* node, NonnullOwnPtr computed_values) : NodeWithStyle(document, node, move(computed_values)) { } diff --git a/Userland/Libraries/LibWeb/Layout/TableWrapper.cpp b/Userland/Libraries/LibWeb/Layout/TableWrapper.cpp index 7baa516308..8323226cfa 100644 --- a/Userland/Libraries/LibWeb/Layout/TableWrapper.cpp +++ b/Userland/Libraries/LibWeb/Layout/TableWrapper.cpp @@ -13,7 +13,7 @@ TableWrapper::TableWrapper(DOM::Document& document, DOM::Node* node, NonnullRefP { } -TableWrapper::TableWrapper(DOM::Document& document, DOM::Node* node, CSS::ComputedValues computed_values) +TableWrapper::TableWrapper(DOM::Document& document, DOM::Node* node, NonnullOwnPtr computed_values) : BlockContainer(document, node, move(computed_values)) { } diff --git a/Userland/Libraries/LibWeb/Layout/TableWrapper.h b/Userland/Libraries/LibWeb/Layout/TableWrapper.h index de707bb783..faa26b8e9f 100644 --- a/Userland/Libraries/LibWeb/Layout/TableWrapper.h +++ b/Userland/Libraries/LibWeb/Layout/TableWrapper.h @@ -15,7 +15,7 @@ class TableWrapper : public BlockContainer { public: TableWrapper(DOM::Document&, DOM::Node*, NonnullRefPtr); - TableWrapper(DOM::Document&, DOM::Node*, CSS::ComputedValues); + TableWrapper(DOM::Document&, DOM::Node*, NonnullOwnPtr); virtual ~TableWrapper() override; private: diff --git a/Userland/Libraries/LibWeb/Layout/TreeBuilder.cpp b/Userland/Libraries/LibWeb/Layout/TreeBuilder.cpp index 0299564bfe..38ae3e7dc8 100644 --- a/Userland/Libraries/LibWeb/Layout/TreeBuilder.cpp +++ b/Userland/Libraries/LibWeb/Layout/TreeBuilder.cpp @@ -411,7 +411,7 @@ ErrorOr TreeBuilder::create_layout_tree(DOM::Node& dom_node, TreeBuilder:: // If the box does not overflow in the vertical axis, then it is centered vertically. // FIXME: Only apply alignment when box overflows auto flex_computed_values = parent.computed_values().clone_inherited_values(); - auto& mutable_flex_computed_values = static_cast(flex_computed_values); + auto& mutable_flex_computed_values = static_cast(*flex_computed_values); mutable_flex_computed_values.set_display(CSS::Display { CSS::DisplayOutside::Block, CSS::DisplayInside::Flex }); mutable_flex_computed_values.set_justify_content(CSS::JustifyContent::Center); mutable_flex_computed_values.set_flex_direction(CSS::FlexDirection::Column); @@ -613,7 +613,7 @@ static void wrap_in_anonymous(Vector>& sequence, Node* nearest_ VERIFY(!sequence.is_empty()); auto& parent = *sequence.first()->parent(); auto computed_values = parent.computed_values().clone_inherited_values(); - static_cast(computed_values).set_display(display); + static_cast(*computed_values).set_display(display); auto wrapper = parent.heap().template allocate_without_realm(parent.document(), nullptr, move(computed_values)); for (auto& child : sequence) { parent.remove_child(*child); @@ -699,8 +699,8 @@ Vector> TreeBuilder::generate_missing_parents(NodeWithStyle& roo auto* nearest_sibling = table_box->next_sibling(); auto& parent = *table_box->parent(); - CSS::ComputedValues wrapper_computed_values = table_box->computed_values().clone_inherited_values(); - table_box->transfer_table_box_computed_values_to_wrapper_computed_values(wrapper_computed_values); + auto wrapper_computed_values = table_box->computed_values().clone_inherited_values(); + table_box->transfer_table_box_computed_values_to_wrapper_computed_values(*wrapper_computed_values); auto wrapper = parent.heap().allocate_without_realm(parent.document(), nullptr, move(wrapper_computed_values)); @@ -731,12 +731,12 @@ static void fixup_row(Box& row_box, TableGrid const& table_grid, size_t row_inde if (table_grid.occupancy_grid().contains({ column_index, row_index })) continue; - auto row_computed_values = row_box.computed_values().clone_inherited_values(); - auto& cell_computed_values = static_cast(row_computed_values); - cell_computed_values.set_display(Web::CSS::Display { CSS::DisplayInternal::TableCell }); + auto computed_values = row_box.computed_values().clone_inherited_values(); + auto& mutable_computed_values = static_cast(*computed_values); + mutable_computed_values.set_display(Web::CSS::Display { CSS::DisplayInternal::TableCell }); // Ensure that the cell (with zero content height) will have the same height as the row by setting vertical-align to middle. - cell_computed_values.set_vertical_align(CSS::VerticalAlign::Middle); - auto cell_box = row_box.heap().template allocate_without_realm(row_box.document(), nullptr, cell_computed_values); + mutable_computed_values.set_vertical_align(CSS::VerticalAlign::Middle); + auto cell_box = row_box.heap().template allocate_without_realm(row_box.document(), nullptr, move(computed_values)); row_box.append_child(cell_box); } }