Merge pull request #2522 from ghostty-org/push-myqrrtsnwmum

terminal: refactor hyperlink memory management

src/terminal/Screen.zig

@@ -125,7 +125,7 @@ pub const Cursor = struct {
     /// the cursor page pin changes. We can't get it from the old screen
     /// state because the page may be cleared. This is heap allocated
     /// because its most likely null.
-    hyperlink: ?*Hyperlink = null,
+    hyperlink: ?*hyperlink.Hyperlink = null,
 
     /// The pointers into the page list where the cursor is currently
     /// located. This makes it faster to move the cursor.
@@ -134,7 +134,10 @@ pub const Cursor = struct {
     page_cell: *pagepkg.Cell,
 
     pub fn deinit(self: *Cursor, alloc: Allocator) void {
-        if (self.hyperlink) |link| link.destroy(alloc);
+        if (self.hyperlink) |link| {
+            link.deinit(alloc);
+            alloc.destroy(link);
+        }
     }
 };
 
@@ -182,31 +185,6 @@ pub const CharsetState = struct {
     const CharsetArray = std.EnumArray(charsets.Slots, charsets.Charset);
 };
 
-pub const Hyperlink = struct {
-    id: ?[]const u8,
-    uri: []const u8,
-
-    pub fn create(
-        alloc: Allocator,
-        uri: []const u8,
-        id: ?[]const u8,
-    ) !*Hyperlink {
-        const self = try alloc.create(Hyperlink);
-        errdefer alloc.destroy(self);
-        self.id = if (id) |v| try alloc.dupe(u8, v) else null;
-        errdefer if (self.id) |v| alloc.free(v);
-        self.uri = try alloc.dupe(u8, uri);
-        errdefer alloc.free(self.uri);
-        return self;
-    }
-
-    pub fn destroy(self: *Hyperlink, alloc: Allocator) void {
-        if (self.id) |id| alloc.free(id);
-        alloc.free(self.uri);
-        alloc.destroy(self);
-    }
-};
-
 /// Initialize a new screen.
 ///
 /// max_scrollback is the amount of scrollback to keep in bytes. This
@@ -471,10 +449,11 @@ pub fn adjustCapacity(
         self.cursor.hyperlink = null;
 
         // Re-add
-        self.startHyperlinkOnce(link.uri, link.id) catch unreachable;
+        self.startHyperlinkOnce(link.*) catch unreachable;
 
         // Remove our old link
-        link.destroy(self.alloc);
+        link.deinit(self.alloc);
+        self.alloc.destroy(link);
     }
 
     // Reload the cursor information because the pin changed.
@@ -1023,7 +1002,10 @@ fn cursorChangePin(self: *Screen, new: Pin) void {
         self.cursor.hyperlink = null;
 
         // Re-add
-        self.startHyperlink(link.uri, link.id) catch |err| {
+        self.startHyperlink(link.uri, switch (link.id) {
+            .explicit => |v| v,
+            .implicit => null,
+        }) catch |err| {
             // This shouldn't happen because startHyperlink should handle
             // resizing. This only happens if we're truly out of RAM. Degrade
             // to forgetting the hyperlink.
@@ -1031,7 +1013,8 @@ fn cursorChangePin(self: *Screen, new: Pin) void {
         };
 
         // Remove our old link
-        link.destroy(self.alloc);
+        link.deinit(self.alloc);
+        self.alloc.destroy(link);
     }
 }
 
@@ -1550,7 +1533,10 @@ fn resizeInternal(
 
     // Fix up our hyperlink if we had one.
     if (hyperlink_) |link| {
-        self.startHyperlink(link.uri, link.id) catch |err| {
+        self.startHyperlink(link.uri, switch (link.id) {
+            .explicit => |v| v,
+            .implicit => null,
+        }) catch |err| {
             // This shouldn't happen because startHyperlink should handle
             // resizing. This only happens if we're truly out of RAM. Degrade
             // to forgetting the hyperlink.
@@ -1558,7 +1544,8 @@ fn resizeInternal(
         };
 
         // Remove our old link
-        link.destroy(self.alloc);
+        link.deinit(self.alloc);
+        self.alloc.destroy(link);
     }
 }
 
@@ -1805,6 +1792,8 @@ pub fn appendGrapheme(self: *Screen, cell: *Cell, cp: u21) !void {
     };
 }
 
+pub const StartHyperlinkError = Allocator.Error || PageList.AdjustCapacityError;
+
 /// Start the hyperlink state. Future cells will be marked as hyperlinks with
 /// this state. Note that various terminal operations may clear the hyperlink
 /// state, such as switching screens (alt screen).
@@ -1812,14 +1801,29 @@ pub fn startHyperlink(
     self: *Screen,
     uri: []const u8,
     id_: ?[]const u8,
-) !void {
+) StartHyperlinkError!void {
+    // Create our pending entry.
+    const link: hyperlink.Hyperlink = .{
+        .uri = uri,
+        .id = if (id_) |id| .{
+            .explicit = id,
+        } else implicit: {
+            defer self.cursor.hyperlink_implicit_id += 1;
+            break :implicit .{ .implicit = self.cursor.hyperlink_implicit_id };
+        },
+    };
+    errdefer switch (link.id) {
+        .explicit => {},
+        .implicit => self.cursor.hyperlink_implicit_id -= 1,
+    };
+
     // Loop until we have enough page memory to add the hyperlink
     while (true) {
-        if (self.startHyperlinkOnce(uri, id_)) {
+        if (self.startHyperlinkOnce(link)) {
             return;
         } else |err| switch (err) {
             // An actual self.alloc OOM is a fatal error.
-            error.RealOutOfMemory => return error.OutOfMemory,
+            error.OutOfMemory => return error.OutOfMemory,
 
             // strings table is out of memory, adjust it up
             error.StringsOutOfMemory => _ = try self.adjustCapacity(
@@ -1849,74 +1853,21 @@ pub fn startHyperlink(
 /// all the previous state and try again.
 fn startHyperlinkOnce(
     self: *Screen,
-    uri: []const u8,
+    source: hyperlink.Hyperlink,
-    id_: ?[]const u8,
+) (Allocator.Error || Page.InsertHyperlinkError)!void {
-) !void {
     // End any prior hyperlink
     self.endHyperlink();
 
-    // Create our hyperlink state.
+    // Allocate our new Hyperlink entry in non-page memory. This
-    const link = Hyperlink.create(self.alloc, uri, id_) catch |err| switch (err) {
+    // lets us quickly get access to URI, ID.
-        error.OutOfMemory => return error.RealOutOfMemory,
+    const link = try self.alloc.create(hyperlink.Hyperlink);
-    };
+    errdefer self.alloc.destroy(link);
-    errdefer link.destroy(self.alloc);
+    link.* = try source.dupe(self.alloc);
+    errdefer link.deinit(self.alloc);
 
-    // Copy our URI into the page memory.
+    // Insert the hyperlink into page memory
     var page = &self.cursor.page_pin.page.data;
-    const string_alloc = &page.string_alloc;
+    const id: hyperlink.Id = try page.insertHyperlink(link.*);
-    const page_uri: Offset(u8).Slice = uri: {
-        const buf = string_alloc.alloc(u8, page.memory, uri.len) catch |err| switch (err) {
-            error.OutOfMemory => return error.StringsOutOfMemory,
-        };
-        errdefer string_alloc.free(page.memory, buf);
-        @memcpy(buf, uri);
-
-        break :uri .{
-            .offset = size.getOffset(u8, page.memory, &buf[0]),
-            .len = uri.len,
-        };
-    };
-    errdefer string_alloc.free(
-        page.memory,
-        page_uri.offset.ptr(page.memory)[0..page_uri.len],
-    );
-
-    // Copy our ID into page memory or create an implicit ID via the counter
-    const page_id: hyperlink.Hyperlink.Id = if (id_) |id| explicit: {
-        const buf = string_alloc.alloc(u8, page.memory, id.len) catch |err| switch (err) {
-            error.OutOfMemory => return error.StringsOutOfMemory,
-        };
-        errdefer string_alloc.free(page.memory, buf);
-        @memcpy(buf, id);
-
-        break :explicit .{
-            .explicit = .{
-                .offset = size.getOffset(u8, page.memory, &buf[0]),
-                .len = id.len,
-            },
-        };
-    } else implicit: {
-        defer self.cursor.hyperlink_implicit_id += 1;
-        break :implicit .{ .implicit = self.cursor.hyperlink_implicit_id };
-    };
-    errdefer switch (page_id) {
-        .implicit => self.cursor.hyperlink_implicit_id -= 1,
-        .explicit => |slice| string_alloc.free(
-            page.memory,
-            slice.offset.ptr(page.memory)[0..slice.len],
-        ),
-    };
-
-    // Put our hyperlink into the hyperlink set to get an ID
-    const id = page.hyperlink_set.addContext(
-        page.memory,
-        .{ .id = page_id, .uri = page_uri },
-        .{ .page = page },
-    ) catch |err| switch (err) {
-        error.OutOfMemory => return error.SetOutOfMemory,
-        error.NeedsRehash => return error.SetNeedsRehash,
-    };
-    errdefer page.hyperlink_set.release(page.memory, id);
 
     // Save it all
     self.cursor.hyperlink = link;
@@ -1944,7 +1895,8 @@ pub fn endHyperlink(self: *Screen) void {
     // will be called.
     var page = &self.cursor.page_pin.page.data;
     page.hyperlink_set.release(page.memory, self.cursor.hyperlink_id);
-    self.cursor.hyperlink.?.destroy(self.alloc);
+    self.cursor.hyperlink.?.deinit(self.alloc);
+    self.alloc.destroy(self.cursor.hyperlink.?);
     self.cursor.hyperlink_id = 0;
     self.cursor.hyperlink = null;
 }

src/terminal/hyperlink.zig

@@ -1,4 +1,5 @@
 const std = @import("std");
+const Allocator = std.mem.Allocator;
 const assert = std.debug.assert;
 const hash_map = @import("hash_map.zig");
 const AutoOffsetHashMap = hash_map.AutoOffsetHashMap;
@@ -21,9 +22,63 @@ pub const Id = size.CellCountInt;
 // the hyperlink ID in the cell itself.
 pub const Map = AutoOffsetHashMap(Offset(Cell), Id);
 
-/// The main entry for hyperlinks.
+/// A fully decoded hyperlink that may or may not have its
+/// memory within a page. The memory location of this is dependent
+/// on the context so users should check with the source of the
+/// hyperlink.
 pub const Hyperlink = struct {
     id: Hyperlink.Id,
+    uri: []const u8,
+
+    /// See PageEntry.Id
+    pub const Id = union(enum) {
+        explicit: []const u8,
+        implicit: size.OffsetInt,
+    };
+
+    /// Deinit and deallocate all the pointers using the given
+    /// allocator.
+    ///
+    /// WARNING: This should only be called if the hyperlink was
+    /// heap-allocated. This DOES NOT need to be unconditionally
+    /// called.
+    pub fn deinit(self: *const Hyperlink, alloc: Allocator) void {
+        alloc.free(self.uri);
+        switch (self.id) {
+            .implicit => {},
+            .explicit => |v| alloc.free(v),
+        }
+    }
+
+    /// Duplicate a hyperlink by allocating all values with the
+    /// given allocator. The returned hyperlink should have deinit
+    /// called.
+    pub fn dupe(
+        self: *const Hyperlink,
+        alloc: Allocator,
+    ) Allocator.Error!Hyperlink {
+        const uri = try alloc.dupe(u8, self.uri);
+        errdefer alloc.free(uri);
+
+        const id: Hyperlink.Id = switch (self.id) {
+            .implicit => self.id,
+            .explicit => |v| .{ .explicit = try alloc.dupe(u8, v) },
+        };
+        errdefer switch (id) {
+            .implicit => {},
+            .explicit => |v| alloc.free(v),
+        };
+
+        return .{ .id = id, .uri = uri };
+    }
+};
+
+/// A hyperlink that has been committed to page memory. This
+/// is a "page entry" because while it represents a hyperlink,
+/// some decoding (pointer chasing) is still necessary to get the
+/// fully realized ID, URI, etc.
+pub const PageEntry = struct {
+    id: PageEntry.Id,
     uri: Offset(u8).Slice,
 
     pub const Id = union(enum) {
@@ -37,10 +92,10 @@ pub const Hyperlink = struct {
 
     /// Duplicate this hyperlink from one page to another.
     pub fn dupe(
-        self: *const Hyperlink,
+        self: *const PageEntry,
         self_page: *const Page,
         dst_page: *Page,
-    ) error{OutOfMemory}!Hyperlink {
+    ) error{OutOfMemory}!PageEntry {
         var copy = self.*;
 
         // If the pages are the same then we can return a shallow copy.
@@ -85,7 +140,7 @@ pub const Hyperlink = struct {
         return copy;
     }
 
-    pub fn hash(self: *const Hyperlink, base: anytype) u64 {
+    pub fn hash(self: *const PageEntry, base: anytype) u64 {
         var hasher = Wyhash.init(0);
         autoHash(&hasher, std.meta.activeTag(self.id));
         switch (self.id) {
@@ -105,9 +160,9 @@ pub const Hyperlink = struct {
     }
 
     pub fn eql(
-        self: *const Hyperlink,
+        self: *const PageEntry,
         self_base: anytype,
-        other: *const Hyperlink,
+        other: *const PageEntry,
         other_base: anytype,
     ) bool {
         if (std.meta.activeTag(self.id) != std.meta.activeTag(other.id)) return false;
@@ -135,21 +190,21 @@ pub const Hyperlink = struct {
 /// The set of hyperlinks. This is ref-counted so that a set of cells
 /// can share the same hyperlink without duplicating the data.
 pub const Set = RefCountedSet(
-    Hyperlink,
+    PageEntry,
     Id,
     size.CellCountInt,
     struct {
         page: ?*Page = null,
 
-        pub fn hash(self: *const @This(), link: Hyperlink) u64 {
+        pub fn hash(self: *const @This(), link: PageEntry) u64 {
             return link.hash(self.page.?.memory);
         }
 
-        pub fn eql(self: *const @This(), a: Hyperlink, b: Hyperlink) bool {
+        pub fn eql(self: *const @This(), a: PageEntry, b: PageEntry) bool {
             return a.eql(self.page.?.memory, &b, self.page.?.memory);
         }
 
-        pub fn deleted(self: *const @This(), link: Hyperlink) void {
+        pub fn deleted(self: *const @This(), link: PageEntry) void {
             const page = self.page.?;
             const alloc = &page.string_alloc;
             switch (link.id) {

src/terminal/page.zig

@@ -808,7 +808,7 @@ pub const Page = struct {
 
                     // If our page can't support an additional cell with
                     // a hyperlink then we have to return an error.
-                    if (self.hyperlinkCount() >= self.hyperlinkCapacity() - 1) {
+                    if (self.hyperlinkCount() >= self.hyperlinkCapacity()) {
                         // The hyperlink map capacity needs to be increased.
                         return error.HyperlinkMapOutOfMemory;
                     }
@@ -1142,6 +1142,101 @@ pub const Page = struct {
         row.hyperlink = false;
     }
 
+    pub const InsertHyperlinkError = error{
+        /// string_alloc errors
+        StringsOutOfMemory,
+
+        /// hyperlink_set errors
+        SetOutOfMemory,
+        SetNeedsRehash,
+    };
+
+    /// Convert a hyperlink into a page entry, returning the ID.
+    ///
+    /// This does not de-dupe any strings, so if the URI, explicit ID,
+    /// etc. is already in the strings table this will duplicate it.
+    ///
+    /// To release the memory associated with the given hyperlink,
+    /// release the ID from the `hyperlink_set`. If the refcount reaches
+    /// zero and the slot is needed then the context will reap the
+    /// memory.
+    pub fn insertHyperlink(
+        self: *Page,
+        link: hyperlink.Hyperlink,
+    ) InsertHyperlinkError!hyperlink.Id {
+        // Insert our URI into the page strings table.
+        const page_uri: Offset(u8).Slice = uri: {
+            const buf = self.string_alloc.alloc(
+                u8,
+                self.memory,
+                link.uri.len,
+            ) catch |err| switch (err) {
+                error.OutOfMemory => return error.StringsOutOfMemory,
+            };
+            errdefer self.string_alloc.free(self.memory, buf);
+            @memcpy(buf, link.uri);
+
+            break :uri .{
+                .offset = size.getOffset(u8, self.memory, &buf[0]),
+                .len = link.uri.len,
+            };
+        };
+        errdefer self.string_alloc.free(
+            self.memory,
+            page_uri.offset.ptr(self.memory)[0..page_uri.len],
+        );
+
+        // Allocate an ID for our page memory if we have to.
+        const page_id: hyperlink.PageEntry.Id = switch (link.id) {
+            .explicit => |id| explicit: {
+                const buf = self.string_alloc.alloc(
+                    u8,
+                    self.memory,
+                    id.len,
+                ) catch |err| switch (err) {
+                    error.OutOfMemory => return error.StringsOutOfMemory,
+                };
+                errdefer self.string_alloc.free(self.memory, buf);
+                @memcpy(buf, id);
+
+                break :explicit .{
+                    .explicit = .{
+                        .offset = size.getOffset(u8, self.memory, &buf[0]),
+                        .len = id.len,
+                    },
+                };
+            },
+
+            .implicit => |id| .{ .implicit = id },
+        };
+        errdefer switch (page_id) {
+            .implicit => {},
+            .explicit => |slice| self.string_alloc.free(
+                self.memory,
+                slice.offset.ptr(self.memory)[0..slice.len],
+            ),
+        };
+
+        // Build our entry
+        const entry: hyperlink.PageEntry = .{
+            .id = page_id,
+            .uri = page_uri,
+        };
+
+        // Put our hyperlink into the hyperlink set to get an ID
+        const id = self.hyperlink_set.addContext(
+            self.memory,
+            entry,
+            .{ .page = self },
+        ) catch |err| switch (err) {
+            error.OutOfMemory => return error.SetOutOfMemory,
+            error.NeedsRehash => return error.SetNeedsRehash,
+        };
+        errdefer self.hyperlink_set.release(self.memory, id);
+
+        return id;
+    }
+
     /// Set the hyperlink for the given cell. If the cell already has a
     /// hyperlink, then this will handle memory management and refcount
     /// update for the prior hyperlink.
@@ -2237,6 +2332,50 @@ test "Page cloneFrom partial" {
     }
 }
 
+test "Page cloneFrom hyperlinks exact capacity" {
+    var page = try Page.init(.{
+        .cols = 50,
+        .rows = 50,
+    });
+    defer page.deinit();
+
+    // Ensure our page can accommodate the capacity.
+    const hyperlink_cap = page.hyperlinkCapacity();
+    try testing.expect(hyperlink_cap <= page.size.cols * page.size.rows);
+
+    // Create a hyperlink.
+    const hyperlink_id = try page.insertHyperlink(.{
+        .id = .{ .implicit = 0 },
+        .uri = "https://example.com",
+    });
+
+    // Fill the exact cap with cells.
+    fill: for (0..page.size.cols) |x| {
+        for (0..page.size.rows) |y| {
+            const rac = page.getRowAndCell(x, y);
+            rac.cell.* = .{
+                .content_tag = .codepoint,
+                .content = .{ .codepoint = 42 },
+            };
+            try page.setHyperlink(rac.row, rac.cell, hyperlink_id);
+            page.hyperlink_set.use(page.memory, hyperlink_id);
+
+            if (page.hyperlinkCount() == hyperlink_cap) {
+                break :fill;
+            }
+        }
+    }
+    try testing.expectEqual(page.hyperlinkCount(), page.hyperlinkCapacity());
+
+    // Clone the full page
+    var page2 = try Page.init(page.capacity);
+    defer page2.deinit();
+    try page2.cloneFrom(&page, 0, page.size.rows);
+
+    // We should have the same number of hyperlinks
+    try testing.expectEqual(page2.hyperlinkCount(), page.hyperlinkCount());
+}
+
 test "Page cloneFrom graphemes" {
     var page = try Page.init(.{
         .cols = 10,