From 7604c2f38ed92137457b0475a188722352011a5a Mon Sep 17 00:00:00 2001 From: Timothy Flynn Date: Wed, 30 Jun 2021 07:07:38 -0400 Subject: [PATCH] LibWeb: Do not create copies of JSON values in OOPWV DOM Inspector To improve the performance of the DOM Inspector when the Browser is run in multi-process mode, do not create copies of the JSON values sent via IPC when searching for a model index. Methods that are guaranteed to return a value now return a reference. Methods that do not have such a guarantee return a pointer (rather than an Optional, because Optional cannot hold references). The DOM Inspector performs well at first, but will start lagging again once the tree is expanded a few nodes deep and/or with many nodes visible in the tree. --- .../Libraries/LibWeb/DOMTreeJSONModel.cpp | 106 ++++++++++-------- Userland/Libraries/LibWeb/DOMTreeJSONModel.h | 12 +- 2 files changed, 64 insertions(+), 54 deletions(-) diff --git a/Userland/Libraries/LibWeb/DOMTreeJSONModel.cpp b/Userland/Libraries/LibWeb/DOMTreeJSONModel.cpp index 70719302c9..efd0fea541 100644 --- a/Userland/Libraries/LibWeb/DOMTreeJSONModel.cpp +++ b/Userland/Libraries/LibWeb/DOMTreeJSONModel.cpp @@ -13,7 +13,7 @@ namespace Web { DOMTreeJSONModel::DOMTreeJSONModel(JsonObject dom_tree) - : m_dom_tree(dom_tree) + : m_dom_tree(move(dom_tree)) { m_document_icon.set_bitmap_for_size(16, Gfx::Bitmap::load_from_file("/res/icons/16x16/filetype-html.png")); m_element_icon.set_bitmap_for_size(16, Gfx::Bitmap::load_from_file("/res/icons/16x16/inspector-object.png")); @@ -30,10 +30,12 @@ GUI::ModelIndex DOMTreeJSONModel::index(int row, int column, const GUI::ModelInd return create_index(row, column, (void*)get_internal_id(m_dom_tree)); } - auto parent_node = find_node(parent); - auto children = get_children(parent_node); - auto child_node = children[row].as_object(); + auto const& parent_node = find_node(parent); + auto const* children = get_children(parent_node); + if (!children) + return create_index(row, column, (void*)get_internal_id(m_dom_tree)); + auto const& child_node = children->at(row).as_object(); auto child_internal_id = (void*)get_internal_id(child_node); return create_index(row, column, child_internal_id); } @@ -45,32 +47,33 @@ GUI::ModelIndex DOMTreeJSONModel::parent_index(const GUI::ModelIndex& index) con if (!index.is_valid()) return {}; - auto node = find_node(index); - + auto const& node = find_node(index); auto node_internal_id = get_internal_id(node); - auto parent_node = find_parent_of_child_with_internal_id(node_internal_id); - if (!parent_node.has_value()) + + auto const* parent_node = find_parent_of_child_with_internal_id(node_internal_id); + if (!parent_node) return {}; - auto parent_node_internal_id = get_internal_id(parent_node.value()); // If the parent is the root document, we know it has index 0, 0 + auto parent_node_internal_id = get_internal_id(*parent_node); if (parent_node_internal_id == get_internal_id(m_dom_tree)) { return create_index(0, 0, (void*)parent_node_internal_id); } // Otherwise, we need to find the grandparent, to find the index of parent within that - auto grandparent_node = find_parent_of_child_with_internal_id(parent_node_internal_id); - VERIFY(grandparent_node.has_value()); + auto const* grandparent_node = find_parent_of_child_with_internal_id(parent_node_internal_id); + VERIFY(grandparent_node); - auto grandparent_children = get_children(*grandparent_node); - if (grandparent_children.is_empty()) + auto const* grandparent_children = get_children(*grandparent_node); + if (!grandparent_children) return {}; - for (size_t grandparent_child_index = 0; grandparent_child_index < grandparent_children.size(); ++grandparent_child_index) { - auto child = grandparent_children[grandparent_child_index].as_object(); + for (size_t grandparent_child_index = 0; grandparent_child_index < grandparent_children->size(); ++grandparent_child_index) { + auto const& child = grandparent_children->at(grandparent_child_index).as_object(); if (get_internal_id(child) == parent_node_internal_id) return create_index(grandparent_child_index, 0, (void*)(parent_node_internal_id)); } + return {}; } @@ -79,8 +82,9 @@ int DOMTreeJSONModel::row_count(const GUI::ModelIndex& index) const if (!index.is_valid()) return 1; - auto child = find_node(index); - return get_children(child).size(); + auto const& node = find_node(index); + auto const* children = get_children(node); + return children ? children->size() : 0; } int DOMTreeJSONModel::column_count(const GUI::ModelIndex&) const @@ -111,7 +115,7 @@ static String with_whitespace_collapsed(const StringView& string) GUI::Variant DOMTreeJSONModel::data(const GUI::ModelIndex& index, GUI::ModelRole role) const { - auto node = find_node(index); + auto const& node = find_node(index); auto node_name = node.get("name").as_string(); auto type = node.get("type").as_string_or("unknown"); @@ -154,47 +158,55 @@ void DOMTreeJSONModel::update() did_update(); } -Optional DOMTreeJSONModel::find_parent_of_child_with_internal_id(size_t internal_id) const +JsonObject const* DOMTreeJSONModel::find_parent_of_child_with_internal_id(size_t internal_id) const { return find_parent_of_child_with_internal_id(m_dom_tree, internal_id); } -Optional DOMTreeJSONModel::find_parent_of_child_with_internal_id(JsonObject node, size_t internal_id) const +JsonObject const* DOMTreeJSONModel::find_parent_of_child_with_internal_id(JsonObject const& node, size_t internal_id) const { - auto children = get_children(node); + auto const* children = get_children(node); + if (!children) + return nullptr; + + for (size_t i = 0; i < children->size(); ++i) { + auto const& child = children->at(i).as_object(); - for (size_t i = 0; i < children.size(); ++i) { - auto child = children[i].as_object(); auto child_internal_id = get_internal_id(child); if (child_internal_id == internal_id) - return node; - auto maybe_node = find_parent_of_child_with_internal_id(child, internal_id); - if (maybe_node.has_value()) + return &node; + + if (auto const* maybe_node = find_parent_of_child_with_internal_id(child, internal_id); maybe_node) return maybe_node; } - return {}; + + return nullptr; } -Optional DOMTreeJSONModel::find_child_with_internal_id(size_t internal_id) const +JsonObject const* DOMTreeJSONModel::find_child_with_internal_id(size_t internal_id) const { return find_child_with_internal_id(m_dom_tree, internal_id); } -Optional DOMTreeJSONModel::find_child_with_internal_id(JsonObject node, size_t internal_id) const +JsonObject const* DOMTreeJSONModel::find_child_with_internal_id(JsonObject const& node, size_t internal_id) const { auto node_internal_id = get_internal_id(node); if (node_internal_id == internal_id) { - return node; + return &node; } - auto children = get_children(node); - for (size_t i = 0; i < children.size(); ++i) { - auto child = children[i].as_object(); - auto maybe_node = find_child_with_internal_id(child, internal_id); - if (maybe_node.has_value()) + auto const* children = get_children(node); + if (!children) + return nullptr; + + for (size_t i = 0; i < children->size(); ++i) { + auto const& child = children->at(i).as_object(); + + if (auto const* maybe_node = find_child_with_internal_id(child, internal_id); maybe_node) return maybe_node; } - return {}; + + return nullptr; } size_t DOMTreeJSONModel::get_internal_id(JsonObject const& o) @@ -202,24 +214,22 @@ size_t DOMTreeJSONModel::get_internal_id(JsonObject const& o) return o.get("internal_id").as_u32(); } -JsonArray DOMTreeJSONModel::get_children(JsonObject const& o) +JsonArray const* DOMTreeJSONModel::get_children(JsonObject const& o) { - auto maybe_children = o.get("children"); - if (maybe_children.is_null()) - return {}; - return maybe_children.as_array(); + if (auto const* maybe_children = o.get_ptr("children"); maybe_children) + return &maybe_children->as_array(); + return nullptr; } -JsonObject DOMTreeJSONModel::find_node(GUI::ModelIndex index) const +JsonObject const& DOMTreeJSONModel::find_node(GUI::ModelIndex const& index) const { auto internal_id = (size_t)(index.internal_data()); - auto maybe_node = find_child_with_internal_id(internal_id); - if (!maybe_node.has_value()) { - dbgln("Failed to find node with internal_id={}", internal_id); - VERIFY_NOT_REACHED(); - } - return maybe_node.value(); + if (auto const* maybe_node = find_child_with_internal_id(internal_id); maybe_node) + return *maybe_node; + + dbgln("Failed to find node with internal_id={}", internal_id); + VERIFY_NOT_REACHED(); } } diff --git a/Userland/Libraries/LibWeb/DOMTreeJSONModel.h b/Userland/Libraries/LibWeb/DOMTreeJSONModel.h index a0a6e17950..d81896a9d1 100644 --- a/Userland/Libraries/LibWeb/DOMTreeJSONModel.h +++ b/Userland/Libraries/LibWeb/DOMTreeJSONModel.h @@ -36,16 +36,16 @@ public: private: explicit DOMTreeJSONModel(JsonObject); - Optional find_parent_of_child_with_internal_id(size_t) const; - Optional find_parent_of_child_with_internal_id(JsonObject, size_t) const; + JsonObject const* find_parent_of_child_with_internal_id(size_t) const; + JsonObject const* find_parent_of_child_with_internal_id(JsonObject const&, size_t) const; - Optional find_child_with_internal_id(size_t) const; - Optional find_child_with_internal_id(JsonObject, size_t) const; + JsonObject const* find_child_with_internal_id(size_t) const; + JsonObject const* find_child_with_internal_id(JsonObject const&, size_t) const; - JsonObject find_node(GUI::ModelIndex) const; + JsonObject const& find_node(GUI::ModelIndex const&) const; static size_t get_internal_id(const JsonObject& o); - static JsonArray get_children(const JsonObject& o); + static JsonArray const* get_children(const JsonObject& o); GUI::Icon m_document_icon; GUI::Icon m_element_icon;