From c288bfb40453d4abe95af694dfc7c2175cd04a14 Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Fri, 2 Aug 2024 11:58:56 +0200 Subject: [PATCH] LibWeb: Only remember source CSSStyleDeclaration for animation-name We were saving to source declarations for *every* property, even though we only ever looked it up for animation-name. This patch gets rid of the per-property source pointer and we now keep a single pointer to the animation-name source only. This shrinks StyleProperties from 6512 bytes to 4368 bytes per instance. --- .../LibWeb/Animations/KeyframeEffect.cpp | 2 +- .../Libraries/LibWeb/CSS/StyleComputer.cpp | 56 ++++++++++--------- Userland/Libraries/LibWeb/CSS/StyleComputer.h | 4 +- .../Libraries/LibWeb/CSS/StyleProperties.cpp | 9 +-- .../Libraries/LibWeb/CSS/StyleProperties.h | 16 ++++-- Userland/Libraries/LibWeb/DOM/Element.cpp | 2 +- .../Libraries/LibWeb/HTML/HTMLBodyElement.cpp | 6 +- 7 files changed, 50 insertions(+), 45 deletions(-) diff --git a/Userland/Libraries/LibWeb/Animations/KeyframeEffect.cpp b/Userland/Libraries/LibWeb/Animations/KeyframeEffect.cpp index 2f670f2a3ad..668475f4aae 100644 --- a/Userland/Libraries/LibWeb/Animations/KeyframeEffect.cpp +++ b/Userland/Libraries/LibWeb/Animations/KeyframeEffect.cpp @@ -977,7 +977,7 @@ void KeyframeEffect::update_style_properties() for (auto i = to_underlying(CSS::first_property_id); i <= to_underlying(CSS::last_property_id); ++i) { if (element_style->is_property_inherited(static_cast(i))) { auto new_value = CSS::StyleComputer::get_inherit_value(document.realm(), static_cast(i), &element); - element_style->set_property(static_cast(i), *new_value, nullptr, CSS::StyleProperties::Inherited::Yes); + element_style->set_property(static_cast(i), *new_value, CSS::StyleProperties::Inherited::Yes); } } diff --git a/Userland/Libraries/LibWeb/CSS/StyleComputer.cpp b/Userland/Libraries/LibWeb/CSS/StyleComputer.cpp index b1e6a456e81..dd3889c825d 100644 --- a/Userland/Libraries/LibWeb/CSS/StyleComputer.cpp +++ b/Userland/Libraries/LibWeb/CSS/StyleComputer.cpp @@ -835,20 +835,25 @@ void StyleComputer::for_each_property_expanding_shorthands(PropertyID property_i set_longhand_property(property_id, value); } -void StyleComputer::set_property_expanding_shorthands(StyleProperties& style, CSS::PropertyID property_id, StyleValue const& value, CSS::CSSStyleDeclaration const* declaration, StyleProperties::PropertyValues const& properties_for_revert, StyleProperties::Important important) +void StyleComputer::set_property_expanding_shorthands(StyleProperties& style, CSS::PropertyID property_id, StyleValue const& value, CSS::CSSStyleDeclaration const* declaration, StyleProperties::PropertyValues const& properties_for_revert, CSS::CSSStyleDeclaration const* animation_name_source_for_revert, StyleProperties::Important important) { for_each_property_expanding_shorthands(property_id, value, AllowUnresolved::No, [&](PropertyID shorthand_id, StyleValue const& shorthand_value) { if (shorthand_value.is_revert()) { auto& property_in_previous_cascade_origin = properties_for_revert[to_underlying(shorthand_id)]; - if (property_in_previous_cascade_origin.style) - style.set_property(shorthand_id, *property_in_previous_cascade_origin.style, property_in_previous_cascade_origin.declaration, StyleProperties::Inherited::No, important); + if (property_in_previous_cascade_origin.style) { + style.set_property(shorthand_id, *property_in_previous_cascade_origin.style, StyleProperties::Inherited::No, important); + if (shorthand_id == CSS::PropertyID::AnimationName) + style.set_animation_name_source(animation_name_source_for_revert); + } } else { - style.set_property(shorthand_id, shorthand_value, declaration, StyleProperties::Inherited::No, important); + style.set_property(shorthand_id, shorthand_value, StyleProperties::Inherited::No, important); + if (shorthand_id == CSS::PropertyID::AnimationName) + style.set_animation_name_source(declaration); } }); } -void StyleComputer::set_all_properties(DOM::Element& element, Optional pseudo_element, StyleProperties& style, StyleValue const& value, DOM::Document& document, CSS::CSSStyleDeclaration const* declaration, StyleProperties::PropertyValues const& properties_for_revert, StyleProperties::Important important) const +void StyleComputer::set_all_properties(DOM::Element& element, Optional pseudo_element, StyleProperties& style, StyleValue const& value, DOM::Document& document, CSS::CSSStyleDeclaration const* declaration, StyleProperties::PropertyValues const& properties_for_revert, CSS::CSSStyleDeclaration const* animation_name_source_for_revert, StyleProperties::Important important) const { for (auto i = to_underlying(CSS::first_longhand_property_id); i <= to_underlying(CSS::last_longhand_property_id); ++i) { auto property_id = (CSS::PropertyID)i; @@ -861,9 +866,9 @@ void StyleComputer::set_all_properties(DOM::Element& element, Optionalis_unresolved()) property_value = Parser::Parser::resolve_unresolved_style_value(Parser::ParsingContext { document }, element, pseudo_element, property_id, property_value->as_unresolved()); if (!property_value->is_unresolved()) - set_property_expanding_shorthands(style, property_id, property_value, declaration, properties_for_revert); + set_property_expanding_shorthands(style, property_id, property_value, declaration, properties_for_revert, animation_name_source_for_revert); style.m_property_values[to_underlying(property_id)].important = important; - set_property_expanding_shorthands(style, property_id, value, declaration, properties_for_revert, important); + set_property_expanding_shorthands(style, property_id, value, declaration, properties_for_revert, animation_name_source_for_revert, important); } } void StyleComputer::cascade_declarations(StyleProperties& style, DOM::Element& element, Optional pseudo_element, Vector const& matching_rules, CascadeOrigin cascade_origin, Important important) const { + auto animation_name_source_for_revert = style.animation_name_source(); auto properties_for_revert = style.properties(); for (auto const& match : matching_rules) { @@ -890,7 +896,7 @@ void StyleComputer::cascade_declarations(StyleProperties& style, DOM::Element& e continue; if (property.property_id == CSS::PropertyID::All) { - set_all_properties(element, pseudo_element, style, property.value, m_document, &match.rule->declaration(), properties_for_revert, important == Important::Yes ? StyleProperties::Important::Yes : StyleProperties::Important::No); + set_all_properties(element, pseudo_element, style, property.value, m_document, &match.rule->declaration(), properties_for_revert, animation_name_source_for_revert, important == Important::Yes ? StyleProperties::Important::Yes : StyleProperties::Important::No); continue; } @@ -898,7 +904,7 @@ void StyleComputer::cascade_declarations(StyleProperties& style, DOM::Element& e if (property.value->is_unresolved()) property_value = Parser::Parser::resolve_unresolved_style_value(Parser::ParsingContext { document() }, element, pseudo_element, property.property_id, property.value->as_unresolved()); if (!property_value->is_unresolved()) - set_property_expanding_shorthands(style, property.property_id, property_value, &match.rule->declaration(), properties_for_revert, important == Important::Yes ? StyleProperties::Important::Yes : StyleProperties::Important::No); + set_property_expanding_shorthands(style, property.property_id, property_value, &match.rule->declaration(), properties_for_revert, animation_name_source_for_revert, important == Important::Yes ? StyleProperties::Important::Yes : StyleProperties::Important::No); } } @@ -909,7 +915,7 @@ void StyleComputer::cascade_declarations(StyleProperties& style, DOM::Element& e continue; if (property.property_id == CSS::PropertyID::All) { - set_all_properties(element, pseudo_element, style, property.value, m_document, inline_style, properties_for_revert, important == Important::Yes ? StyleProperties::Important::Yes : StyleProperties::Important::No); + set_all_properties(element, pseudo_element, style, property.value, m_document, inline_style, properties_for_revert, animation_name_source_for_revert, important == Important::Yes ? StyleProperties::Important::Yes : StyleProperties::Important::No); continue; } @@ -917,7 +923,7 @@ void StyleComputer::cascade_declarations(StyleProperties& style, DOM::Element& e if (property.value->is_unresolved()) property_value = Parser::Parser::resolve_unresolved_style_value(Parser::ParsingContext { document() }, element, pseudo_element, property.property_id, property.value->as_unresolved()); if (!property_value->is_unresolved()) - set_property_expanding_shorthands(style, property.property_id, property_value, inline_style, properties_for_revert, important == Important::Yes ? StyleProperties::Important::Yes : StyleProperties::Important::No); + set_property_expanding_shorthands(style, property.property_id, property_value, inline_style, properties_for_revert, animation_name_source_for_revert, important == Important::Yes ? StyleProperties::Important::Yes : StyleProperties::Important::No); } } } @@ -1771,7 +1777,7 @@ void StyleComputer::compute_cascaded_values(StyleProperties& style, DOM::Element }(); if (animation_name.has_value()) { - if (auto source_declaration = style.property_source_declaration(PropertyID::AnimationName); source_declaration) { + if (auto source_declaration = style.animation_name_source()) { auto& realm = element.realm(); if (source_declaration != element.cached_animation_name_source(pseudo_element)) { @@ -1862,9 +1868,9 @@ void StyleComputer::compute_defaulted_property_value(StyleProperties& style, DOM auto& value_slot = style.m_property_values[to_underlying(property_id)]; if (!value_slot.style) { if (is_inherited_property(property_id)) - style.m_property_values[to_underlying(property_id)] = { get_inherit_value(document().realm(), property_id, element, pseudo_element), nullptr, StyleProperties::Important::No, StyleProperties::Inherited::Yes }; + style.m_property_values[to_underlying(property_id)] = { get_inherit_value(document().realm(), property_id, element, pseudo_element), StyleProperties::Important::No, StyleProperties::Inherited::Yes }; else - style.m_property_values[to_underlying(property_id)] = { property_initial_value(document().realm(), property_id), nullptr }; + style.m_property_values[to_underlying(property_id)] = { property_initial_value(document().realm(), property_id) }; return; } @@ -2291,7 +2297,7 @@ void StyleComputer::compute_font(StyleProperties& style, DOM::Element const* ele RefPtr const found_font = font_list->first(); - style.set_property(CSS::PropertyID::FontSize, LengthStyleValue::create(CSS::Length::make_px(CSSPixels::nearest_value_for(found_font->pixel_size()))), nullptr); + style.set_property(CSS::PropertyID::FontSize, LengthStyleValue::create(CSS::Length::make_px(CSSPixels::nearest_value_for(found_font->pixel_size())))); style.set_property(CSS::PropertyID::FontWeight, NumberStyleValue::create(font_weight->to_font_weight())); style.set_computed_font_list(*font_list); @@ -2355,13 +2361,13 @@ void StyleComputer::resolve_effective_overflow_values(StyleProperties& style) co auto overflow_y_is_visible_or_clip = overflow_y == Overflow::Visible || overflow_y == Overflow::Clip; if (!overflow_x_is_visible_or_clip || !overflow_y_is_visible_or_clip) { if (overflow_x == CSS::Overflow::Visible) - style.set_property(CSS::PropertyID::OverflowX, IdentifierStyleValue::create(CSS::ValueID::Auto), nullptr); + style.set_property(CSS::PropertyID::OverflowX, IdentifierStyleValue::create(CSS::ValueID::Auto)); if (overflow_x == CSS::Overflow::Clip) - style.set_property(CSS::PropertyID::OverflowX, IdentifierStyleValue::create(CSS::ValueID::Hidden), nullptr); + style.set_property(CSS::PropertyID::OverflowX, IdentifierStyleValue::create(CSS::ValueID::Hidden)); if (overflow_y == CSS::Overflow::Visible) - style.set_property(CSS::PropertyID::OverflowY, IdentifierStyleValue::create(CSS::ValueID::Auto), nullptr); + style.set_property(CSS::PropertyID::OverflowY, IdentifierStyleValue::create(CSS::ValueID::Auto)); if (overflow_y == CSS::Overflow::Clip) - style.set_property(CSS::PropertyID::OverflowY, IdentifierStyleValue::create(CSS::ValueID::Hidden), nullptr); + style.set_property(CSS::PropertyID::OverflowY, IdentifierStyleValue::create(CSS::ValueID::Hidden)); } } @@ -2477,7 +2483,7 @@ void StyleComputer::transform_box_type_if_needed(StyleProperties& style, DOM::El } if (new_display != display) - style.set_property(CSS::PropertyID::Display, DisplayStyleValue::create(new_display), style.property_source_declaration(CSS::PropertyID::Display)); + style.set_property(CSS::PropertyID::Display, DisplayStyleValue::create(new_display)); } NonnullRefPtr StyleComputer::create_document_style() const @@ -2487,9 +2493,9 @@ NonnullRefPtr StyleComputer::create_document_style() const compute_font(style, nullptr, {}); compute_defaulted_values(style, nullptr, {}); absolutize_values(style); - style->set_property(CSS::PropertyID::Width, CSS::LengthStyleValue::create(CSS::Length::make_px(viewport_rect().width())), nullptr); - style->set_property(CSS::PropertyID::Height, CSS::LengthStyleValue::create(CSS::Length::make_px(viewport_rect().height())), nullptr); - style->set_property(CSS::PropertyID::Display, CSS::DisplayStyleValue::create(CSS::Display::from_short(CSS::Display::Short::Block)), nullptr); + style->set_property(CSS::PropertyID::Width, CSS::LengthStyleValue::create(CSS::Length::make_px(viewport_rect().width()))); + style->set_property(CSS::PropertyID::Height, CSS::LengthStyleValue::create(CSS::Length::make_px(viewport_rect().height()))); + style->set_property(CSS::PropertyID::Display, CSS::DisplayStyleValue::create(CSS::Display::from_short(CSS::Display::Short::Block))); return style; } diff --git a/Userland/Libraries/LibWeb/CSS/StyleComputer.h b/Userland/Libraries/LibWeb/CSS/StyleComputer.h index 9da35c51325..0d60aa0ab8a 100644 --- a/Userland/Libraries/LibWeb/CSS/StyleComputer.h +++ b/Userland/Libraries/LibWeb/CSS/StyleComputer.h @@ -113,7 +113,7 @@ public: No, }; static void for_each_property_expanding_shorthands(PropertyID, StyleValue const&, AllowUnresolved, Function const& set_longhand_property); - static void set_property_expanding_shorthands(StyleProperties&, PropertyID, StyleValue const&, CSS::CSSStyleDeclaration const*, StyleProperties::PropertyValues const& properties_for_revert, StyleProperties::Important = StyleProperties::Important::No); + static void set_property_expanding_shorthands(StyleProperties&, PropertyID, StyleValue const&, CSS::CSSStyleDeclaration const*, StyleProperties::PropertyValues const& properties_for_revert, CSS::CSSStyleDeclaration const* animation_name_source_for_revert, StyleProperties::Important = StyleProperties::Important::No); static NonnullRefPtr get_inherit_value(JS::Realm& initial_value_context_realm, CSS::PropertyID, DOM::Element const*, Optional = {}); explicit StyleComputer(DOM::Document&); @@ -177,7 +177,7 @@ private: void compute_defaulted_property_value(StyleProperties&, DOM::Element const*, CSS::PropertyID, Optional) const; - void set_all_properties(DOM::Element&, Optional, StyleProperties&, StyleValue const&, DOM::Document&, CSS::CSSStyleDeclaration const*, StyleProperties::PropertyValues const& properties_for_revert, StyleProperties::Important = StyleProperties::Important::No) const; + void set_all_properties(DOM::Element&, Optional, StyleProperties&, StyleValue const&, DOM::Document&, CSS::CSSStyleDeclaration const*, StyleProperties::PropertyValues const& properties_for_revert, CSS::CSSStyleDeclaration const* animation_name_source_for_revert, StyleProperties::Important = StyleProperties::Important::No) const; template void for_each_stylesheet(CascadeOrigin, Callback) const; diff --git a/Userland/Libraries/LibWeb/CSS/StyleProperties.cpp b/Userland/Libraries/LibWeb/CSS/StyleProperties.cpp index 305aa531c55..3f32e2e1f82 100644 --- a/Userland/Libraries/LibWeb/CSS/StyleProperties.cpp +++ b/Userland/Libraries/LibWeb/CSS/StyleProperties.cpp @@ -47,9 +47,9 @@ bool StyleProperties::is_property_inherited(CSS::PropertyID property_id) const return m_property_values[to_underlying(property_id)].style && m_property_values[to_underlying(property_id)].inherited == Inherited::Yes; } -void StyleProperties::set_property(CSS::PropertyID id, NonnullRefPtr value, CSS::CSSStyleDeclaration const* source_declaration, Inherited inherited, Important important) +void StyleProperties::set_property(CSS::PropertyID id, NonnullRefPtr value, Inherited inherited, Important important) { - m_property_values[to_underlying(id)] = StyleAndSourceDeclaration { move(value), source_declaration, important, inherited }; + m_property_values[to_underlying(id)] = StyleValueAndMetadata { move(value), important, inherited }; } void StyleProperties::set_animated_property(CSS::PropertyID id, NonnullRefPtr value) @@ -78,11 +78,6 @@ RefPtr StyleProperties::maybe_null_property(CSS::PropertyID pr return m_property_values[to_underlying(property_id)].style; } -CSS::CSSStyleDeclaration const* StyleProperties::property_source_declaration(CSS::PropertyID property_id) const -{ - return m_property_values[to_underlying(property_id)].declaration; -} - CSS::Size StyleProperties::size_value(CSS::PropertyID id) const { auto value = property(id); diff --git a/Userland/Libraries/LibWeb/CSS/StyleProperties.h b/Userland/Libraries/LibWeb/CSS/StyleProperties.h index 7558842d503..d5fb1caf83b 100644 --- a/Userland/Libraries/LibWeb/CSS/StyleProperties.h +++ b/Userland/Libraries/LibWeb/CSS/StyleProperties.h @@ -1,5 +1,5 @@ /* - * Copyright (c) 2018-2023, Andreas Kling + * Copyright (c) 2018-2024, Andreas Kling * * SPDX-License-Identifier: BSD-2-Clause */ @@ -43,13 +43,12 @@ public: Yes }; - struct StyleAndSourceDeclaration { + struct StyleValueAndMetadata { RefPtr style; - JS::GCPtr declaration; Important important { Important::No }; Inherited inherited { Inherited::No }; }; - using PropertyValues = Array; + using PropertyValues = Array; auto& properties() { return m_property_values; } auto const& properties() const { return m_property_values; } @@ -60,11 +59,13 @@ public: bool is_property_important(CSS::PropertyID property_id) const; bool is_property_inherited(CSS::PropertyID property_id) const; - void set_property(CSS::PropertyID, NonnullRefPtr value, CSS::CSSStyleDeclaration const* source_declaration = nullptr, Inherited = Inherited::No, Important = Important::No); + void set_property(CSS::PropertyID, NonnullRefPtr value, Inherited = Inherited::No, Important = Important::No); void set_animated_property(CSS::PropertyID, NonnullRefPtr value); NonnullRefPtr property(CSS::PropertyID) const; RefPtr maybe_null_property(CSS::PropertyID) const; - CSS::CSSStyleDeclaration const* property_source_declaration(CSS::PropertyID) const; + + JS::GCPtr animation_name_source() const { return m_animation_name_source; } + void set_animation_name_source(JS::GCPtr declaration) { m_animation_name_source = declaration; } CSS::Size size_value(CSS::PropertyID) const; LengthPercentage length_percentage_or_fallback(CSS::PropertyID, LengthPercentage const& fallback) const; @@ -188,6 +189,9 @@ public: private: friend class StyleComputer; + // FIXME: This needs protection from GC! + JS::GCPtr m_animation_name_source; + PropertyValues m_property_values; HashMap> m_animated_property_values; diff --git a/Userland/Libraries/LibWeb/DOM/Element.cpp b/Userland/Libraries/LibWeb/DOM/Element.cpp index cc82e7a5a81..4a70684f513 100644 --- a/Userland/Libraries/LibWeb/DOM/Element.cpp +++ b/Userland/Libraries/LibWeb/DOM/Element.cpp @@ -617,7 +617,7 @@ NonnullRefPtr Element::resolved_css_values() auto maybe_value = element_computed_style->property(property_id); if (!maybe_value.has_value()) continue; - properties->set_property(property_id, maybe_value.release_value().value, nullptr); + properties->set_property(property_id, maybe_value.release_value().value); } return properties; diff --git a/Userland/Libraries/LibWeb/HTML/HTMLBodyElement.cpp b/Userland/Libraries/LibWeb/HTML/HTMLBodyElement.cpp index e359449d1ad..bb8ea8ec6b5 100644 --- a/Userland/Libraries/LibWeb/HTML/HTMLBodyElement.cpp +++ b/Userland/Libraries/LibWeb/HTML/HTMLBodyElement.cpp @@ -46,15 +46,15 @@ void HTMLBodyElement::apply_presentational_hints(CSS::StyleProperties& style) co // https://html.spec.whatwg.org/multipage/rendering.html#the-page:rules-for-parsing-a-legacy-colour-value auto color = parse_legacy_color_value(value); if (color.has_value()) - style.set_property(CSS::PropertyID::BackgroundColor, CSS::ColorStyleValue::create(color.value()), nullptr); + style.set_property(CSS::PropertyID::BackgroundColor, CSS::ColorStyleValue::create(color.value())); } else if (name.equals_ignoring_ascii_case("text"sv)) { // https://html.spec.whatwg.org/multipage/rendering.html#the-page:rules-for-parsing-a-legacy-colour-value-2 auto color = parse_legacy_color_value(value); if (color.has_value()) - style.set_property(CSS::PropertyID::Color, CSS::ColorStyleValue::create(color.value()), nullptr); + style.set_property(CSS::PropertyID::Color, CSS::ColorStyleValue::create(color.value())); } else if (name.equals_ignoring_ascii_case("background"sv)) { VERIFY(m_background_style_value); - style.set_property(CSS::PropertyID::BackgroundImage, *m_background_style_value, nullptr); + style.set_property(CSS::PropertyID::BackgroundImage, *m_background_style_value); } }); }