From 315c9a2d92b3fe2c81d7947afd77a443d96f24d2 Mon Sep 17 00:00:00 2001 From: Karl Seguin Date: Fri, 27 Feb 2026 10:17:30 +0800 Subject: [PATCH] Add RC support to NodeList Most importantly, this allows the Selector.List to be self-contained with an arena from the ArenaPool. Selector.List can be both relatively large and relatively common, so moving it off the page.arena is a nice win. Also applied this to ChildNodes, which is much smaller but could also be called often. I was initially going to hook into the v8::Object's internal fields to store the referencing v8::Object. So the v8::Object representing the Iterator would store the v8::Object representing the NodeList inside of its internal field - which the GC would trace/detect/respect. And that is probably the fastest and most v8-ish solution, but I couldn't come up with an elegant solution. The best I had was having a "afterCreate" callback which passed the v8 object (this is similar to the old postAttach callback we had, but used for a different purpose). However, since "acquireRef" was recently added to events, re-using that was much simpler and worked well. --- .../tests/document/query_selector_all.html | 26 ++++++++++++ src/browser/webapi/collections/ChildNodes.zig | 17 ++++++-- .../HTMLFormControlsCollection.zig | 2 +- src/browser/webapi/collections/NodeList.zig | 42 ++++++++++++++----- src/browser/webapi/collections/iterator.zig | 17 +++++++- src/browser/webapi/collections/node_live.zig | 2 +- src/browser/webapi/selector/List.zig | 12 +++++- src/browser/webapi/selector/Selector.zig | 10 +++-- src/cdp/Node.zig | 30 +++++++++---- src/cdp/domains/dom.zig | 4 ++ 10 files changed, 131 insertions(+), 31 deletions(-) diff --git a/src/browser/tests/document/query_selector_all.html b/src/browser/tests/document/query_selector_all.html index 4e46f7e3..ae95ed9e 100644 --- a/src/browser/tests/document/query_selector_all.html +++ b/src/browser/tests/document/query_selector_all.html @@ -442,3 +442,29 @@ } + diff --git a/src/browser/webapi/collections/ChildNodes.zig b/src/browser/webapi/collections/ChildNodes.zig index fa65a3fb..5ba6256a 100644 --- a/src/browser/webapi/collections/ChildNodes.zig +++ b/src/browser/webapi/collections/ChildNodes.zig @@ -26,6 +26,7 @@ const GenericIterator = @import("iterator.zig").Entry; // No need to go through a TreeWalker or add any filtering. const ChildNodes = @This(); +_arena: std.mem.Allocator, _last_index: usize, _last_length: ?u32, _last_node: ?*std.DoublyLinkedList.Node, @@ -37,13 +38,23 @@ pub const ValueIterator = GenericIterator(Iterator, "1"); pub const EntryIterator = GenericIterator(Iterator, null); pub fn init(node: *Node, page: *Page) !*ChildNodes { - return page._factory.create(ChildNodes{ + const arena = try page.getArena(.{ .debug = "ChildNodes" }); + errdefer page.releaseArena(arena); + + const self = try arena.create(ChildNodes); + self.* = .{ ._node = node, + ._arena = arena, ._last_index = 0, ._last_node = null, ._last_length = null, ._cached_version = page.version, - }); + }; + return self; +} + +pub fn deinit(self: *const ChildNodes, page: *Page) void { + page.releaseArena(self._arena); } pub fn length(self: *ChildNodes, page: *Page) !u32 { @@ -115,7 +126,7 @@ fn versionCheck(self: *ChildNodes, page: *Page) bool { const NodeList = @import("NodeList.zig"); pub fn runtimeGenericWrap(self: *ChildNodes, page: *Page) !*NodeList { - return page._factory.create(NodeList{ .data = .{ .child_nodes = self } }); + return page._factory.create(NodeList{ ._data = .{ .child_nodes = self } }); } const Iterator = struct { diff --git a/src/browser/webapi/collections/HTMLFormControlsCollection.zig b/src/browser/webapi/collections/HTMLFormControlsCollection.zig index fe5f1a97..c6b68fe0 100644 --- a/src/browser/webapi/collections/HTMLFormControlsCollection.zig +++ b/src/browser/webapi/collections/HTMLFormControlsCollection.zig @@ -85,7 +85,7 @@ pub fn namedItem(self: *HTMLFormControlsCollection, name: []const u8, page: *Pag ._name = try page.dupeString(name), }); - radio_node_list._proto = try page._factory.create(NodeList{ .data = .{ .radio_node_list = radio_node_list } }); + radio_node_list._proto = try page._factory.create(NodeList{ ._data = .{ .radio_node_list = radio_node_list } }); return .{ .radio_node_list = radio_node_list }; } diff --git a/src/browser/webapi/collections/NodeList.zig b/src/browser/webapi/collections/NodeList.zig index 2f236a27..0237a76c 100644 --- a/src/browser/webapi/collections/NodeList.zig +++ b/src/browser/webapi/collections/NodeList.zig @@ -28,24 +28,36 @@ const RadioNodeList = @import("RadioNodeList.zig"); const SelectorList = @import("../selector/List.zig"); const NodeLive = @import("node_live.zig").NodeLive; -const Mode = enum { - child_nodes, - selector_list, - radio_node_list, - name, -}; - const NodeList = @This(); -data: union(Mode) { +_data: union(enum) { child_nodes: *ChildNodes, selector_list: *SelectorList, radio_node_list: *RadioNodeList, name: NodeLive(.name), }, +_rc: usize = 0, + +pub fn deinit(self: *NodeList, _: bool, page: *Page) void { + const rc = self._rc; + if (rc > 1) { + self._rc = rc - 1; + return; + } + + switch (self._data) { + .selector_list => |list| list.deinit(page), + .child_nodes => |cn| cn.deinit(page), + else => {}, + } +} + +pub fn acquireRef(self: *NodeList) void { + self._rc += 1; +} pub fn length(self: *NodeList, page: *Page) !u32 { - return switch (self.data) { + return switch (self._data) { .child_nodes => |impl| impl.length(page), .selector_list => |impl| @intCast(impl.getLength()), .radio_node_list => |impl| impl.getLength(), @@ -58,7 +70,7 @@ pub fn indexedGet(self: *NodeList, index: usize, page: *Page) !*Node { } pub fn getAtIndex(self: *NodeList, index: usize, page: *Page) !?*Node { - return switch (self.data) { + return switch (self._data) { .child_nodes => |impl| impl.getAtIndex(index, page), .selector_list => |impl| impl.getAtIndex(index), .radio_node_list => |impl| impl.getAtIndex(index, page), @@ -106,6 +118,14 @@ const Iterator = struct { const Entry = struct { u32, *Node }; + pub fn deinit(self: *Iterator, shutdown: bool, page: *Page) void { + self.list.deinit(shutdown, page); + } + + pub fn acquireRef(self: *Iterator) void { + self.list.acquireRef(); + } + pub fn next(self: *Iterator, page: *Page) !?Entry { const index = self.index; const node = try self.list.getAtIndex(index, page) orelse return null; @@ -122,6 +142,8 @@ pub const JsApi = struct { pub const prototype_chain = bridge.prototypeChain(); pub var class_id: bridge.ClassId = undefined; pub const enumerable = false; + pub const weak = true; + pub const finalizer = bridge.finalizer(NodeList.deinit); }; pub const length = bridge.accessor(NodeList.length, null, .{}); diff --git a/src/browser/webapi/collections/iterator.zig b/src/browser/webapi/collections/iterator.zig index 0c063e84..3c43f3f8 100644 --- a/src/browser/webapi/collections/iterator.zig +++ b/src/browser/webapi/collections/iterator.zig @@ -21,8 +21,7 @@ const js = @import("../../js/js.zig"); const Page = @import("../../Page.zig"); pub fn Entry(comptime Inner: type, comptime field: ?[]const u8) type { - const InnerStruct = Inner; - const R = reflect(InnerStruct, field); + const R = reflect(Inner, field); return struct { inner: Inner, @@ -40,6 +39,18 @@ pub fn Entry(comptime Inner: type, comptime field: ?[]const u8) type { return page._factory.create(Self{ .inner = inner }); } + pub fn deinit(self: *Self, shutdown: bool, page: *Page) void { + if (@hasDecl(Inner, "deinit")) { + self.inner.deinit(shutdown, page); + } + } + + pub fn acquireRef(self: *Self) void { + if (@hasDecl(Inner, "acquireRef")) { + self.inner.acquireRef(); + } + } + pub fn next(self: *Self, page: *Page) if (R.has_error_return) anyerror!Result else Result { const entry = (if (comptime R.has_error_return) try self.inner.next(page) else self.inner.next(page)) orelse { return .{ .done = true, .value = null }; @@ -61,6 +72,8 @@ pub fn Entry(comptime Inner: type, comptime field: ?[]const u8) type { pub const Meta = struct { pub const prototype_chain = bridge.prototypeChain(); pub var class_id: bridge.ClassId = undefined; + pub const weak = true; + pub const finalizer = bridge.finalizer(Self.deinit); }; pub const next = bridge.function(Self.next, .{ .null_as_undefined = true }); diff --git a/src/browser/webapi/collections/node_live.zig b/src/browser/webapi/collections/node_live.zig index 5f6bd330..65fdfbf7 100644 --- a/src/browser/webapi/collections/node_live.zig +++ b/src/browser/webapi/collections/node_live.zig @@ -349,7 +349,7 @@ pub fn NodeLive(comptime mode: Mode) type { pub fn runtimeGenericWrap(self: Self, page: *Page) !if (mode == .name) *NodeList else *HTMLCollection { const collection = switch (mode) { - .name => return page._factory.create(NodeList{ .data = .{ .name = self } }), + .name => return page._factory.create(NodeList{ ._data = .{ .name = self } }), .tag => HTMLCollection{ ._data = .{ .tag = self } }, .tag_name => HTMLCollection{ ._data = .{ .tag_name = self } }, .tag_name_ns => HTMLCollection{ ._data = .{ .tag_name_ns = self } }, diff --git a/src/browser/webapi/selector/List.zig b/src/browser/webapi/selector/List.zig index 3ed295c4..04055910 100644 --- a/src/browser/webapi/selector/List.zig +++ b/src/browser/webapi/selector/List.zig @@ -40,6 +40,10 @@ pub const EntryIterator = GenericIterator(Iterator, null); pub const KeyIterator = GenericIterator(Iterator, "0"); pub const ValueIterator = GenericIterator(Iterator, "1"); +pub fn deinit(self: *const List, page: *Page) void { + page.releaseArena(self._arena); +} + pub fn collect( allocator: std.mem.Allocator, root: *Node, @@ -207,8 +211,12 @@ pub fn getAtIndex(self: *const List, index: usize) !?*Node { } const NodeList = @import("../collections/NodeList.zig"); -pub fn runtimeGenericWrap(self: *List, page: *Page) !*NodeList { - return page._factory.create(NodeList{ .data = .{ .selector_list = self } }); +pub fn runtimeGenericWrap(self: *List, _: *const Page) !*NodeList { + const nl = try self._arena.create(NodeList); + nl.* = .{ + ._data = .{ .selector_list = self }, + }; + return nl; } const IdAnchor = struct { diff --git a/src/browser/webapi/selector/Selector.zig b/src/browser/webapi/selector/Selector.zig index 4a236fc3..3e987a92 100644 --- a/src/browser/webapi/selector/Selector.zig +++ b/src/browser/webapi/selector/Selector.zig @@ -62,7 +62,9 @@ pub fn querySelectorAll(root: *Node, input: []const u8, page: *Page) !*List { return error.SyntaxError; } - const arena = page.arena; + const arena = try page.getArena(.{ .debug = "querySelectorAll" }); + errdefer page.releaseArena(arena); + var nodes: std.AutoArrayHashMapUnmanaged(*Node, void) = .empty; const selectors = try Parser.parseList(arena, input, page); @@ -70,10 +72,12 @@ pub fn querySelectorAll(root: *Node, input: []const u8, page: *Page) !*List { try List.collect(arena, root, selector, &nodes, page); } - return page._factory.create(List{ + const list = try arena.create(List); + list.* = .{ ._arena = arena, ._nodes = nodes.keys(), - }); + }; + return list; } pub fn matches(el: *Node.Element, input: []const u8, page: *Page) !bool { diff --git a/src/cdp/Node.zig b/src/cdp/Node.zig index 885fa93b..6a7bb114 100644 --- a/src/cdp/Node.zig +++ b/src/cdp/Node.zig @@ -400,20 +400,32 @@ test "cdp Node: search list" { defer page._session.removePage(); var doc = page.window._document; - const s1 = try search_list.create((try doc.querySelectorAll(.wrap("a"), page))._nodes); - try testing.expectEqual("1", s1.name); - try testing.expectEqualSlices(u32, &.{ 1, 2 }, s1.node_ids); + { + const l1 = try doc.querySelectorAll(.wrap("a"), page); + defer l1.deinit(page); + const s1 = try search_list.create(l1._nodes); + try testing.expectEqual("1", s1.name); + try testing.expectEqualSlices(u32, &.{ 1, 2 }, s1.node_ids); + } try testing.expectEqual(2, registry.lookup_by_id.count()); try testing.expectEqual(2, registry.lookup_by_node.count()); - const s2 = try search_list.create((try doc.querySelectorAll(.wrap("#a1"), page))._nodes); - try testing.expectEqual("2", s2.name); - try testing.expectEqualSlices(u32, &.{1}, s2.node_ids); + { + const l2 = try doc.querySelectorAll(.wrap("#a1"), page); + defer l2.deinit(page); + const s2 = try search_list.create(l2._nodes); + try testing.expectEqual("2", s2.name); + try testing.expectEqualSlices(u32, &.{1}, s2.node_ids); + } - const s3 = try search_list.create((try doc.querySelectorAll(.wrap("#a2"), page))._nodes); - try testing.expectEqual("3", s3.name); - try testing.expectEqualSlices(u32, &.{2}, s3.node_ids); + { + const l3 = try doc.querySelectorAll(.wrap("#a2"), page); + defer l3.deinit(page); + const s3 = try search_list.create(l3._nodes); + try testing.expectEqual("3", s3.name); + try testing.expectEqualSlices(u32, &.{2}, s3.node_ids); + } try testing.expectEqual(2, registry.lookup_by_id.count()); try testing.expectEqual(2, registry.lookup_by_node.count()); diff --git a/src/cdp/domains/dom.zig b/src/cdp/domains/dom.zig index 45bcbd59..2f2befa5 100644 --- a/src/cdp/domains/dom.zig +++ b/src/cdp/domains/dom.zig @@ -98,6 +98,8 @@ fn performSearch(cmd: anytype) !void { const bc = cmd.browser_context orelse return error.BrowserContextNotLoaded; const page = bc.session.currentPage() orelse return error.PageNotLoaded; const list = try Selector.querySelectorAll(page.window._document.asNode(), params.query, page); + defer list.deinit(page); + const search = try bc.node_search_list.create(list._nodes); // dispatch setChildNodesEvents to inform the client of the subpart of node @@ -247,6 +249,8 @@ fn querySelectorAll(cmd: anytype) !void { }; const selected_nodes = try Selector.querySelectorAll(node.dom, params.selector, page); + defer selected_nodes.deinit(page); + const nodes = selected_nodes._nodes; const node_ids = try cmd.arena.alloc(Node.Id, nodes.len);