From 80d6330a26f8c55a1bd38fd2f1388e50c0ae8977 Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Wed, 17 May 2023 19:47:16 +0200 Subject: [PATCH] LibWeb: Don't create mutation record node lists if nobody is interested By deferring allocation of StaticNodeList objects until we know somebody actually wants the MutationRecord, we avoid a *lot* of allocation work. This shaves several seconds off of loading https://tc39.es/ecma262/ At least one other engine (WebKit) skips creating mutation records if nobody is interested, so even if this is observable somehow, we would at least match the behavior of a major engine. --- Userland/Libraries/LibWeb/DOM/Attr.cpp | 4 +- .../Libraries/LibWeb/DOM/CharacterData.cpp | 4 +- Userland/Libraries/LibWeb/DOM/Node.cpp | 39 ++++++++----------- Userland/Libraries/LibWeb/DOM/Node.h | 6 +-- 4 files changed, 22 insertions(+), 31 deletions(-) diff --git a/Userland/Libraries/LibWeb/DOM/Attr.cpp b/Userland/Libraries/LibWeb/DOM/Attr.cpp index aaf0b86cce..50a9f35649 100644 --- a/Userland/Libraries/LibWeb/DOM/Attr.cpp +++ b/Userland/Libraries/LibWeb/DOM/Attr.cpp @@ -89,9 +89,7 @@ void Attr::set_value(DeprecatedString value) void Attr::handle_attribute_changes(Element& element, DeprecatedString const& old_value, DeprecatedString const& new_value) { // 1. Queue a mutation record of "attributes" for element with attribute’s local name, attribute’s namespace, oldValue, « », « », null, and null. - auto added_node_list = StaticNodeList::create(realm(), {}).release_value_but_fixme_should_propagate_errors(); - auto removed_node_list = StaticNodeList::create(realm(), {}).release_value_but_fixme_should_propagate_errors(); - element.queue_mutation_record(MutationType::attributes, local_name(), namespace_uri(), old_value, added_node_list, removed_node_list, nullptr, nullptr); + element.queue_mutation_record(MutationType::attributes, local_name(), namespace_uri(), old_value, {}, {}, nullptr, nullptr); // 2. If element is custom, then enqueue a custom element callback reaction with element, callback name "attributeChangedCallback", and an argument list containing attribute’s local name, oldValue, newValue, and attribute’s namespace. if (element.is_custom()) { diff --git a/Userland/Libraries/LibWeb/DOM/CharacterData.cpp b/Userland/Libraries/LibWeb/DOM/CharacterData.cpp index 48744202ab..7ac5a43e2d 100644 --- a/Userland/Libraries/LibWeb/DOM/CharacterData.cpp +++ b/Userland/Libraries/LibWeb/DOM/CharacterData.cpp @@ -71,9 +71,7 @@ WebIDL::ExceptionOr CharacterData::replace_data(size_t offset, size_t coun count = length - offset; // 4. Queue a mutation record of "characterData" for node with null, null, node’s data, « », « », null, and null. - auto added_node_list = TRY(StaticNodeList::create(realm(), {})); - auto removed_node_list = TRY(StaticNodeList::create(realm(), {})); - queue_mutation_record(MutationType::characterData, {}, {}, m_data, added_node_list, removed_node_list, nullptr, nullptr); + queue_mutation_record(MutationType::characterData, {}, {}, m_data, {}, {}, nullptr, nullptr); // 5. Insert data into node’s data after offset code units. // 6. Let delete offset be offset + data’s length. diff --git a/Userland/Libraries/LibWeb/DOM/Node.cpp b/Userland/Libraries/LibWeb/DOM/Node.cpp index 8fc11aaad1..0a080b86c1 100644 --- a/Userland/Libraries/LibWeb/DOM/Node.cpp +++ b/Userland/Libraries/LibWeb/DOM/Node.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2018-2022, Andreas Kling + * Copyright (c) 2018-2023, Andreas Kling * Copyright (c) 2021-2022, Linus Groh * Copyright (c) 2021, Luke Wilde * @@ -405,9 +405,7 @@ void Node::insert_before(JS::NonnullGCPtr node, JS::GCPtr child, boo // 2. Queue a tree mutation record for node with « », nodes, null, and null. // NOTE: This step intentionally does not pay attention to the suppress observers flag. - auto added_node_list = StaticNodeList::create(realm(), {}).release_value_but_fixme_should_propagate_errors(); - auto removed_node_list = StaticNodeList::create(realm(), nodes).release_value_but_fixme_should_propagate_errors(); - node->queue_tree_mutation_record(added_node_list, removed_node_list, nullptr, nullptr); + node->queue_tree_mutation_record({}, nodes, nullptr, nullptr); } // 5. If child is non-null, then: @@ -480,9 +478,7 @@ void Node::insert_before(JS::NonnullGCPtr node, JS::GCPtr child, boo // 8. If suppress observers flag is unset, then queue a tree mutation record for parent with nodes, « », previousSibling, and child. if (!suppress_observers) { - auto added_node_list = StaticNodeList::create(realm(), move(nodes)).release_value_but_fixme_should_propagate_errors(); - auto removed_node_list = StaticNodeList::create(realm(), {}).release_value_but_fixme_should_propagate_errors(); - queue_tree_mutation_record(added_node_list, removed_node_list, previous_sibling.ptr(), child.ptr()); + queue_tree_mutation_record(move(nodes), {}, previous_sibling.ptr(), child.ptr()); } // 9. Run the children changed steps for parent. @@ -650,11 +646,7 @@ void Node::remove(bool suppress_observers) // 20. If suppress observers flag is unset, then queue a tree mutation record for parent with « », « node », oldPreviousSibling, and oldNextSibling. if (!suppress_observers) { - Vector> removed_nodes; - removed_nodes.append(JS::make_handle(*this)); - auto added_node_list = StaticNodeList::create(realm(), {}).release_value_but_fixme_should_propagate_errors(); - auto removed_node_list = StaticNodeList::create(realm(), move(removed_nodes)).release_value_but_fixme_should_propagate_errors(); - parent->queue_tree_mutation_record(added_node_list, removed_node_list, old_previous_sibling.ptr(), old_next_sibling.ptr()); + parent->queue_tree_mutation_record({}, { *this }, old_previous_sibling.ptr(), old_next_sibling.ptr()); } // 21. Run the children changed steps for parent. @@ -749,9 +741,7 @@ WebIDL::ExceptionOr> Node::replace_child(JS::NonnullGCPtr insert_before(node, reference_child, true); // 14. Queue a tree mutation record for parent with nodes, removedNodes, previousSibling, and referenceChild. - auto added_node_list = TRY(StaticNodeList::create(realm(), move(nodes))); - auto removed_node_list = TRY(StaticNodeList::create(realm(), move(removed_nodes))); - queue_tree_mutation_record(added_node_list, removed_node_list, previous_sibling.ptr(), reference_child.ptr()); + queue_tree_mutation_record(move(nodes), move(removed_nodes), previous_sibling.ptr(), reference_child.ptr()); // 15. Return child. return child; @@ -1220,9 +1210,7 @@ void Node::replace_all(JS::GCPtr node) // 7. If either addedNodes or removedNodes is not empty, then queue a tree mutation record for parent with addedNodes, removedNodes, null, and null. if (!added_nodes.is_empty() || !removed_nodes.is_empty()) { - auto added_node_list = StaticNodeList::create(realm(), move(added_nodes)).release_value_but_fixme_should_propagate_errors(); - auto removed_node_list = StaticNodeList::create(realm(), move(removed_nodes)).release_value_but_fixme_should_propagate_errors(); - queue_tree_mutation_record(added_node_list, removed_node_list, nullptr, nullptr); + queue_tree_mutation_record(move(added_nodes), move(removed_nodes), nullptr, nullptr); } } @@ -1430,7 +1418,7 @@ Painting::PaintableBox const* Node::paintable_box() const } // https://dom.spec.whatwg.org/#queue-a-mutation-record -void Node::queue_mutation_record(FlyString const& type, DeprecatedString attribute_name, DeprecatedString attribute_namespace, DeprecatedString old_value, JS::NonnullGCPtr added_nodes, JS::NonnullGCPtr removed_nodes, Node* previous_sibling, Node* next_sibling) const +void Node::queue_mutation_record(FlyString const& type, DeprecatedString attribute_name, DeprecatedString attribute_namespace, DeprecatedString old_value, Vector> added_nodes, Vector> removed_nodes, Node* previous_sibling, Node* next_sibling) const { // NOTE: We defer garbage collection until the end of the scope, since we can't safely use MutationObserver* as a hashmap key otherwise. // FIXME: This is a total hack. @@ -1479,11 +1467,18 @@ void Node::queue_mutation_record(FlyString const& type, DeprecatedString attribu } } + // OPTIMIZATION: If there are no interested observers, bail without doing any more work. + if (interested_observers.is_empty()) + return; + + auto added_nodes_list = StaticNodeList::create(realm(), move(added_nodes)).release_value_but_fixme_should_propagate_errors(); + auto removed_nodes_list = StaticNodeList::create(realm(), move(removed_nodes)).release_value_but_fixme_should_propagate_errors(); + // 4. For each observer → mappedOldValue of interestedObservers: for (auto& interested_observer : interested_observers) { // 1. Let record be a new MutationRecord object with its type set to type, target set to target, attributeName set to name, attributeNamespace set to namespace, oldValue set to mappedOldValue, // addedNodes set to addedNodes, removedNodes set to removedNodes, previousSibling set to previousSibling, and nextSibling set to nextSibling. - auto record = MutationRecord::create(realm(), type, *this, added_nodes, removed_nodes, previous_sibling, next_sibling, attribute_name, attribute_namespace, /* mappedOldValue */ interested_observer.value).release_value_but_fixme_should_propagate_errors(); + auto record = MutationRecord::create(realm(), type, *this, added_nodes_list, removed_nodes_list, previous_sibling, next_sibling, attribute_name, attribute_namespace, /* mappedOldValue */ interested_observer.value).release_value_but_fixme_should_propagate_errors(); // 2. Enqueue record to observer’s record queue. interested_observer.key->enqueue_record({}, move(record)); @@ -1494,10 +1489,10 @@ void Node::queue_mutation_record(FlyString const& type, DeprecatedString attribu } // https://dom.spec.whatwg.org/#queue-a-tree-mutation-record -void Node::queue_tree_mutation_record(JS::NonnullGCPtr added_nodes, JS::NonnullGCPtr removed_nodes, Node* previous_sibling, Node* next_sibling) +void Node::queue_tree_mutation_record(Vector> added_nodes, Vector> removed_nodes, Node* previous_sibling, Node* next_sibling) { // 1. Assert: either addedNodes or removedNodes is not empty. - VERIFY(added_nodes->length() > 0 || removed_nodes->length() > 0); + VERIFY(added_nodes.size() > 0 || removed_nodes.size() > 0); // 2. Queue a mutation record of "childList" for target with null, null, null, addedNodes, removedNodes, previousSibling, and nextSibling. queue_mutation_record(MutationType::childList, {}, {}, {}, move(added_nodes), move(removed_nodes), previous_sibling, next_sibling); diff --git a/Userland/Libraries/LibWeb/DOM/Node.h b/Userland/Libraries/LibWeb/DOM/Node.h index f0b3f486ba..c1abd5ea0d 100644 --- a/Userland/Libraries/LibWeb/DOM/Node.h +++ b/Userland/Libraries/LibWeb/DOM/Node.h @@ -1,5 +1,5 @@ /* - * Copyright (c) 2018-2022, Andreas Kling + * Copyright (c) 2018-2023, Andreas Kling * * SPDX-License-Identifier: BSD-2-Clause */ @@ -242,7 +242,7 @@ public: void add_registered_observer(RegisteredObserver& registered_observer) { m_registered_observer_list.append(registered_observer); } - void queue_mutation_record(FlyString const& type, DeprecatedString attribute_name, DeprecatedString attribute_namespace, DeprecatedString old_value, JS::NonnullGCPtr added_nodes, JS::NonnullGCPtr removed_nodes, Node* previous_sibling, Node* next_sibling) const; + void queue_mutation_record(FlyString const& type, DeprecatedString attribute_name, DeprecatedString attribute_namespace, DeprecatedString old_value, Vector> added_nodes, Vector> removed_nodes, Node* previous_sibling, Node* next_sibling) const; // https://dom.spec.whatwg.org/#concept-shadow-including-inclusive-descendant template @@ -671,7 +671,7 @@ protected: ErrorOr name_or_description(NameOrDescription, Document const&, HashTable&) const; private: - void queue_tree_mutation_record(JS::NonnullGCPtr added_nodes, JS::NonnullGCPtr removed_nodes, Node* previous_sibling, Node* next_sibling); + void queue_tree_mutation_record(Vector> added_nodes, Vector> removed_nodes, Node* previous_sibling, Node* next_sibling); void insert_before_impl(JS::NonnullGCPtr, JS::GCPtr child); void append_child_impl(JS::NonnullGCPtr);