renderer/Metal: cell Contents cleanup

Cleaned up naming, some logic changes, added comments.

src/renderer/Metal.zig

@@ -566,9 +566,6 @@ pub fn init(alloc: Allocator, options: renderer.Options) !Metal {
         };
     };
 
-    var cells = try mtl_cell.Contents.init(alloc);
-    errdefer cells.deinit(alloc);
-
     const display_link: ?DisplayLink = switch (builtin.os.tag) {
         .macos => if (options.config.vsync)
             try macos.video.DisplayLink.createWithActiveCGDisplays()
@@ -592,7 +589,7 @@ pub fn init(alloc: Allocator, options: renderer.Options) !Metal {
         .current_background_color = options.config.background,
 
         // Render state
-        .cells = cells,
+        .cells = .{},
         .uniforms = .{
             .projection_matrix = undefined,
             .cell_size = undefined,
@@ -1037,8 +1034,8 @@ pub fn drawFrame(self: *Metal, surface: *apprt.Surface) !void {
 
     // Setup our frame data
     try frame.uniforms.sync(self.gpu_state.device, &.{self.uniforms});
-    const bg_count = try frame.cells_bg.syncFromArrayLists(self.gpu_state.device, self.cells.bgs.pools);
+    const bg_count = try frame.cells_bg.syncFromArrayLists(self.gpu_state.device, self.cells.bg_rows.lists);
-    const fg_count = try frame.cells.syncFromArrayLists(self.gpu_state.device, self.cells.text.pools);
+    const fg_count = try frame.cells.syncFromArrayLists(self.gpu_state.device, self.cells.fg_rows.lists);
 
     // If we have custom shaders, update the animation time.
     if (self.custom_shader_state) |*state| {

src/renderer/metal/cell.zig

@@ -26,54 +26,41 @@ pub const Key = enum {
     }
 };
 
-/// A collection of ArrayLists with methods for bulk operations.
+/// A pool of ArrayLists with methods for bulk operations.
-fn PooledArrayList(comptime T: type) type {
+fn ArrayListPool(comptime T: type) type {
     return struct {
-        pools: []std.ArrayListUnmanaged(T),
+        const Self = ArrayListPool(T);
+        const ArrayListT = std.ArrayListUnmanaged(T);
 
-        pub fn init(alloc: Allocator, pool_count: usize) !PooledArrayList(T) {
+        // An array containing the lists that belong to this pool.
-            var self: PooledArrayList(T) = .{
+        lists: []ArrayListT = &[_]ArrayListT{},
-                .pools = try alloc.alloc(std.ArrayListUnmanaged(T), pool_count),
+
+        // The pool will be initialized with empty ArrayLists.
+        pub fn init(alloc: Allocator, list_count: usize, initial_capacity: usize) !Self {
+            const self: Self = .{
+                .lists = try alloc.alloc(ArrayListT, list_count),
             };
 
-            for (self.pools) |*list| {
+            for (self.lists) |*list| {
-                list.* = .{};
+                list.* = try ArrayListT.initCapacity(alloc, initial_capacity);
             }
 
-            self.reset();
-
             return self;
         }
 
-        pub fn deinit(self: *PooledArrayList(T), alloc: Allocator) void {
+        pub fn deinit(self: *Self, alloc: Allocator) void {
-            for (self.pools) |*list| {
+            for (self.lists) |*list| {
                 list.deinit(alloc);
             }
-            alloc.free(self.pools);
+            alloc.free(self.lists);
         }
 
-        /// Reset all pools to an empty state without freeing or resizing.
+        /// Clear all lists in the pool.
-        pub fn reset(self: *PooledArrayList(T)) void {
+        pub fn reset(self: *Self) void {
-            for (self.pools) |*list| {
+            for (self.lists) |*list| {
                 list.clearRetainingCapacity();
             }
         }
-
-        /// Change the pool count and clear the contents of all pools.
-        pub fn resize(self: *PooledArrayList(T), alloc: Allocator, pool_count: u16) !void {
-            const pools  = try alloc.alloc(std.ArrayListUnmanaged(T), pool_count);
-            errdefer alloc.free(pools);
-
-            alloc.free(self.pools);
-
-            self.pools = pools;
-
-            for (self.pools) |*list| {
-                list.* = .{};
-            }
-
-            self.reset();
-        }
     };
 }
 
@@ -83,25 +70,54 @@ fn PooledArrayList(comptime T: type) type {
 /// clearing of data from the GPU buffers, to allow for row-wise dirty
 /// tracking to eliminate the overhead of rebuilding the GPU buffers
 /// each frame.
+///
+/// Must be initialized by resizing before calling any operations.
 pub const Contents = struct {
-    size: renderer.GridSize,
+    size: renderer.GridSize = .{ .rows = 0, .columns = 0 },
 
-    bgs: PooledArrayList(mtl_shaders.CellBg),
+    /// The ArrayListPool which holds all of the background cells. When sized
-    text: PooledArrayList(mtl_shaders.CellText),
+    /// with Contents.resize the individual ArrayLists SHOULD be given enough
+    /// capacity that appendAssumeCapacity may be used, since it should be
+    /// impossible for a row to have more background cells than columns.
+    ///
+    /// HOWEVER, the initial capacity can be exceeded due to multi-glyph
+    /// composites each adding a background cell for the same position.
+    /// This should probably be considered a bug, but for now it means
+    /// that sometimes allocations might happen, so appendAssumeCapacity
+    /// MUST NOT be used.
+    ///
+    /// Rows are indexed as Contents.bg_rows[y].
+    ///
+    /// Must be initialized by calling resize on the Contents struct before
+    /// calling any operations.
+    bg_rows: ArrayListPool(mtl_shaders.CellBg) = .{},
 
-    pub fn init(alloc: Allocator) !Contents {
+    /// The ArrayListPool which holds all of the foreground cells. When sized
-        const result: Contents = .{
+    /// with Contents.resize the individual ArrayLists are given enough room
-            .size = .{ .rows = 0, .columns = 0 },
+    /// that they can hold a single row with #cols glyphs, underlines, and
-            .bgs = try PooledArrayList(mtl_shaders.CellBg).init(alloc, 0),
+    /// strikethroughs; however, appendAssumeCapacity MUST NOT be used since
-            .text = try PooledArrayList(mtl_shaders.CellText).init(alloc, 0),
+    /// it is possible to exceed this with combining glyphs that add a glyph
-        };
+    /// but take up no column since they combine with the previous one, as
-
+    /// well as with fonts that perform multi-substitutions for glyphs, which
-        return result;
+    /// can result in a similar situation where multiple glyphs reside in the
-    }
+    /// same column.
+    ///
+    /// Allocations should nevertheless be exceedingly rare since hitting the
+    /// initial capacity of a list would require a row filled with underlined
+    /// struck through characters, at least one of which is a multi-glyph
+    /// composite.
+    ///
+    /// Rows are indexed as Contents.fg_rows[y + 1], because the first list in
+    /// the pool is reserved for the cursor, which must be the first item in
+    /// the buffer.
+    ///
+    /// Must be initialized by calling resize on the Contents struct before
+    /// calling any operations.
+    fg_rows: ArrayListPool(mtl_shaders.CellText) = .{},
 
     pub fn deinit(self: *Contents, alloc: Allocator) void {
-        self.bgs.deinit(alloc);
+        self.bg_rows.deinit(alloc);
-        self.text.deinit(alloc);
+        self.fg_rows.deinit(alloc);
     }
 
     /// Resize the cell contents for the given grid size. This will
@@ -112,25 +128,58 @@ pub const Contents = struct {
         size: renderer.GridSize,
     ) !void {
         self.size = size;
-        try self.bgs.resize(alloc, size.rows);
-        try self.text.resize(alloc, size.rows + 1);
 
-        // Make sure we don't have to allocate for the cursor cell.
+        // When we create our bg_rows pool, we give the lists an initial
-        try self.text.pools[0].ensureTotalCapacity(alloc, 1);
+        // capacity of size.columns. This is to account for the usual case
+        // where you have a row with normal text and background colors.
+        // This can be exceeded due to multi-glyph composites each adding
+        // a background cell for the same position. This should probably be
+        // considered a bug, but for now it means that sometimes allocations
+        // might happen, and appendAssumeCapacity MUST NOT be used.
+        var bg_rows = try ArrayListPool(mtl_shaders.CellBg).init(alloc, size.rows, size.columns);
+        errdefer bg_rows.deinit(alloc);
+
+        // The foreground lists can hold 3 types of items:
+        // - Glyphs
+        // - Underlines
+        // - Strikethroughs
+        // So we give them an initial capacity of size.columns * 3, which will
+        // avoid any further allocations in the vast majority of cases. Sadly
+        // we can not assume capacity though, since with combining glyphs that
+        // form a single grapheme, and multi-substitutions in fonts, the number
+        // of glyphs in a row is theoretically unlimited.
+        //
+        // We have size.rows + 1 lists because index 0 is used for a special
+        // list containing the cursor cell which needs to be first in the buffer.
+        var fg_rows = try ArrayListPool(mtl_shaders.CellText).init(alloc, size.rows + 1, size.columns * 3);
+        errdefer fg_rows.deinit(alloc);
+
+        self.bg_rows.deinit(alloc);
+        self.fg_rows.deinit(alloc);
+
+        self.bg_rows = bg_rows;
+        self.fg_rows = fg_rows;
+
+        // We don't need 3*cols worth of cells for the cursor list, so we can
+        // replace it with a smaller list. This is technically a tiny bit of
+        // extra work but resize is not a hot function so it's worth it to not
+        // waste the memory.
+        self.fg_rows.lists[0].deinit(alloc);
+        self.fg_rows.lists[0] = try std.ArrayListUnmanaged(mtl_shaders.CellText).initCapacity(alloc, 1);
     }
 
     /// Reset the cell contents to an empty state without resizing.
     pub fn reset(self: *Contents) void {
-        self.bgs.reset();
+        self.bg_rows.reset();
-        self.text.reset();
+        self.fg_rows.reset();
     }
 
     /// Set the cursor value. If the value is null then the cursor is hidden.
     pub fn setCursor(self: *Contents, v: ?mtl_shaders.CellText) void {
-        self.text.pools[0].clearRetainingCapacity();
+        self.fg_rows.lists[0].clearRetainingCapacity();
 
         if (v) |cell| {
-            self.text.pools[0].appendAssumeCapacity(cell);
+            self.fg_rows.lists[0].appendAssumeCapacity(cell);
         }
     }
 
@@ -147,25 +196,25 @@ pub const Contents = struct {
 
         switch (key) {
             .bg
-            => try self.bgs.pools[y].append(alloc, cell),
+            => try self.bg_rows.lists[y].append(alloc, cell),
 
             .text,
             .underline,
             .strikethrough
-            // We have a special pool containing the cursor cell at the start
+            // We have a special list containing the cursor cell at the start
-            // of our text pool list, so we need to add 1 to the y to get the
+            // of our fg row pool, so we need to add 1 to the y to get the
             // correct index.
-            => try self.text.pools[y + 1].append(alloc, cell),
+            => try self.fg_rows.lists[y + 1].append(alloc, cell),
         }
     }
 
     /// Clear all of the cell contents for a given row.
     pub fn clear(self: *Contents, y: terminal.size.CellCountInt) void {
-        self.bgs.pools[y].clearRetainingCapacity();
+        self.bg_rows.lists[y].clearRetainingCapacity();
-        // We have a special pool containing the cursor cell at the start
+        // We have a special list containing the cursor cell at the start
-        // of our text pool list, so we need to add 1 to the y to get the
+        // of our fg row pool, so we need to add 1 to the y to get the
         // correct index.
-        self.text.pools[y + 1].clearRetainingCapacity();
+        self.fg_rows.lists[y + 1].clearRetainingCapacity();
     }
 };