From 2de56622d5c403c7149015150a03c229c5d242f1 Mon Sep 17 00:00:00 2001 From: Karl Seguin Date: Thu, 7 May 2026 08:14:03 +0800 Subject: [PATCH 01/12] Track DOM version on the page This is a follow up to https://github.com/lightpanda-io/browser/pull/2365 which implements a different fix with different performance tradeoffs. To recap, #2365 fixed an issue where the frame used to track the dom version was the frame here the JS was being executed, not necessarily where the node existed. This would cause the caching to break. #2365 fixed this by always using the node's frame. But this is relatively expensive, requiring that we find the owning document of a node in order to get its frame. This traversal has to happen on every dom manipulation (to update the version) and every cached read (to verify the dom version hasn't changed). We _could_ store the owning frame on every Node, but that's relatively memory expensive to do. This commit moves the dom version from the Frame to the Page. This allows us to use the executing Frame to get the page (`frame._page`) which is much faster than traversing the parent nodes to find the document. The downside is that a mutation in Frame1 will invalidate the cache in Frame2. Given that many sites have a single frame, this seems like a clear win. --- src/browser/Frame.zig | 9 +++------ src/browser/Page.zig | 13 ++++++++++++ src/browser/webapi/Document.zig | 6 +++--- src/browser/webapi/DocumentFragment.zig | 2 +- src/browser/webapi/Element.zig | 8 ++++---- src/browser/webapi/Node.zig | 19 +++++------------- src/browser/webapi/Range.zig | 2 +- src/browser/webapi/collections/ChildNodes.zig | 20 +++++++------------ .../webapi/collections/HTMLAllCollection.zig | 9 +++------ src/browser/webapi/collections/node_live.zig | 11 ++++------ src/browser/webapi/element/Attribute.zig | 4 ++-- src/browser/webapi/element/Html.zig | 2 +- src/browser/webapi/element/html/Option.zig | 2 +- 13 files changed, 48 insertions(+), 59 deletions(-) diff --git a/src/browser/Frame.zig b/src/browser/Frame.zig index 7ea7474c..4b5c60c9 100644 --- a/src/browser/Frame.zig +++ b/src/browser/Frame.zig @@ -243,9 +243,6 @@ child_frames: std.ArrayList(*Frame) = .{}, // Workers created by this frame. Cleaned up when frame is destroyed. workers: std.ArrayList(*Worker) = .{}, -// DOM version used to invalidate cached state of "live" collections -version: usize = 0, - // This is maybe not great. It's a counter on the number of events that we're // waiting on before triggering the "load" event. Essentially, we need all // synchronous scripts and all iframes to be loaded. Scripts are handled by the @@ -1439,7 +1436,7 @@ pub fn openPopup(self: *Frame, opts: OpenPopupOpts) !*Frame { } pub fn domChanged(self: *Frame) void { - self.version += 1; + self._page.dom_version += 1; if (self._intersection_check_scheduled) { return; @@ -2977,7 +2974,7 @@ pub fn appendNode(self: *Frame, parent: *Node, child: *Node, opts: InsertNodeOpt } pub fn appendAllChildren(self: *Frame, parent: *Node, target: *Node) !void { - target.bumpDomVersion(self); + self.domChanged(); const dest_connected = target.isConnected(); // Use firstChild() instead of iterator to handle cases where callbacks @@ -2992,7 +2989,7 @@ pub fn appendAllChildren(self: *Frame, parent: *Node, target: *Node) !void { } pub fn insertAllChildrenBefore(self: *Frame, fragment: *Node, parent: *Node, ref_node: *Node) !void { - parent.bumpDomVersion(self); + self.domChanged(); const dest_connected = parent.isConnected(); // Use firstChild() instead of iterator to handle cases where callbacks diff --git a/src/browser/Page.zig b/src/browser/Page.zig index 4a35ff17..0205f8da 100644 --- a/src/browser/Page.zig +++ b/src/browser/Page.zig @@ -45,6 +45,19 @@ const Page = @This(); session: *Session, +// DOM version used to invalidate cached state of "live" collections. Ideally +// this would be on the Frame (and that's where it used to be). But getting the +// frame from a DOM mutation call is [relatively] expensive. You can't use +// the bridge-injected *Frame, because that's the frame where the JS is being +// executed, which might not be the *Frame that owns the node. We don't store +// *Frame in node (think of the memory!), so we have to iterate through its +// parents, find the Document, which has the frame. +// So the choice is between making every DOM mutation (which has to increase +// the dom_version) + every read (which has to check the version) slow, or +// putting this on the Page, and having an DOM mutation in Frame 1 invalidate +// a cached lookup on Frame 2. We picked the latter. +dom_version: usize = 0, + // DOM object factory scoped to this Page's documents. factory: Factory, diff --git a/src/browser/webapi/Document.zig b/src/browser/webapi/Document.zig index b18211f5..747e08c2 100644 --- a/src/browser/webapi/Document.zig +++ b/src/browser/webapi/Document.zig @@ -489,7 +489,7 @@ pub fn append(self: *Document, nodes: []const Node.NodeOrText, frame: *Frame) !v try validateDocumentNodes(self, nodes, false); const parent = self.asNode(); - parent.bumpDomVersion(frame); + frame.domChanged(); const parent_is_connected = parent.isConnected(); for (nodes) |node_or_text| { @@ -514,7 +514,7 @@ pub fn prepend(self: *Document, nodes: []const Node.NodeOrText, frame: *Frame) ! try validateDocumentNodes(self, nodes, false); const parent = self.asNode(); - parent.bumpDomVersion(frame); + frame.domChanged(); const parent_is_connected = parent.isConnected(); var i = nodes.len; @@ -736,7 +736,7 @@ fn writeInternal(self: *Document, text: []const []const u8, append_newline: bool insert_after = child; } - parent.bumpDomVersion(frame); + frame.domChanged(); self._write_insertion_point = children_to_insert.getLast(); } diff --git a/src/browser/webapi/DocumentFragment.zig b/src/browser/webapi/DocumentFragment.zig index 106cb427..186bc68a 100644 --- a/src/browser/webapi/DocumentFragment.zig +++ b/src/browser/webapi/DocumentFragment.zig @@ -154,7 +154,7 @@ pub fn getInnerHTML(self: *DocumentFragment, writer: *std.Io.Writer, frame: *Fra pub fn setInnerHTML(self: *DocumentFragment, html: []const u8, frame: *Frame) !void { const parent = self.asNode(); - parent.bumpDomVersion(frame); + frame.domChanged(); var it = parent.childrenIterator(); while (it.next()) |child| { frame.removeNode(parent, child, .{ .will_be_reconnected = false }); diff --git a/src/browser/webapi/Element.zig b/src/browser/webapi/Element.zig index 91f47758..4de1a732 100644 --- a/src/browser/webapi/Element.zig +++ b/src/browser/webapi/Element.zig @@ -467,7 +467,7 @@ pub fn setOuterHTML(self: *Element, html: []const u8, frame: *Frame) !void { const node = self.asNode(); const parent = node._parent orelse return; - parent.bumpDomVersion(frame); + frame.domChanged(); if (html.len > 0) { const fragment = (try Node.DocumentFragment.init(frame)).asNode(); try frame.parseHtmlAsChildren(fragment, html); @@ -493,7 +493,7 @@ pub fn setInnerHTML(self: *Element, html: []const u8, frame: *Frame) !void { // fires disconnectedCallback for custom elements, which can mutate // the child list and dangle any cached next-pointer the iterator // would otherwise hold. - parent.bumpDomVersion(frame); + frame.domChanged(); while (parent.firstChild()) |child| { frame.removeNode(parent, child, .{ .will_be_reconnected = false }); } @@ -881,7 +881,7 @@ pub fn replaceChildren(self: *Element, nodes: []const Node.NodeOrText, frame: *F pub fn replaceWith(self: *Element, nodes: []const Node.NodeOrText, frame: *Frame) !void { const ref_node = self.asNode(); const parent = ref_node._parent orelse return; - parent.bumpDomVersion(frame); + frame.domChanged(); const parent_is_connected = parent.isConnected(); @@ -920,7 +920,7 @@ pub fn replaceWith(self: *Element, nodes: []const Node.NodeOrText, frame: *Frame pub fn remove(self: *Element, frame: *Frame) void { const node = self.asNode(); const parent = node._parent orelse return; - parent.bumpDomVersion(frame); + frame.domChanged(); frame.removeNode(parent, node, .{ .will_be_reconnected = false }); } diff --git a/src/browser/webapi/Node.zig b/src/browser/webapi/Node.zig index 8e8eea21..7df1fd6a 100644 --- a/src/browser/webapi/Node.zig +++ b/src/browser/webapi/Node.zig @@ -226,7 +226,7 @@ pub fn appendChild(self: *Node, child: *Node, frame: *Frame) !*Node { try validateNodeInsertion(self, child); - self.bumpDomVersion(frame); + frame.domChanged(); // If the child is currently connected, and if its new parent is connected, // then we can remove + add a bit more efficiently (we don't have to fully @@ -532,15 +532,6 @@ pub fn ownerFrame(self: *const Node, default: *Frame) *Frame { return doc._frame orelse default; } -// Tells the owning frame that this node's subtree has changed. Use this from -// mutation paths instead of `frame.domChanged()` so cross-realm mutations -// (e.g., parent JS mutating an iframe's DOM) bump the iframe's version, not -// the caller's. Otherwise, live collections that key off the owning frame's -// version won't see the change and will return stale cached state. -pub fn bumpDomVersion(self: *const Node, default: *Frame) void { - self.ownerFrame(default).domChanged(); -} - pub const ResolveURLOpts = struct { allocator: ?Allocator = null, }; @@ -572,7 +563,7 @@ pub fn removeChild(self: *Node, child: *Node, frame: *Frame) !*Node { var it = self.childrenIterator(); while (it.next()) |n| { if (n == child) { - self.bumpDomVersion(frame); + frame.domChanged(); frame.removeNode(self, child, .{ .will_be_reconnected = false }); return child; } @@ -587,7 +578,7 @@ pub fn insertBefore(self: *Node, new_node: *Node, ref_node_: ?*Node, frame: *Fra // special case: if nodes are the same, ignore the change. if (new_node == ref_node_) { - self.bumpDomVersion(frame); + frame.domChanged(); if (frame.hasMutationObservers()) { const parent = new_node._parent.?; @@ -618,7 +609,7 @@ pub fn insertBefore(self: *Node, new_node: *Node, ref_node_: ?*Node, frame: *Fra const parent_owner = self.ownerDocument(frame) orelse self.as(Document); const adopting_to_new_document = child_owner != null and child_owner.? != parent_owner; - self.bumpDomVersion(frame); + frame.domChanged(); const will_be_reconnected = self.isConnected() and !adopting_to_new_document; if (new_node._parent) |parent| { frame.removeNode(parent, new_node, .{ .will_be_reconnected = will_be_reconnected }); @@ -1089,7 +1080,7 @@ pub fn replaceChildren(self: *Node, nodes: []const NodeOrText, frame: *Frame) !v } } - self.bumpDomVersion(frame); + frame.domChanged(); // Remove all existing children var it = self.childrenIterator(); diff --git a/src/browser/webapi/Range.zig b/src/browser/webapi/Range.zig index 686c69df..c1eb3fde 100644 --- a/src/browser/webapi/Range.zig +++ b/src/browser/webapi/Range.zig @@ -374,7 +374,7 @@ pub fn deleteContents(self: *Range, frame: *Frame) !void { if (self._proto.getCollapsed()) { return; } - self._proto._start_container.bumpDomVersion(frame); + frame.domChanged(); // Simple case: same container if (self._proto._start_container == self._proto._end_container) { diff --git a/src/browser/webapi/collections/ChildNodes.zig b/src/browser/webapi/collections/ChildNodes.zig index c94e7e53..77cabaa5 100644 --- a/src/browser/webapi/collections/ChildNodes.zig +++ b/src/browser/webapi/collections/ChildNodes.zig @@ -34,11 +34,8 @@ _arena: std.mem.Allocator, _last_index: usize, _last_length: ?u32, _last_node: ?*std.DoublyLinkedList.Node, -// Version observed on `_owning_frame` the last time we refreshed our cache. -// Compare against `_owning_frame.version` to detect DOM mutations. _cached_version: usize, _node: *Node, -_owning_frame: *Frame, pub const KeyIterator = GenericIterator(Iterator, "0"); pub const ValueIterator = GenericIterator(Iterator, "1"); @@ -48,8 +45,6 @@ pub fn init(node: *Node, frame: *Frame) !*ChildNodes { const arena = try frame.getArena(.small, "ChildNodes"); errdefer frame.releaseArena(arena); - const owning_frame = node.ownerFrame(frame); - const self = try arena.create(ChildNodes); self.* = .{ ._node = node, @@ -57,8 +52,7 @@ pub fn init(node: *Node, frame: *Frame) !*ChildNodes { ._last_index = 0, ._last_node = null, ._last_length = null, - ._cached_version = owning_frame.version, - ._owning_frame = owning_frame, + ._cached_version = frame._page.dom_version, }; return self; } @@ -67,8 +61,8 @@ pub fn deinit(self: *const ChildNodes, page: *Page) void { page.releaseArena(self._arena); } -pub fn length(self: *ChildNodes, _: *Frame) !u32 { - if (self.versionCheck()) { +pub fn length(self: *ChildNodes, frame: *const Frame) !u32 { + if (self.versionCheck(frame)) { if (self._last_length) |cached_length| { return cached_length; } @@ -81,8 +75,8 @@ pub fn length(self: *ChildNodes, _: *Frame) !u32 { return len; } -pub fn getAtIndex(self: *ChildNodes, index: usize, _: *Frame) !?*Node { - _ = self.versionCheck(); +pub fn getAtIndex(self: *ChildNodes, index: usize, frame: *const Frame) !?*Node { + _ = self.versionCheck(frame); var current = self._last_index; var node: ?*std.DoublyLinkedList.Node = null; @@ -122,8 +116,8 @@ pub fn entries(self: *ChildNodes, frame: *Frame) !*EntryIterator { return .init(.{ .list = self }, frame); } -fn versionCheck(self: *ChildNodes) bool { - const current = self._owning_frame.version; +fn versionCheck(self: *ChildNodes, frame: *const Frame) bool { + const current = frame._page.dom_version; if (current == self._cached_version) { return true; } diff --git a/src/browser/webapi/collections/HTMLAllCollection.zig b/src/browser/webapi/collections/HTMLAllCollection.zig index 7e9758b7..48e2952f 100644 --- a/src/browser/webapi/collections/HTMLAllCollection.zig +++ b/src/browser/webapi/collections/HTMLAllCollection.zig @@ -32,21 +32,18 @@ _tw: TreeWalker.FullExcludeSelf, _last_index: usize, _last_length: ?u32, _cached_version: usize, -_owning_frame: *Frame, pub fn init(root: *Node, frame: *Frame) HTMLAllCollection { - const owning_frame = root.ownerFrame(frame); return .{ ._last_index = 0, ._last_length = null, ._tw = TreeWalker.FullExcludeSelf.init(root, .{}), - ._cached_version = owning_frame.version, - ._owning_frame = owning_frame, + ._cached_version = frame._page.dom_version, }; } -fn versionCheck(self: *HTMLAllCollection, _: *const Frame) bool { - const current = self._owning_frame.version; +fn versionCheck(self: *HTMLAllCollection, frame: *const Frame) bool { + const current = frame._page.dom_version; if (self._cached_version != current) { self._cached_version = current; self._last_index = 0; diff --git a/src/browser/webapi/collections/node_live.zig b/src/browser/webapi/collections/node_live.zig index 33962d8e..73b19714 100644 --- a/src/browser/webapi/collections/node_live.zig +++ b/src/browser/webapi/collections/node_live.zig @@ -79,7 +79,7 @@ const Filters = union(Mode) { // But, if the version hasn't changed, then we can leverage other stateful data // to improve performance. For example, we cache the length property. So once // we've walked the tree to figure the length, we can re-use the cached property -// if the DOM is unchanged (i.e. if our _cached_version == frame.version). +// if the DOM is unchanged (i.e. if our _cached_version == page.dom_version). // // We do something similar for indexed getter (e.g. coll[4]), by preserving the // last node visited in the tree (implicitly by not resetting the TreeWalker). @@ -99,19 +99,16 @@ pub fn NodeLive(comptime mode: Mode) type { _last_index: usize, _last_length: ?u32, _cached_version: usize, - _owning_frame: *Frame, const Self = @This(); pub fn init(root: *Node, filter: Filter, frame: *Frame) Self { - const owning_frame = root.ownerFrame(frame); return .{ ._last_index = 0, ._last_length = null, ._filter = filter, ._tw = TW.init(root, .{}), - ._cached_version = owning_frame.version, - ._owning_frame = owning_frame, + ._cached_version = frame._page.dom_version, }; } @@ -345,8 +342,8 @@ pub fn NodeLive(comptime mode: Mode) type { }; } - fn versionCheck(self: *Self, _: *const Frame) bool { - const current = self._owning_frame.version; + fn versionCheck(self: *Self, frame: *const Frame) bool { + const current = frame._page.dom_version; if (current == self._cached_version) { return true; } diff --git a/src/browser/webapi/element/Attribute.zig b/src/browser/webapi/element/Attribute.zig index 24b6477c..c0d4d053 100644 --- a/src/browser/webapi/element/Attribute.zig +++ b/src/browser/webapi/element/Attribute.zig @@ -229,7 +229,7 @@ pub const List = struct { }; try frame.addElementId(parent, element, entry._value.str()); } - element.asNode().bumpDomVersion(frame); + frame.domChanged(); frame.attributeChange(element, result.normalized, entry._value, old_value); return entry; } @@ -292,7 +292,7 @@ pub const List = struct { frame.removeElementId(element, entry._value.str()); } - element.asNode().bumpDomVersion(frame); + frame.domChanged(); frame.attributeRemove(element, result.normalized, old_value); _ = frame._attribute_lookup.remove(@intFromPtr(entry)); self._list.remove(&entry._node); diff --git a/src/browser/webapi/element/Html.zig b/src/browser/webapi/element/Html.zig index ffaba715..3ed96f57 100644 --- a/src/browser/webapi/element/Html.zig +++ b/src/browser/webapi/element/Html.zig @@ -273,7 +273,7 @@ pub fn setInnerText(self: *HtmlElement, text: []const u8, frame: *Frame) !void { const parent = self.asElement().asNode(); // Remove all existing children - parent.bumpDomVersion(frame); + frame.domChanged(); var it = parent.childrenIterator(); while (it.next()) |child| { frame.removeNode(parent, child, .{ .will_be_reconnected = false }); diff --git a/src/browser/webapi/element/html/Option.zig b/src/browser/webapi/element/html/Option.zig index 494573cf..9ca53f25 100644 --- a/src/browser/webapi/element/html/Option.zig +++ b/src/browser/webapi/element/html/Option.zig @@ -80,7 +80,7 @@ pub fn setSelected(self: *Option, selected: bool, frame: *Frame) !void { // TODO: When setting selected=true, may need to unselect other options // in the parent