From db2cefb668e561f1562a1cddbcca7211c7f9142f Mon Sep 17 00:00:00 2001 From: Qwerasd Date: Mon, 10 Jun 2024 12:20:49 -0400 Subject: [PATCH 01/15] misc: improve rebuildCells time logging --- src/renderer/Metal.zig | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/renderer/Metal.zig b/src/renderer/Metal.zig index 173edfa1e..ca52ea362 100644 --- a/src/renderer/Metal.zig +++ b/src/renderer/Metal.zig @@ -1877,9 +1877,11 @@ fn rebuildCells( color_palette: *const terminal.color.Palette, ) !void { // const start = try std.time.Instant.now(); + // const start_micro = std.time.microTimestamp(); // defer { // const end = std.time.Instant.now() catch unreachable; - // std.log.warn("rebuildCells time={}us", .{end.since(start) / std.time.ns_per_us}); + // // "[rebuildCells time] \t" + // std.log.warn("[rebuildCells time] {}\t{}", .{start_micro, end.since(start) / std.time.ns_per_us}); // } // Create an arena for all our temporary allocations while rebuilding From 8d76b5f2835f428adaebd8f3498ebed0e4777956 Mon Sep 17 00:00:00 2001 From: Qwerasd Date: Mon, 10 Jun 2024 13:58:35 -0400 Subject: [PATCH 02/15] perf: introduce CacheTable strcture, use it for shaper cache --- src/cache_table.zig | 135 ++++++++++++++++++++++++++++++++++++++ src/fastmem.zig | 45 +++++++++++++ src/font/shaper/Cache.zig | 100 +++++++++++++--------------- 3 files changed, 224 insertions(+), 56 deletions(-) create mode 100644 src/cache_table.zig diff --git a/src/cache_table.zig b/src/cache_table.zig new file mode 100644 index 000000000..bdc99cd14 --- /dev/null +++ b/src/cache_table.zig @@ -0,0 +1,135 @@ +const fastmem = @import("./fastmem.zig"); + +const std = @import("std"); +const assert = std.debug.assert; + +/// An associative data structure used for efficiently storing and +/// retrieving values which are able to be recomputed if necessary. +/// +/// This structure is effectively a hash table with fixed-sized buckets. +/// +/// When inserting an item in to a full bucket, the least recently used +/// item is replaced. +/// +/// To achieve this, when an item is accessed, it's moved to the end of +/// the bucket, and the rest of the items are moved over to fill the gap. +/// +/// This should provide very good query performance and keep frequently +/// accessed items cached indefinitely. +/// +/// Parameters: +/// +/// `Context` +/// A type containing methods to define CacheTable behaviors. +/// - `fn hash(*Context, K) u64` - Return a hash for a key. +/// - `fn eql(*Context, K, K) bool` - Check two keys for equality. +/// +/// - `fn evicted(*Context, K, V) void` - [OPTIONAL] Eviction callback. +/// If present, called whenever an item is evicted from the cache. +/// +/// `bucket_count` +/// Should ideally be close to the median number of important items that +/// you expect to be cached at any given point. +/// +/// Performance will suffer if this is not a power of 2. +/// +/// `bucket_size` +/// should be larger if you expect a large number of unimportant items to +/// enter the cache at a time. Having larger buckets will avoid important +/// items being dropped from the cache prematurely. +/// +pub fn CacheTable( + comptime K: type, + comptime V: type, + comptime Context: type, + comptime bucket_count: usize, + comptime bucket_size: u8, +) type { + return struct { + const Self = CacheTable(K, V, Context, bucket_count, bucket_size); + + const KV = struct { + key: K, + value: V, + }; + + /// `bucket_count` buckets containing `bucket_size` KV pairs each. + /// + /// We don't need to initialize this memory because we don't use it + /// unless it's within a bucket's stored length, which will guarantee + /// that we put actual items there. + buckets: [bucket_count][bucket_size]KV = undefined, + + /// We use this array to keep track of how many slots in each bucket + /// have actual items in them. Once all the buckets fill up this will + /// become a pointless check, but hopefully branch prediction picks + /// up on it at that point. The memory cost isn't too bad since it's + /// just bytes, so should be a fraction the size of the main table. + lengths: [bucket_count]u8 = [_]u8{0} ** bucket_count, + + /// An instance of the context structure. + /// Must be initialized before calling any operations. + context: Context, + + /// Adds an item to the cache table. If an old value was removed to + /// make room then it is returned in a struct with its key and value. + pub fn put(self: *Self, key: K, value: V) ?KV { + const idx: u64 = self.context.hash(key) % bucket_count; + + const kv = .{ + .key = key, + .value = value, + }; + + if (self.lengths[idx] < bucket_size) { + self.buckets[idx][self.lengths[idx]] = kv; + self.lengths[idx] += 1; + return null; + } + + assert(self.lengths[idx] == bucket_size); + + const evicted = fastmem.rotateIn(KV, &self.buckets[idx], kv); + + if (comptime @hasDecl(Context, "evicted")) { + self.context.evicted(evicted.key, evicted.value); + } + + return evicted; + } + + /// Retrieves an item from the cache table. + /// + /// Returns null if no item is found with the provided key. + pub fn get(self: *Self, key: K) ?V { + const idx = self.context.hash(key) % bucket_count; + + const len = self.lengths[idx]; + var i: usize = len; + while (i > 0) { + i -= 1; + if (self.context.eql(key, self.buckets[idx][i].key)) { + defer fastmem.rotateOnce(KV, self.buckets[idx][i..len]); + return self.buckets[idx][i].value; + } + } + + return null; + } + + /// Removes all items from the cache table. + /// + /// If your `Context` has an `evicted` method, + /// it will be called with all removed items. + pub fn clear(self: *Self) void { + if (comptime @hasDecl(Context, "evicted")) { + for (self.buckets, self.lengths) |b, l| { + for (b[0..l]) |kv| { + self.context.evicted(kv.key, kv.value); + } + } + } + @memset(&self.lengths, 0); + } + }; +} diff --git a/src/fastmem.zig b/src/fastmem.zig index 53c9e1122..687c057af 100644 --- a/src/fastmem.zig +++ b/src/fastmem.zig @@ -22,13 +22,58 @@ pub inline fn copy(comptime T: type, dest: []T, source: []const T) void { } } +/// Moves the first item to the end. +/// For the reverse of this, use `fastmem.rotateOnceR`. +/// /// Same as std.mem.rotate(T, items, 1) but more efficient by using memmove /// and a tmp var for the single rotated item instead of 3 calls to reverse. +/// +/// e.g. `0 1 2 3` -> `1 2 3 0`. pub inline fn rotateOnce(comptime T: type, items: []T) void { const tmp = items[0]; move(T, items[0 .. items.len - 1], items[1..items.len]); items[items.len - 1] = tmp; } +/// Moves the last item to the start. +/// Reverse operation of `fastmem.rotateOnce`. +/// +/// Same as std.mem.rotate(T, items, items.len - 1) but more efficient by +/// using memmove and a tmp var for the single rotated item instead of 3 +/// calls to reverse. +/// +/// e.g. `0 1 2 3` -> `3 0 1 2`. +pub inline fn rotateOnceR(comptime T: type, items: []T) void { + const tmp = items[items.len - 1]; + move(T, items[1..items.len], items[0 .. items.len - 1]); + items[0] = tmp; +} + +/// Rotates a new item in to the end of a slice. +/// The first item from the slice is removed and returned. +/// +/// e.g. rotating `4` in to `0 1 2 3` makes it `1 2 3 4` and returns `0`. +/// +/// For the reverse of this, use `fastmem.rotateInR`. +pub inline fn rotateIn(comptime T: type, items: []T, item: T) T { + const removed = items[0]; + move(T, items[0 .. items.len - 1], items[1..items.len]); + items[items.len - 1] = item; + return removed; +} + +/// Rotates a new item in to the start of a slice. +/// The last item from the slice is removed and returned. +/// +/// e.g. rotating `4` in to `0 1 2 3` makes it `4 0 1 2` and returns `3`. +/// +/// Reverse operation of `fastmem.rotateIn`. +pub inline fn rotateInR(comptime T: type, items: []T, item: T) T { + const removed = items[items.len - 1]; + move(T, items[1..items.len], items[0 .. items.len - 1]); + items[0] = item; + return removed; +} + extern "c" fn memcpy(*anyopaque, *const anyopaque, usize) *anyopaque; extern "c" fn memmove(*anyopaque, *const anyopaque, usize) *anyopaque; diff --git a/src/font/shaper/Cache.zig b/src/font/shaper/Cache.zig index afb89ee9c..2a1424118 100644 --- a/src/font/shaper/Cache.zig +++ b/src/font/shaper/Cache.zig @@ -14,55 +14,57 @@ const std = @import("std"); const assert = std.debug.assert; const Allocator = std.mem.Allocator; const font = @import("../main.zig"); -const lru = @import("../../lru.zig"); +const CacheTable = @import("../../cache_table.zig").CacheTable; const log = std.log.scoped(.font_shaper_cache); -/// Our LRU is the run hash to the shaped cells. -const LRU = lru.AutoHashMap(u64, []font.shape.Cell); +/// Context for cache table. +const CellCacheTableContext = struct { + pub fn hash(self: *const CellCacheTableContext, key: u64) u64 { + _ = self; + return key; + } + pub fn eql(self: *const CellCacheTableContext, a: u64, b: u64) bool { + _ = self; + return a == b; + } +}; -/// This is the threshold of evictions at which point we reset -/// the LRU completely. This is a workaround for the issue that -/// Zig stdlib hashmap gets slower over time -/// (https://github.com/ziglang/zig/issues/17851). -/// -/// The value is based on naive measuring on my local machine. -/// If someone has a better idea of what this value should be, -/// please let me know. -const evictions_threshold = 8192; +/// Cache table for run hash -> shaped cells. +const CellCacheTable = CacheTable( + u64, + []font.shape.Cell, + CellCacheTableContext, -/// The cache of shaped cells. -map: LRU, + // Capacity is slightly arbitrary. These numbers are guesses. + // + // I'd expect then an average of 256 frequently cached runs is a + // safe guess most terminal screens. + 256, + // 8 items per bucket to give decent resilliency to important runs. + 8, +); -/// Keep track of the number of evictions. We use this to workaround -/// the issue that Zig stdlib hashmap gets slower over time -/// (https://github.com/ziglang/zig/issues/17851). When evictions -/// reaches a certain threshold, we reset the LRU. -evictions: std.math.IntFittingRange(0, evictions_threshold) = 0, +/// The cache table of shaped cells. +map: CellCacheTable, pub fn init() Cache { - // Note: this is very arbitrary. Increasing this number will increase - // the cache hit rate, but also increase the memory usage. We should do - // some more empirical testing to see what the best value is. - const capacity = 1024; - - return .{ .map = LRU.init(capacity) }; + return .{ .map = .{ .context = .{} } }; } pub fn deinit(self: *Cache, alloc: Allocator) void { - var it = self.map.map.iterator(); - while (it.next()) |entry| alloc.free(entry.value_ptr.*.data.value); - self.map.deinit(alloc); + self.clear(alloc); } -/// Get the shaped cells for the given text run or null if they are not -/// in the cache. -pub fn get(self: *const Cache, run: font.shape.TextRun) ?[]const font.shape.Cell { +/// Get the shaped cells for the given text run, +/// or null if they are not in the cache. +pub fn get(self: *Cache, run: font.shape.TextRun) ?[]const font.shape.Cell { return self.map.get(run.hash); } -/// Insert the shaped cells for the given text run into the cache. The -/// cells will be duplicated. +/// Insert the shaped cells for the given text run into the cache. +/// +/// The cells will be duplicated. pub fn put( self: *Cache, alloc: Allocator, @@ -70,33 +72,19 @@ pub fn put( cells: []const font.shape.Cell, ) Allocator.Error!void { const copy = try alloc.dupe(font.shape.Cell, cells); - const gop = try self.map.getOrPut(alloc, run.hash); - if (gop.evicted) |evicted| { - alloc.free(evicted.value); - - // See the doc comment on evictions_threshold for why we do this. - self.evictions += 1; - if (self.evictions >= evictions_threshold) { - log.debug("resetting cache due to too many evictions", .{}); - // We need to put our value here so deinit can free - gop.value_ptr.* = copy; - self.clear(alloc); - - // We need to call put again because self is now a - // different pointer value so our gop pointers are invalid. - return try self.put(alloc, run, cells); - } + const evicted = self.map.put(run.hash, copy); + if (evicted) |kv| { + alloc.free(kv.value); } - gop.value_ptr.* = copy; -} - -pub fn count(self: *const Cache) usize { - return self.map.map.count(); } fn clear(self: *Cache, alloc: Allocator) void { - self.deinit(alloc); - self.* = init(); + for (self.map.buckets, self.map.lengths) |b, l| { + for (b[0..l]) |kv| { + alloc.free(kv.value); + } + } + self.map.clear(); } test Cache { From 9741b3a18cbbda98580ed67f2ed4f62170cda3dd Mon Sep 17 00:00:00 2001 From: Qwerasd Date: Thu, 13 Jun 2024 22:48:15 -0400 Subject: [PATCH 03/15] perf: introduce RefCountedSet structure, use it for Style.Set --- src/terminal/PageList.zig | 48 +-- src/terminal/Screen.zig | 183 ++++------ src/terminal/Terminal.zig | 74 ++-- src/terminal/page.zig | 71 ++-- src/terminal/ref_counted_set.zig | 573 +++++++++++++++++++++++++++++++ src/terminal/style.zig | 325 ++++++++---------- 6 files changed, 871 insertions(+), 403 deletions(-) create mode 100644 src/terminal/ref_counted_set.zig diff --git a/src/terminal/PageList.zig b/src/terminal/PageList.zig index 6681b733e..4d1ac5d2c 100644 --- a/src/terminal/PageList.zig +++ b/src/terminal/PageList.zig @@ -1158,17 +1158,17 @@ fn reflowPage( // If the source cell has a style, we need to copy it. if (src_cursor.page_cell.style_id != stylepkg.default_id) { - const src_style = src_cursor.page.styles.lookupId( + const src_style = src_cursor.page.styles.get( src_cursor.page.memory, src_cursor.page_cell.style_id, - ).?.*; - - const dst_md = try dst_cursor.page.styles.upsert( + ).*; + if (try dst_cursor.page.styles.addWithId( dst_cursor.page.memory, src_style, - ); - dst_md.ref += 1; - dst_cursor.page_cell.style_id = dst_md.id; + src_cursor.page_cell.style_id, + )) |id| { + dst_cursor.page_cell.style_id = id; + } dst_cursor.page_row.styled = true; } } @@ -1905,18 +1905,6 @@ pub fn adjustCapacity( return new_page; } -/// Compact a page, reallocating to minimize the amount of memory -/// required for the page. This is useful when we've overflowed ID -/// spaces, are archiving a page, etc. -/// -/// Note today: this doesn't minimize the memory usage, but it does -/// fix style ID overflow. A future update can shrink the memory -/// allocation. -pub fn compact(self: *PageList, page: *List.Node) !*List.Node { - // Adjusting capacity with no adjustments forces a reallocation. - return try self.adjustCapacity(page, .{}); -} - /// Create a new page node. This does not add it to the list and this /// does not do any memory size accounting with max_size/page_size. fn createPage( @@ -3039,10 +3027,10 @@ pub const Pin = struct { /// Returns the style for the given cell in this pin. pub fn style(self: Pin, cell: *pagepkg.Cell) stylepkg.Style { if (cell.style_id == stylepkg.default_id) return .{}; - return self.page.data.styles.lookupId( + return self.page.data.styles.get( self.page.data.memory, cell.style_id, - ).?.*; + ).*; } /// Check if this pin is dirty. @@ -3302,10 +3290,10 @@ const Cell = struct { /// Not meant for non-test usage since this is inefficient. pub fn style(self: Cell) stylepkg.Style { if (self.cell.style_id == stylepkg.default_id) return .{}; - return self.page.data.styles.lookupId( + return self.page.data.styles.get( self.page.data.memory, self.cell.style_id, - ).?.*; + ).*; } /// Gets the screen point for the given cell. @@ -7363,18 +7351,20 @@ test "PageList resize reflow less cols copy style" { // Create a style const style: stylepkg.Style = .{ .flags = .{ .bold = true } }; - const style_md = try page.styles.upsert(page.memory, style); + const style_id = try page.styles.add(page.memory, style); for (0..s.cols - 1) |x| { const rac = page.getRowAndCell(x, 0); rac.cell.* = .{ .content_tag = .codepoint, .content = .{ .codepoint = @intCast(x) }, - .style_id = style_md.id, + .style_id = style_id, }; - - style_md.ref += 1; + page.styles.use(page.memory, style_id); } + + // We're over-counted by 1 because `add` implies `use`. + page.styles.release(page.memory, style_id); } // Resize @@ -7391,10 +7381,10 @@ test "PageList resize reflow less cols copy style" { const style_id = rac.cell.style_id; try testing.expect(style_id != 0); - const style = offset.page.data.styles.lookupId( + const style = offset.page.data.styles.get( offset.page.data.memory, style_id, - ).?; + ); try testing.expect(style.flags.bold); const row = rac.row; diff --git a/src/terminal/Screen.zig b/src/terminal/Screen.zig index df00546fd..cbe278ec5 100644 --- a/src/terminal/Screen.zig +++ b/src/terminal/Screen.zig @@ -100,7 +100,6 @@ pub const Cursor = struct { /// we change pages we need to ensure that we update that page with /// our style when used. style_id: style.Id = style.default_id, - style_ref: ?*size.CellCountInt = null, /// The pointers into the page list where the cursor is currently /// located. This makes it faster to move the cursor. @@ -202,31 +201,6 @@ pub fn assertIntegrity(self: *const Screen) void { ) orelse unreachable; assert(self.cursor.x == pt.active.x); assert(self.cursor.y == pt.active.y); - - if (self.cursor.style_id == style.default_id) { - // If our style is default, we should have no refs. - assert(self.cursor.style.default()); - assert(self.cursor.style_ref == null); - } else { - // If our style is not default, we should have a ref. - assert(!self.cursor.style.default()); - assert(self.cursor.style_ref != null); - - // Further, the ref should be valid within the current page. - const page = &self.cursor.page_pin.page.data; - const page_style = page.styles.lookupId( - page.memory, - self.cursor.style_id, - ).?.*; - assert(self.cursor.style.eql(page_style)); - - // The metadata pointer should be equal to the ref. - const md = page.styles.upsert( - page.memory, - page_style, - ) catch unreachable; - assert(&md.ref == self.cursor.style_ref.?); - } } } @@ -524,14 +498,6 @@ pub fn cursorReload(self: *Screen) void { // If we have a style, we need to ensure it is in the page because this // method may also be called after a page change. if (self.cursor.style_id != style.default_id) { - // We set our ref to null because manualStyleUpdate will refresh it. - // If we had a valid ref and it was zero before, then manualStyleUpdate - // will reload the same ref. - // - // We want to avoid the scenario this was non-null but the pointer - // is now invalid because it pointed to a page that no longer exists. - self.cursor.style_ref = null; - self.manualStyleUpdate() catch |err| { // This failure should not happen because manualStyleUpdate // handles page splitting, overflow, and more. This should only @@ -540,7 +506,6 @@ pub fn cursorReload(self: *Screen) void { log.err("failed to update style on cursor reload err={}", .{err}); self.cursor.style = .{}; self.cursor.style_id = 0; - self.cursor.style_ref = null; }; } } @@ -595,7 +560,6 @@ pub fn cursorDownScroll(self: *Screen) !void { log.err("failed to update style on cursor scroll err={}", .{err}); self.cursor.style = .{}; self.cursor.style_id = 0; - self.cursor.style_ref = null; }; } } else { @@ -683,7 +647,6 @@ pub fn cursorCopy(self: *Screen, other: Cursor) !void { // invalid. self.cursor.style = .{}; self.cursor.style_id = 0; - self.cursor.style_ref = null; // We need to keep our old x/y because that is our cursorAbsolute // will fix up our pointers. @@ -698,7 +661,6 @@ pub fn cursorCopy(self: *Screen, other: Cursor) !void { // We keep the old style ref so manualStyleUpdate can clean our old style up. self.cursor.style = other.style; self.cursor.style_id = old.style_id; - self.cursor.style_ref = old.style_ref; try self.manualStyleUpdate(); } @@ -744,7 +706,6 @@ fn cursorChangePin(self: *Screen, new: Pin) void { log.err("failed to update style on cursor change err={}", .{err}); self.cursor.style = .{}; self.cursor.style_id = 0; - self.cursor.style_ref = null; }; } @@ -889,33 +850,7 @@ pub fn clearCells( if (row.styled) { for (cells) |*cell| { if (cell.style_id == style.default_id) continue; - - // Fast-path, the style ID matches, in this case we just update - // our own ref and continue. We never delete because our style - // is still active. - if (page == &self.cursor.page_pin.page.data and - cell.style_id == self.cursor.style_id) - { - self.cursor.style_ref.?.* -= 1; - continue; - } - - // Slow path: we need to lookup this style so we can decrement - // the ref count. Since we've already loaded everything, we also - // just go ahead and GC it if it reaches zero, too. - if (page.styles.lookupId( - page.memory, - cell.style_id, - )) |prev_style| { - // Below upsert can't fail because it should already be present - const md = page.styles.upsert( - page.memory, - prev_style.*, - ) catch unreachable; - assert(md.ref > 0); - md.ref -= 1; - if (md.ref == 0) page.styles.remove(page.memory, cell.style_id); - } + page.styles.release(page.memory, cell.style_id); } // If we have no left/right scroll region we can be sure that @@ -1044,17 +979,37 @@ pub fn resizeWithoutReflow( } /// Resize the screen. -// TODO: replace resize and resizeWithoutReflow with this. fn resizeInternal( self: *Screen, cols: size.CellCountInt, rows: size.CellCountInt, reflow: bool, ) !void { + defer self.assertIntegrity(); + // No matter what we mark our image state as dirty self.kitty_images.dirty = true; - // Perform the resize operation. This will update cursor by reference. + // Release the cursor style while resizing just + // in case the cursor ends up on a different page. + const cursor_style = self.cursor.style; + self.cursor.style = .{}; + self.manualStyleUpdate() catch unreachable; + defer { + // Restore the cursor style. + self.cursor.style = cursor_style; + self.manualStyleUpdate() catch |err| { + // This failure should not happen because manualStyleUpdate + // handles page splitting, overflow, and more. This should only + // happen if we're out of RAM. In this case, we'll just degrade + // gracefully back to the default style. + log.err("failed to update style on cursor reload err={}", .{err}); + self.cursor.style = .{}; + self.cursor.style_id = 0; + }; + } + + // Perform the resize operation. try self.pages.resize(.{ .rows = rows, .cols = cols, @@ -1072,7 +1027,6 @@ fn resizeInternal( // If our cursor was updated, we do a full reload so all our cursor // state is correct. self.cursorReload(); - self.assertIntegrity(); } /// Set a style attribute for the current cursor. @@ -1223,21 +1177,14 @@ pub fn manualStyleUpdate(self: *Screen) !void { // std.log.warn("active styles={}", .{page.styles.count(page.memory)}); - // Remove our previous style if is unused. - if (self.cursor.style_ref) |ref| { - if (ref.* == 0) { - page.styles.remove(page.memory, self.cursor.style_id); - } - - // Reset our ID and ref to null since the ref is now invalid. - self.cursor.style_id = 0; - self.cursor.style_ref = null; + // Release our previous style if it was not default. + if (self.cursor.style_id != style.default_id) { + page.styles.release(page.memory, self.cursor.style_id); } // If our new style is the default, just reset to that if (self.cursor.style.default()) { self.cursor.style_id = 0; - self.cursor.style_ref = null; return; } @@ -1251,42 +1198,29 @@ pub fn manualStyleUpdate(self: *Screen) !void { // if that makes a meaningful difference. Our priority is to keep print // fast because setting a ton of styles that do nothing is uncommon // and weird. - const md = page.styles.upsert( + const id = page.styles.add( page.memory, self.cursor.style, - ) catch |err| md: { - switch (err) { - // Our style map is full. Let's allocate a new page by doubling - // the size and then try again. - error.OutOfMemory => { - const node = try self.pages.adjustCapacity( - self.cursor.page_pin.page, - .{ .styles = page.capacity.styles * 2 }, - ); + ) catch id: { + // Our style map is full. Let's allocate a new + // page by doubling the size and then try again. + const node = try self.pages.adjustCapacity( + self.cursor.page_pin.page, + .{ .styles = page.capacity.styles * 2 }, + ); - page = &node.data; - }, - - // We've run out of style IDs. This is fixed by doing a page - // compaction. - error.Overflow => { - const node = try self.pages.compact( - self.cursor.page_pin.page, - ); - page = &node.data; - }, - } + page = &node.data; // Since this modifies our cursor page, we need to reload cursor_reload = true; - break :md try page.styles.upsert( + break :id try page.styles.add( page.memory, self.cursor.style, ); }; - self.cursor.style_id = md.id; - self.cursor.style_ref = &md.ref; + self.cursor.style_id = id; + if (cursor_reload) self.cursorReload(); self.assertIntegrity(); } @@ -2271,8 +2205,9 @@ pub fn testWriteString(self: *Screen, text: []const u8) !void { }; // If we have a ref-counted style, increase. - if (self.cursor.style_ref) |ref| { - ref.* += 1; + if (self.cursor.style_id != style.default_id) { + const page = self.cursor.page_pin.page.data; + page.styles.use(page.memory, self.cursor.style_id); self.cursor.page_row.styled = true; } }, @@ -2310,6 +2245,14 @@ pub fn testWriteString(self: *Screen, text: []const u8) !void { .wide = .spacer_tail, .protected = self.cursor.protected, }; + + // If we have a ref-counted style, increase twice. + if (self.cursor.style_id != style.default_id) { + const page = self.cursor.page_pin.page.data; + page.styles.use(page.memory, self.cursor.style_id); + page.styles.use(page.memory, self.cursor.style_id); + self.cursor.page_row.styled = true; + } }, else => unreachable, @@ -2792,10 +2735,10 @@ test "Screen: cursorDown across pages preserves style" { try s.setAttribute(.{ .bold = {} }); { const page = &s.cursor.page_pin.page.data; - const styleval = page.styles.lookupId( + const styleval = page.styles.get( page.memory, s.cursor.style_id, - ).?; + ); try testing.expect(styleval.flags.bold); } @@ -2803,10 +2746,10 @@ test "Screen: cursorDown across pages preserves style" { s.cursorDown(1); { const page = &s.cursor.page_pin.page.data; - const styleval = page.styles.lookupId( + const styleval = page.styles.get( page.memory, s.cursor.style_id, - ).?; + ); try testing.expect(styleval.flags.bold); } } @@ -2835,10 +2778,10 @@ test "Screen: cursorUp across pages preserves style" { try s.setAttribute(.{ .bold = {} }); { const page = &s.cursor.page_pin.page.data; - const styleval = page.styles.lookupId( + const styleval = page.styles.get( page.memory, s.cursor.style_id, - ).?; + ); try testing.expect(styleval.flags.bold); } @@ -2848,10 +2791,10 @@ test "Screen: cursorUp across pages preserves style" { const page = &s.cursor.page_pin.page.data; try testing.expect(start_page == page); - const styleval = page.styles.lookupId( + const styleval = page.styles.get( page.memory, s.cursor.style_id, - ).?; + ); try testing.expect(styleval.flags.bold); } } @@ -2880,10 +2823,10 @@ test "Screen: cursorAbsolute across pages preserves style" { try s.setAttribute(.{ .bold = {} }); { const page = &s.cursor.page_pin.page.data; - const styleval = page.styles.lookupId( + const styleval = page.styles.get( page.memory, s.cursor.style_id, - ).?; + ); try testing.expect(styleval.flags.bold); } @@ -2893,10 +2836,10 @@ test "Screen: cursorAbsolute across pages preserves style" { const page = &s.cursor.page_pin.page.data; try testing.expect(start_page == page); - const styleval = page.styles.lookupId( + const styleval = page.styles.get( page.memory, s.cursor.style_id, - ).?; + ); try testing.expect(styleval.flags.bold); } } @@ -3013,10 +2956,10 @@ test "Screen: scrolling across pages preserves style" { const page = &s.pages.pages.last.?.data; try testing.expect(start_page != page); - const styleval = page.styles.lookupId( + const styleval = page.styles.get( page.memory, s.cursor.style_id, - ).?; + ); try testing.expect(styleval.flags.bold); } diff --git a/src/terminal/Terminal.zig b/src/terminal/Terminal.zig index b2783ba82..09223aeeb 100644 --- a/src/terminal/Terminal.zig +++ b/src/terminal/Terminal.zig @@ -600,8 +600,19 @@ fn printCell( ); } - // Keep track of the previous style so we can decrement the ref count - const prev_style_id = cell.style_id; + // We don't need to update the style refs unless the + // cell's new style will be different after writing. + const style_changed = cell.style_id != self.screen.cursor.style_id; + + if (style_changed) { + var page = &self.screen.cursor.page_pin.page.data; + + // Release the old style. + if (cell.style_id != style.default_id) { + assert(self.screen.cursor.page_row.styled); + page.styles.release(page.memory, cell.style_id); + } + } // Write cell.* = .{ @@ -612,50 +623,12 @@ fn printCell( .protected = self.screen.cursor.protected, }; - if (comptime std.debug.runtime_safety) { - // We've had bugs around this, so let's add an assertion: every - // style we use should be present in the style table. - if (self.screen.cursor.style_id != style.default_id) { - const page = &self.screen.cursor.page_pin.page.data; - if (page.styles.lookupId( - page.memory, - self.screen.cursor.style_id, - ) == null) { - log.err("can't find style page={X} id={}", .{ - @intFromPtr(&self.screen.cursor.page_pin.page.data), - self.screen.cursor.style_id, - }); - @panic("style not found"); - } - } - } + if (style_changed) { + var page = &self.screen.cursor.page_pin.page.data; - // Handle the style ref count handling - style_ref: { - if (prev_style_id != style.default_id) { - const row = self.screen.cursor.page_row; - assert(row.styled); - - // If our previous cell had the same style ID as us currently, - // then we don't bother with any ref counts because we're the same. - if (prev_style_id == self.screen.cursor.style_id) break :style_ref; - - // Slow path: we need to lookup this style so we can decrement - // the ref count. Since we've already loaded everything, we also - // just go ahead and GC it if it reaches zero, too. - var page = &self.screen.cursor.page_pin.page.data; - if (page.styles.lookupId(page.memory, prev_style_id)) |prev_style| { - // Below upsert can't fail because it should already be present - const md = page.styles.upsert(page.memory, prev_style.*) catch unreachable; - assert(md.ref > 0); - md.ref -= 1; - if (md.ref == 0) page.styles.remove(page.memory, prev_style_id); - } - } - - // If we have a ref-counted style, increase. - if (self.screen.cursor.style_ref) |ref| { - ref.* += 1; + // Use the new style. + if (cell.style_id != style.default_id) { + page.styles.use(page.memory, cell.style_id); self.screen.cursor.page_row.styled = true; } } @@ -2190,8 +2163,12 @@ pub fn decaln(self: *Terminal) !void { }); // If we have a ref-counted style, increase - if (self.screen.cursor.style_ref) |ref| { - ref.* += @intCast(cells.len); + if (self.screen.cursor.style_id != style.default_id) { + page.styles.useMultiple( + page.memory, + self.screen.cursor.style_id, + @intCast(cells.len), + ); row.styled = true; } @@ -7066,7 +7043,8 @@ test "Terminal: bold style" { const cell = list_cell.cell; try testing.expectEqual(@as(u21, 'A'), cell.content.codepoint); try testing.expect(cell.style_id != 0); - try testing.expect(t.screen.cursor.style_ref.?.* > 0); + const page = t.screen.cursor.page_pin.page.data; + try testing.expect(page.styles.refCount(page.memory, t.screen.cursor.style_id) > 1); } } diff --git a/src/terminal/page.zig b/src/terminal/page.zig index 7137431cf..b14632e46 100644 --- a/src/terminal/page.zig +++ b/src/terminal/page.zig @@ -197,6 +197,7 @@ pub const Page = struct { .styles = style.Set.init( buf.add(l.styles_start), l.styles_layout, + style.StyleSetContext{}, ), .grapheme_alloc = GraphemeAlloc.init( buf.add(l.grapheme_alloc_start), @@ -324,17 +325,11 @@ pub const Page = struct { if (cell.style_id != style.default_id) { // If a cell has a style, it must be present in the styles - // set. - _ = self.styles.lookupId( + // set. Accessing it with `get` asserts that. + _ = self.styles.get( self.memory, cell.style_id, - ) orelse { - log.warn( - "page integrity violation y={} x={} style missing id={}", - .{ y, x, cell.style_id }, - ); - return IntegrityError.MissingStyle; - }; + ); if (!row.styled) { log.warn( @@ -424,12 +419,11 @@ pub const Page = struct { { var it = styles_seen.iterator(); while (it.next()) |entry| { - const style_val = self.styles.lookupId(self.memory, entry.key_ptr.*).?.*; - const md = self.styles.upsert(self.memory, style_val) catch unreachable; - if (md.ref < entry.value_ptr.*) { + const ref_count = self.styles.refCount(self.memory, entry.key_ptr.*); + if (ref_count < entry.value_ptr.*) { log.warn( "page integrity violation style ref count mismatch id={} expected={} actual={}", - .{ entry.key_ptr.*, entry.value_ptr.*, md.ref }, + .{ entry.key_ptr.*, entry.value_ptr.*, ref_count }, ); return IntegrityError.MismatchedStyleRef; } @@ -474,7 +468,7 @@ pub const Page = struct { return result; } - pub const CloneFromError = Allocator.Error || style.Set.UpsertError; + pub const CloneFromError = Allocator.Error || error{OutOfMemory}; /// Clone the contents of another page into this page. The capacities /// can be different, but the size of the other page must fit into @@ -586,11 +580,22 @@ pub const Page = struct { for (cps) |cp| try self.appendGrapheme(dst_row, dst_cell, cp); } if (src_cell.style_id != style.default_id) { - const other_style = other.styles.lookupId(other.memory, src_cell.style_id).?.*; - const md = try self.styles.upsert(self.memory, other_style); - md.ref += 1; - dst_cell.style_id = md.id; dst_row.styled = true; + + if (other == self) { + // If it's the same page we don't have to worry about + // copying the style, we can use the style ID directly. + dst_cell.style_id = src_cell.style_id; + self.styles.use(self.memory, dst_cell.style_id); + continue; + } + + // Slow path: Get the style from the other + // page and add it to this page's style set. + const other_style = other.styles.get(other.memory, src_cell.style_id).*; + if (try self.styles.addWithId(self.memory, other_style, src_cell.style_id)) |id| { + dst_cell.style_id = id; + } } } } @@ -767,13 +772,7 @@ pub const Page = struct { for (cells) |*cell| { if (cell.style_id == style.default_id) continue; - if (self.styles.lookupId(self.memory, cell.style_id)) |prev_style| { - // Below upsert can't fail because it should already be present - const md = self.styles.upsert(self.memory, prev_style.*) catch unreachable; - assert(md.ref > 0); - md.ref -= 1; - if (md.ref == 0) self.styles.remove(self.memory, cell.style_id); - } + self.styles.release(self.memory, cell.style_id); } if (cells.len == self.size.cols) row.styled = false; @@ -2112,7 +2111,7 @@ test "Page verifyIntegrity styles good" { defer page.deinit(); // Upsert a style we'll use - const md = try page.styles.upsert(page.memory, .{ .flags = .{ + const id = try page.styles.add(page.memory, .{ .flags = .{ .bold = true, } }); @@ -2123,11 +2122,15 @@ test "Page verifyIntegrity styles good" { rac.cell.* = .{ .content_tag = .codepoint, .content = .{ .codepoint = @intCast(x + 1) }, - .style_id = md.id, + .style_id = id, }; - md.ref += 1; + page.styles.use(page.memory, id); } + // The original style add would have incremented the + // ref count too, so release it to balance that out. + page.styles.release(page.memory, id); + try page.verifyIntegrity(testing.allocator); } @@ -2140,7 +2143,7 @@ test "Page verifyIntegrity styles ref count mismatch" { defer page.deinit(); // Upsert a style we'll use - const md = try page.styles.upsert(page.memory, .{ .flags = .{ + const id = try page.styles.add(page.memory, .{ .flags = .{ .bold = true, } }); @@ -2151,13 +2154,17 @@ test "Page verifyIntegrity styles ref count mismatch" { rac.cell.* = .{ .content_tag = .codepoint, .content = .{ .codepoint = @intCast(x + 1) }, - .style_id = md.id, + .style_id = id, }; - md.ref += 1; + page.styles.use(page.memory, id); } + // The original style add would have incremented the + // ref count too, so release it to balance that out. + page.styles.release(page.memory, id); + // Miss a ref - md.ref -= 1; + page.styles.release(page.memory, id); try testing.expectError( Page.IntegrityError.MismatchedStyleRef, diff --git a/src/terminal/ref_counted_set.zig b/src/terminal/ref_counted_set.zig new file mode 100644 index 000000000..19d938cfc --- /dev/null +++ b/src/terminal/ref_counted_set.zig @@ -0,0 +1,573 @@ +const size = @import("size.zig"); +const Offset = size.Offset; +const OffsetBuf = size.OffsetBuf; + +const fastmem = @import("../fastmem.zig"); + +const std = @import("std"); +const assert = std.debug.assert; + +/// A reference counted set. +/// +/// This set is created with some capacity in mind. You can determine +/// the exact memory requirement of a given capacity by calling `layout` +/// and checking the total size. +/// +/// When the set exceeds capacity, `error.OutOfMemory` is returned from +/// any memory-using methods. The caller is responsible for determining +/// a path forward. +/// +/// This set is reference counted. Each item in the set has an associated +/// reference count. The caller is responsible for calling release for an +/// item when it is no longer being used. Items with 0 references will be +/// kept until another item is written to their bucket. This allows items +/// to be ressurected if they are re-added before they get overwritten. +/// +/// The backing data structure of this set is an open addressed hash table +/// with linear probing and Robin Hood hashing, and a flat array of items. +/// +/// The table maps values to item IDs, which are indices in the item array +/// which contain the item's value and its reference count. Item IDs can be +/// used to efficiently access an item and update its reference count after +/// it has been added to the table, to avoid having to use the hash map to +/// look the value back up. +/// +/// ID 0 is reserved and will never be assigned. +/// +/// Parameters: +/// +/// `Context` +/// A type containing methods to define behaviors. +/// - `fn hash(*Context, T) u64` - Return a hash for an item. +/// - `fn eql(*Context, T, T) bool` - Check two items for equality. +/// +/// - `fn deleted(*Context, T) void` - [OPTIONAL] Deletion callback. +/// If present, called whenever an item is finally deleted. +/// Useful if the item has memory that needs to be freed. +/// +pub fn RefCountedSet( + comptime T: type, + comptime Id: type, + comptime RefCountInt: type, + comptime Context: type, +) type { + return struct { + const Self = RefCountedSet(T, Id, RefCountInt, Context); + + pub const base_align = @max( + @alignOf(Context), + @alignOf(Layout), + @alignOf(Item), + @alignOf(Id), + ); + + /// Set item + pub const Item = struct { + /// The value this item represents. + value: T = undefined, + /// Metadata for this item. + meta: Metadata = .{}, + + pub const Metadata = struct { + /// The bucket in the hash table where this item + /// is referenced. + bucket: Id = std.math.maxInt(Id), + + /// The length of the probe sequence between this + /// item's starting bucket and the bucket it's in, + /// used for Robin Hood hashing. + psl: Id = 0, + + /// The reference count for this item. + ref: RefCountInt = 0, + }; + }; + + /// A hash table of item indices + table: Offset(Id), + + /// By keeping track of the max probe sequence length + /// we can bail out early when looking up values that + /// aren't present. + max_psl: Id = 0, + + /// We keep track of how many items have a PSL of any + /// given length, so that we can shrink max_psl when + /// we delete items. + /// + /// A probe sequence of length 32 or more is astronomically + /// unlikely. Roughly a (1/table_cap)^32 -- with any normal + /// table capacity that is so unlikely that it's not worth + /// handling. + psl_stats: [32]Id = [_]Id{0} ** 32, + + /// The backing store of items + items: Offset(Item), + + /// The next index to store an item at. + /// Id 0 is reserved for unused items. + next_id: Id = 1, + + layout: Layout, + + /// An instance of the context structure. + context: Context, + + /// Returns the memory layout for the given base offset and + /// desired capacity. The layout can be used by the caller to + /// determine how much memory to allocate, and the layout must + /// be used to initialize the set so that the set knows all + /// the offsets for the various buffers. + /// + /// The capacity passed for cap will be used for the hash table, + /// which has a load factor of `0.8125` (13/16), so the number of + /// items which can actually be stored in the set will be smaller. + /// + /// The laid out capacity will be at least `cap`, but may be higher, + /// since it is rounded up to the next power of 2 for efficiency. + /// + /// The returned layout `cap` property will be 1 more than the number + /// of items that the set can actually store, since ID 0 is reserved. + pub fn layout(cap: Id) Layout { + // Experimentally, this load factor works quite well. + const load_factor = 0.8125; + + const table_cap: Id = std.math.ceilPowerOfTwoAssert(Id, cap); + const table_mask: Id = (@as(Id, 1) << std.math.log2_int(Id, table_cap)) - 1; + const items_cap: Id = @intFromFloat(load_factor * @as(f64, @floatFromInt(table_cap))); + + const table_start = 0; + const table_end = table_start + table_cap * @sizeOf(Id); + + const items_start = std.mem.alignForward(usize, table_end, @alignOf(Item)); + const items_end = items_start + items_cap * @sizeOf(Item); + + const total_size = items_end; + + return .{ + .cap = items_cap, + .table_cap = table_cap, + .table_mask = table_mask, + .table_start = table_start, + .items_start = items_start, + .total_size = total_size, + }; + } + + pub const Layout = struct { + cap: Id, + table_cap: Id, + table_mask: Id, + table_start: usize, + items_start: usize, + total_size: usize, + }; + + pub fn init(base: OffsetBuf, l: Layout, context: Context) Self { + const table = base.member(Id, l.table_start); + const items = base.member(Item, l.items_start); + + @memset(table.ptr(base)[0..l.table_cap], 0); + @memset(items.ptr(base)[0..l.cap], .{}); + + return .{ + .table = table, + .items = items, + .layout = l, + .context = context, + }; + } + + /// Add an item to the set if not present + /// and increment its reference count. + /// + /// Returns the item's ID. + /// + /// If the set has no more room, then an + /// OutOfMemory error is returned instead. + pub fn add(self: *Self, base: anytype, value: T) error{OutOfMemory}!Id { + const items = self.items.ptr(base); + + // Trim dead items from the end of the list. + while (self.next_id > 1 and items[self.next_id - 1].meta.ref == 0) { + self.next_id -= 1; + + self.deleteItem(base, self.next_id); + } + + // If we still don't have an available ID, we're out of memory. + if (self.next_id >= self.layout.cap) { + return error.OutOfMemory; + } + + const id = self.upsert(base, value, self.next_id); + items[id].meta.ref += 1; + + if (id == self.next_id) { + self.next_id += 1; + } + + return id; + } + + /// Add an item to the set if not present + /// and increment its reference count. + /// If possible, use the provided ID. + /// + /// Returns the item's ID, or null + /// if the provided ID was used. + /// + /// If the set has no more room, then an + /// OutOfMemory error is returned instead. + pub fn addWithId(self: *Self, base: anytype, value: T, id: Id) error{OutOfMemory}!?Id { + const items = self.items.ptr(base); + + if (id < self.next_id) { + if (items[id].meta.ref == 0) { + self.deleteItem(base, id); + + const added_id = self.upsert(base, value, id); + + items[added_id].meta.ref += 1; + + return if (added_id == id) null else added_id; + } else if (self.context.eql(value, items[id].value)) { + items[id].meta.ref += 1; + + return null; + } + } + + return try self.add(base, value); + } + + /// Increment an item's reference count by 1. + /// + /// Asserts that the item's reference count is greater than 0. + pub fn use(self: *const Self, base: anytype, id: Id) void { + assert(id > 0); + assert(id < self.layout.cap); + + const items = self.items.ptr(base); + const item = &items[id]; + + // If `use` is being called on an item with 0 references, then + // either someone forgot to call it before, released too early + // or lied about releasing. In any case something is wrong and + // shouldn't be allowed. + assert(item.meta.ref > 0); + + item.meta.ref += 1; + } + + /// Increment an item's reference count by a specified number. + /// + /// Asserts that the item's reference count is greater than 0. + pub fn useMultiple(self: *const Self, base: anytype, id: Id, n: RefCountInt) void { + assert(id > 0); + assert(id < self.layout.cap); + + const items = self.items.ptr(base); + const item = &items[id]; + + // If `use` is being called on an item with 0 references, then + // either someone forgot to call it before, released too early + // or lied about releasing. In any case something is wrong and + // shouldn't be allowed. + assert(item.meta.ref > 0); + + item.meta.ref += n; + } + + /// Get an item by its ID without incrementing its reference count. + /// + /// Asserts that the item's reference count is greater than 0. + pub fn get(self: *const Self, base: anytype, id: Id) *T { + assert(id > 0); + assert(id < self.layout.cap); + + const items = self.items.ptr(base); + const item = &items[id]; + + assert(item.meta.ref > 0); + + return @ptrCast(&item.value); + } + + /// Releases a reference to an item by its ID. + /// + /// Asserts that the item's reference count is greater than 0. + pub fn release(self: *Self, base: anytype, id: Id) void { + assert(id > 0); + assert(id < self.layout.cap); + + const items = self.items.ptr(base); + const item = &items[id]; + + assert(item.meta.ref > 0); + item.meta.ref -= 1; + } + + /// Release a specified number of references to an item by its ID. + /// + /// Asserts that the item's reference count is at least `n`. + pub fn releaseMultiple(self: *Self, base: anytype, id: Id, n: Id) void { + assert(id > 0); + assert(id < self.layout.cap); + + const items = self.items.ptr(base); + const item = &items[id]; + + assert(item.meta.ref >= n); + item.meta.ref -= n; + } + + /// Get the ref count for an item by its ID. + pub fn refCount(self: *const Self, base: anytype, id: Id) RefCountInt { + assert(id > 0); + assert(id < self.layout.cap); + + const items = self.items.ptr(base); + const item = &items[id]; + return item.meta.ref; + } + + /// Get the current number of non-dead items in the set. + /// + /// NOT DESIGNED TO BE USED OUTSIDE OF TESTING, this is a very slow + /// operation, since it traverses the entire structure to count. + /// + /// Additionally, because this is a testing method, it does extra + /// work to verify the integrity of the structure when called. + pub fn count(self: *const Self, base: anytype) usize { + const table = self.table.ptr(base); + const items = self.items.ptr(base); + + // The number of items accessible through the table. + var tb_ct: usize = 0; + + for (table[0..self.layout.table_cap]) |id| { + if (id != 0) { + const item = items[id]; + if (item.meta.ref > 0) { + tb_ct += 1; + } + } + } + + // The number of items accessible through the backing store. + // The two counts should always match- it shouldn't be possible + // to have untracked items in the backing store. + var it_ct: usize = 0; + + for (items[0..self.layout.cap]) |it| { + if (it.meta.ref > 0) { + it_ct += 1; + } + } + + assert(tb_ct == it_ct); + + return tb_ct; + } + + //================================================// + // The functions below are all internal functions // + // for performing operations on the hash table. // + //================================================// + + /// Delete an item, removing any references from + /// the table, and freeing its ID to be re-used. + fn deleteItem(self: *Self, base: anytype, id: Id) void { + const table = self.table.ptr(base); + const items = self.items.ptr(base); + + const item = items[id]; + + if (item.meta.bucket > self.layout.table_cap) { + return; + } + + if (table[item.meta.bucket] != id) { + return; + } + + if (comptime @hasDecl(Context, "deleted")) { + // Inform the context struct that we're + // deleting the dead item's value for good. + self.context.deleted(item.value); + } + + self.psl_stats[item.meta.psl] -= 1; + table[item.meta.bucket] = 0; + items[id] = .{}; + + var p: Id = item.meta.bucket; + var n: Id = (p + 1) & self.layout.table_mask; + + while (table[n] != 0 and items[table[n]].meta.psl > 0) { + items[table[n]].meta.bucket = p; + self.psl_stats[items[table[n]].meta.psl] -= 1; + items[table[n]].meta.psl -= 1; + self.psl_stats[items[table[n]].meta.psl] += 1; + table[p] = table[n]; + p = n; + n = (n + 1) & self.layout.table_mask; + } + + while (self.max_psl > 0 and self.psl_stats[self.max_psl] == 0) { + self.max_psl -= 1; + } + + table[p] = 0; + } + + /// Find an item in the table and return its ID. + /// If the item does not exist in the table, null is returned. + fn lookup(self: *Self, base: anytype, value: T) ?Id { + const table = self.table.ptr(base); + const items = self.items.ptr(base); + + const hash: u64 = self.context.hash(value); + + for (0..self.max_psl + 1) |i| { + const p = (hash + i) & self.layout.table_mask; + + const id = table[p]; + + // Empty bucket, our item cannot have probed to + // any point after this, meaning it's not present. + if (id == 0) { + return null; + } + + const item = items[id]; + + // An item with a shorter probe sequence length would never + // end up in the middle of another sequence, since it would + // be swapped out if inserted before the new sequence, and + // would not be swapped in if inserted afterwards. + // + // As such, our item cannot be present. + if (item.meta.psl < i) { + return null; + } + + // We don't bother checking dead items. + if (item.meta.ref == 0) { + continue; + } + + // If the item is a part of the same probe sequence, + // we check if it matches the value we're looking for. + if (item.meta.psl == i and + self.context.eql(value, item.value)) + { + return id; + } + } + + return null; + } + + /// Find the provided value in the hash table, or add a new item + /// for it if not present. If a new item is added, `new_id` will + /// be used as the ID. + fn upsert(self: *Self, base: anytype, value: T, new_id: Id) Id { + // If the item already exists, return it. + if (self.lookup(base, value)) |id| { + return id; + } + + const table = self.table.ptr(base); + const items = self.items.ptr(base); + + // The new item that we'll put in to the table. + var new_item: Item = .{ + .value = value, + .meta = .{ + .psl = 0, + .ref = 0, + }, + }; + + const hash: u64 = self.context.hash(value); + + var held_id: Id = new_id; + var held_item: *Item = &new_item; + + var chosen_p: ?Id = null; + var chosen_id: Id = new_id; + + for (0..self.layout.table_cap - 1) |i| { + const p: Id = @intCast((hash + i) & self.layout.table_mask); + const id = table[p]; + + // Empty bucket, put our held item in to it and break. + if (id == 0) { + table[p] = held_id; + held_item.meta.bucket = p; + self.psl_stats[held_item.meta.psl] += 1; + self.max_psl = @max(self.max_psl, held_item.meta.psl); + + break; + } + + const item = &items[id]; + + // If there's a dead item then we resurrect it + // for our value so that we can re-use its ID. + if (item.meta.ref == 0) { + if (comptime @hasDecl(Context, "deleted")) { + // Inform the context struct that we're + // deleting the dead item's value for good. + self.context.deleted(item.value); + } + + chosen_id = id; + + held_item.meta.bucket = p; + self.psl_stats[held_item.meta.psl] += 1; + self.max_psl = @max(self.max_psl, held_item.meta.psl); + + // If we're not still holding our new item then we + // need to make sure that we put the re-used ID in + // the right place, where we previously put new_id. + if (chosen_p) |c| { + table[c] = id; + table[p] = held_id; + } else { + // If we're still holding our new item then we + // don't actually have to do anything, because + // the table already has the correct ID here. + } + + break; + } + + // This item has a lower PSL, swap it out with our held item. + if (item.meta.psl < held_item.meta.psl) { + if (held_id == new_id) { + chosen_p = p; + new_item.meta.bucket = p; + } + + table[p] = held_id; + items[held_id].meta.bucket = p; + self.psl_stats[held_item.meta.psl] += 1; + self.max_psl = @max(self.max_psl, held_item.meta.psl); + + held_id = id; + held_item = item; + self.psl_stats[item.meta.psl] -= 1; + } + + // Advance to the next probe position for our held item. + held_item.meta.psl += 1; + } + + items[chosen_id] = new_item; + return chosen_id; + } + }; +} diff --git a/src/terminal/style.zig b/src/terminal/style.zig index 7e3356bc8..b5bfff6e0 100644 --- a/src/terminal/style.zig +++ b/src/terminal/style.zig @@ -6,8 +6,11 @@ const page = @import("page.zig"); const size = @import("size.zig"); const Offset = size.Offset; const OffsetBuf = size.OffsetBuf; -const hash_map = @import("hash_map.zig"); -const AutoOffsetHashMap = hash_map.AutoOffsetHashMap; + +const Wyhash = std.hash.Wyhash; +const autoHash = std.hash.autoHash; + +const RefCountedSet = @import("ref_counted_set.zig").RefCountedSet; /// The unique identifier for a style. This is at most the number of cells /// that can fit into a terminal page. @@ -43,11 +46,34 @@ pub const Style = struct { none: void, palette: u8, rgb: color.RGB, + + /// Formatting to make debug logs easier to read + /// by only including non-default attributes. + pub fn format( + self: Color, + comptime fmt: []const u8, + options: std.fmt.FormatOptions, + writer: anytype, + ) !void { + _ = fmt; + _ = options; + switch (self) { + .none => { + _ = try writer.write("Color.none"); + }, + .palette => |p| { + _ = try writer.print("Color.palette{{ {} }}", .{ p }); + }, + .rgb => |rgb| { + _ = try writer.print("Color.rgb{{ {}, {}, {} }}", .{ rgb.r, rgb.g, rgb.b }); + } + } + } }; /// True if the style is the default style. pub fn default(self: Style) bool { - return std.meta.eql(self, .{}); + return self.eql(.{}); } /// True if the style is equal to another style. @@ -133,6 +159,83 @@ pub const Style = struct { }; } + /// Formatting to make debug logs easier to read + /// by only including non-default attributes. + pub fn format( + self: Style, + comptime fmt: []const u8, + options: std.fmt.FormatOptions, + writer: anytype, + ) !void { + _ = fmt; + _ = options; + + const dflt: Style = .{}; + + _ = try writer.write("Style{ "); + + var started = false; + + inline for (std.meta.fields(Style)) |f| { + if (std.mem.eql(u8, f.name, "flags")) { + if (started) { + _ = try writer.write(", "); + } + + _ = try writer.write("flags={ "); + + started = false; + + inline for (std.meta.fields(@TypeOf(self.flags))) |ff| { + const v = @as(ff.type, @field(self.flags, ff.name)); + const d = @as(ff.type, @field(dflt.flags, ff.name)); + if (ff.type == bool) { + if (v) { + if (started) { + _ = try writer.write(", "); + } + _ = try writer.print("{s}", .{ff.name}); + started = true; + } + } else if (!std.meta.eql(v, d)) { + if (started) { + _ = try writer.write(", "); + } + _ = try writer.print( + "{s}={any}", + .{ ff.name, v }, + ); + started = true; + } + } + _ = try writer.write(" }"); + + started = true; + comptime continue; + } + const value = @as(f.type, @field(self, f.name)); + const d_val = @as(f.type, @field(dflt, f.name)); + if (!std.meta.eql(value, d_val)) { + if (started) { + _ = try writer.write(", "); + } + _ = try writer.print( + "{s}={any}", + .{ f.name, value }, + ); + started = true; + } + } + + _ = try writer.write(" }"); + } + + pub fn hash(self: *const Style) u64 { + var hasher = Wyhash.init(0); + autoHash(&hasher, self.*); + return hasher.final(); + } + test { // The size of the struct so we can be aware of changes. const testing = std.testing; @@ -140,170 +243,24 @@ pub const Style = struct { } }; -/// A set of styles. -/// -/// This set is created with some capacity in mind. You can determine -/// the exact memory requirement for a capacity by calling `layout` -/// and checking the total size. -/// -/// When the set exceeds capacity, `error.OutOfMemory` is returned -/// from memory-using methods. The caller is responsible for determining -/// a path forward. -/// -/// The general idea behind this structure is that it is optimized for -/// the scenario common in terminals where there aren't many unique -/// styles, and many cells are usually drawn with a single style before -/// changing styles. -/// -/// Callers should call `upsert` when a new style is set. This will -/// return a stable pointer to metadata. You should use this metadata -/// to keep a ref count of the style usage. When it falls to zero you -/// can remove it. -pub const Set = struct { - pub const base_align = @max(MetadataMap.base_align, IdMap.base_align); - - /// The mapping of a style to associated metadata. This is - /// the map that contains the actual style definitions - /// (in the form of the key). - styles: MetadataMap, - - /// The mapping from ID to style. - id_map: IdMap, - - /// The next ID to use for a style that isn't in the set. - /// When this overflows we'll begin returning an IdOverflow - /// error and the caller must manually compact the style - /// set. - /// - /// Id zero is reserved and always is the default style. The - /// default style isn't present in the map, its dependent on - /// the terminal configuration. - next_id: Id = 1, - - /// Maps a style definition to metadata about that style. - const MetadataMap = AutoOffsetHashMap(Style, Metadata); - - /// Maps the unique style ID to the concrete style definition. - const IdMap = AutoOffsetHashMap(Id, Offset(Style)); - - /// Returns the memory layout for the given base offset and - /// desired capacity. The layout can be used by the caller to - /// determine how much memory to allocate, and the layout must - /// be used to initialize the set so that the set knows all - /// the offsets for the various buffers. - pub fn layout(cap: usize) Layout { - const md_layout = MetadataMap.layout(@intCast(cap)); - const md_start = 0; - const md_end = md_start + md_layout.total_size; - - const id_layout = IdMap.layout(@intCast(cap)); - const id_start = std.mem.alignForward(usize, md_end, IdMap.base_align); - const id_end = id_start + id_layout.total_size; - - const total_size = id_end; - - return .{ - .md_start = md_start, - .md_layout = md_layout, - .id_start = id_start, - .id_layout = id_layout, - .total_size = total_size, - }; +pub const StyleSetContext = struct { + pub fn hash(self: *StyleSetContext, style: Style) u64 { + _ = self; + return style.hash(); } - pub const Layout = struct { - md_start: usize, - md_layout: MetadataMap.Layout, - id_start: usize, - id_layout: IdMap.Layout, - total_size: usize, - }; - - pub fn init(base: OffsetBuf, l: Layout) Set { - const styles_buf = base.add(l.md_start); - const id_buf = base.add(l.id_start); - return .{ - .styles = MetadataMap.init(styles_buf, l.md_layout), - .id_map = IdMap.init(id_buf, l.id_layout), - }; - } - - /// Possible errors for upsert. - pub const UpsertError = error{ - /// No more space in the backing buffer. Remove styles or - /// grow and reinitialize. - OutOfMemory, - - /// No more available IDs. Perform a garbage collection - /// operation to compact ID space. - Overflow, - }; - - /// Upsert a style into the set and return a pointer to the metadata - /// for that style. The pointer is valid for the lifetime of the set - /// so long as the style is not removed. - /// - /// The ref count for new styles is initialized to zero and - /// for existing styles remains unmodified. - pub fn upsert(self: *Set, base: anytype, style: Style) UpsertError!*Metadata { - // If we already have the style in the map, this is fast. - var map = self.styles.map(base); - const gop = try map.getOrPut(style); - if (gop.found_existing) return gop.value_ptr; - - // New style, we need to setup all the metadata. First thing, - // let's get the ID we'll assign, because if we're out of space - // we need to fail early. - errdefer map.removeByPtr(gop.key_ptr); - const id = self.next_id; - self.next_id = try std.math.add(Id, self.next_id, 1); - errdefer self.next_id -= 1; - gop.value_ptr.* = .{ .id = id }; - - // Setup our ID mapping - var id_map = self.id_map.map(base); - const id_gop = try id_map.getOrPut(id); - errdefer id_map.removeByPtr(id_gop.key_ptr); - assert(!id_gop.found_existing); - id_gop.value_ptr.* = size.getOffset(Style, base, gop.key_ptr); - return gop.value_ptr; - } - - /// Lookup a style by its unique identifier. - pub fn lookupId(self: *const Set, base: anytype, id: Id) ?*Style { - const id_map = self.id_map.map(base); - const offset = id_map.get(id) orelse return null; - return @ptrCast(offset.ptr(base)); - } - - /// Remove a style by its id. - pub fn remove(self: *Set, base: anytype, id: Id) void { - // Lookup by ID, if it doesn't exist then we return. We use - // getEntry so that we can make removal faster later by using - // the entry's key pointer. - var id_map = self.id_map.map(base); - const id_entry = id_map.getEntry(id) orelse return; - - var style_map = self.styles.map(base); - const style_ptr: *Style = @ptrCast(id_entry.value_ptr.ptr(base)); - - id_map.removeByPtr(id_entry.key_ptr); - style_map.removeByPtr(style_ptr); - } - - /// Return the number of styles currently in the set. - pub fn count(self: *const Set, base: anytype) usize { - return self.id_map.map(base).count(); + pub fn eql(self: *StyleSetContext, a: Style, b: Style) bool { + _ = self; + return a.eql(b); } }; -/// Metadata about a style. This is used to track the reference count -/// and the unique identifier for a style. The unique identifier is used -/// to track the style in the full style map. -pub const Metadata = struct { - ref: size.CellCountInt = 0, - id: Id = 0, -}; +pub const Set = RefCountedSet( + Style, + Id, + size.CellCountInt, + StyleSetContext, +); test "Set basic usage" { const testing = std.testing; @@ -313,29 +270,49 @@ test "Set basic usage" { defer alloc.free(buf); const style: Style = .{ .flags = .{ .bold = true } }; + const style2: Style = .{ .flags = .{ .italic = true } }; - var set = Set.init(OffsetBuf.init(buf), layout); + var set = Set.init(OffsetBuf.init(buf), layout, StyleSetContext{}); - // Upsert - const meta = try set.upsert(buf, style); - try testing.expect(meta.id > 0); + // Add style + const id = try set.add(buf, style); + try testing.expect(id > 0); - // Second upsert should return the same metadata. + // Second add should return the same metadata. { - const meta2 = try set.upsert(buf, style); - try testing.expectEqual(meta.id, meta2.id); + const id2 = try set.add(buf, style); + try testing.expectEqual(id, id2); } // Look it up { - const v = set.lookupId(buf, meta.id).?; + const v = set.get(buf, id); try testing.expect(v.flags.bold); - const v2 = set.lookupId(buf, meta.id).?; + const v2 = set.get(buf, id); try testing.expectEqual(v, v2); } - // Removal - set.remove(buf, meta.id); - try testing.expect(set.lookupId(buf, meta.id) == null); + // Add a second style + const id2 = try set.add(buf, style2); + + // Look it up + { + const v = set.get(buf, id2); + try testing.expect(v.flags.italic); + } + + // Ref count + try testing.expect(set.refCount(buf, id) == 2); + try testing.expect(set.refCount(buf, id2) == 1); + + // Release + set.release(buf, id); + try testing.expect(set.refCount(buf, id) == 1); + set.release(buf, id2); + try testing.expect(set.refCount(buf, id2) == 0); + + // We added the first one twice, so + set.release(buf, id); + try testing.expect(set.refCount(buf, id) == 0); } From 04896a14b43d2f19281c03fedcaf291507ad2d4e Mon Sep 17 00:00:00 2001 From: Qwerasd Date: Fri, 14 Jun 2024 00:14:25 -0400 Subject: [PATCH 04/15] perf(shaper/coretext): cache fonts between shape calls --- src/font/shaper/coretext.zig | 144 +++++++++++++++++++++++------------ 1 file changed, 94 insertions(+), 50 deletions(-) diff --git a/src/font/shaper/coretext.zig b/src/font/shaper/coretext.zig index 9ce929e12..4b4f2abad 100644 --- a/src/font/shaper/coretext.zig +++ b/src/font/shaper/coretext.zig @@ -31,7 +31,7 @@ const log = std.log.scoped(.font_shaper); /// See: https://github.com/mitchellh/ghostty/issues/1643 /// pub const Shaper = struct { - /// The allocated used for the feature list and cell buf. + /// The allocated used for the feature list, font cache, and cell buf. alloc: Allocator, /// The string used for shaping the current run. @@ -49,6 +49,13 @@ pub const Shaper = struct { /// and releasing many objects when shaping. writing_direction: *macos.foundation.Array, + /// List where we cache fonts, so we don't have to + /// remake them for every single shaping operation. + /// + /// Fonts are cached as attribute dictionaries to + /// be applied directly to attributed strings. + cached_fonts: std.ArrayList(?*macos.foundation.Dictionary), + const CellBuf = std.ArrayListUnmanaged(font.shape.Cell); const CodepointList = std.ArrayListUnmanaged(Codepoint); const Codepoint = struct { @@ -202,12 +209,16 @@ pub const Shaper = struct { }; errdefer writing_direction.release(); + const cached_fonts = std.ArrayList(?*macos.foundation.Dictionary).init(alloc); + errdefer cached_fonts.deinit(); + return Shaper{ .alloc = alloc, .cell_buf = .{}, .run_state = run_state, .features = feats, .writing_direction = writing_direction, + .cached_fonts = cached_fonts, }; } @@ -216,6 +227,18 @@ pub const Shaper = struct { self.run_state.deinit(self.alloc); self.features.deinit(); self.writing_direction.release(); + + self.releaseCachedFonts(); + self.cached_fonts.deinit(); + } + + /// Release all cached fonts. + pub fn releaseCachedFonts(self: *Shaper) void { + for (self.cached_fonts.items) |ft| { + if (ft) |f| { + f.release(); + } + } } pub fn runIterator( @@ -267,55 +290,7 @@ pub const Shaper = struct { defer arena.deinit(); const alloc = arena.allocator(); - // Get our font. We have to apply the font features we want for - // the font here. - const run_font: *macos.text.Font = font: { - // The CoreText shaper relies on CoreText and CoreText claims - // that CTFonts are threadsafe. See: - // https://developer.apple.com/documentation/coretext/ - // - // Quote: - // All individual functions in Core Text are thread-safe. Font - // objects (CTFont, CTFontDescriptor, and associated objects) can - // be used simultaneously by multiple operations, work queues, or - // threads. However, the layout objects (CTTypesetter, - // CTFramesetter, CTRun, CTLine, CTFrame, and associated objects) - // should be used in a single operation, work queue, or thread. - // - // Because of this, we only acquire the read lock to grab the - // face and set it up, then release it. - run.grid.lock.lockShared(); - defer run.grid.lock.unlockShared(); - - const face = try run.grid.resolver.collection.getFace(run.font_index); - const original = face.font; - - const attrs = try self.features.attrsDict(face.quirks_disable_default_font_features); - defer attrs.release(); - - const desc = try macos.text.FontDescriptor.createWithAttributes(attrs); - defer desc.release(); - - const copied = try original.copyWithAttributes(0, null, desc); - errdefer copied.release(); - break :font copied; - }; - defer run_font.release(); - - // Get our font and use that get the attributes to set for the - // attributed string so the whole string uses the same font. - const attr_dict = dict: { - var keys = [_]?*const anyopaque{ - macos.text.StringAttribute.font.key(), - macos.text.StringAttribute.writing_direction.key(), - }; - var values = [_]?*const anyopaque{ - run_font, - self.writing_direction, - }; - break :dict try macos.foundation.Dictionary.create(&keys, &values); - }; - defer attr_dict.release(); + const attr_dict: *macos.foundation.Dictionary = try self.getFont(run.grid, run.font_index); // Create an attributed string from our string const attr_str = try macos.foundation.AttributedString.create( @@ -416,6 +391,75 @@ pub const Shaper = struct { return self.cell_buf.items; } + /// Get an attr dict for a font from a specific index. + /// These items are cached, do not retain or release them. + fn getFont(self: *Shaper, grid: *font.SharedGrid, index: font.Collection.Index) !*macos.foundation.Dictionary { + const index_int = index.int(); + + if (self.cached_fonts.items.len <= index_int) { + try self.cached_fonts.ensureTotalCapacity(index_int + 1); + while (self.cached_fonts.items.len <= index_int) { + self.cached_fonts.appendAssumeCapacity(null); + } + } + + if (self.cached_fonts.items[index_int]) |cached| { + return cached; + } + + const run_font = font: { + // The CoreText shaper relies on CoreText and CoreText claims + // that CTFonts are threadsafe. See: + // https://developer.apple.com/documentation/coretext/ + // + // Quote: + // All individual functions in Core Text are thread-safe. Font + // objects (CTFont, CTFontDescriptor, and associated objects) can + // be used simultaneously by multiple operations, work queues, or + // threads. However, the layout objects (CTTypesetter, + // CTFramesetter, CTRun, CTLine, CTFrame, and associated objects) + // should be used in a single operation, work queue, or thread. + // + // Because of this, we only acquire the read lock to grab the + // face and set it up, then release it. + grid.lock.lockShared(); + defer grid.lock.unlockShared(); + + const face = try grid.resolver.collection.getFace(index); + const original = face.font; + + const attrs = try self.features.attrsDict(face.quirks_disable_default_font_features); + defer attrs.release(); + + const desc = try macos.text.FontDescriptor.createWithAttributes(attrs); + defer desc.release(); + + const copied = try original.copyWithAttributes(0, null, desc); + errdefer copied.release(); + + break :font copied; + }; + defer run_font.release(); + + // Get our font and use that get the attributes to set for the + // attributed string so the whole string uses the same font. + const attr_dict = dict: { + var keys = [_]?*const anyopaque{ + macos.text.StringAttribute.font.key(), + macos.text.StringAttribute.writing_direction.key(), + }; + var values = [_]?*const anyopaque{ + run_font, + self.writing_direction, + }; + break :dict try macos.foundation.Dictionary.create(&keys, &values); + }; + + self.cached_fonts.items[index_int] = attr_dict; + + return attr_dict; + } + /// The hooks for RunIterator. pub const RunIteratorHook = struct { shaper: *Shaper, From 626ec2b5ac705d1d382ce605415f073482d8cfd9 Mon Sep 17 00:00:00 2001 From: Qwerasd Date: Fri, 14 Jun 2024 01:43:02 -0400 Subject: [PATCH 05/15] perf: introduce CFReleaseThread for running CoreFoundation releases Some CoreFoundation objects, such as those produced by CoreText, have expensive callbacks that run when they are released. By offloading the CFRelease calls to another thread, we can avoid important threads being blocked by unexpectedly expensive callbacks. This commit also changes the way that the coretext shaper's run iterator builds its string. Rather than using a CFMutableString, an ArrayList of unichars is built which is passed to CFStringCreateWithCharactersNoCopy, which is a lot more efficient since it avoids all the CoreFoundation overhead. --- pkg/macos/foundation/string.zig | 11 ++ src/cf_release_thread.zig | 185 ++++++++++++++++++++++++++++++++ src/font/shaper/coretext.zig | 72 +++++++++---- src/renderer/Metal.zig | 64 ++++++++++- 4 files changed, 307 insertions(+), 25 deletions(-) create mode 100644 src/cf_release_thread.zig diff --git a/pkg/macos/foundation/string.zig b/pkg/macos/foundation/string.zig index b642201de..f1f437140 100644 --- a/pkg/macos/foundation/string.zig +++ b/pkg/macos/foundation/string.zig @@ -19,6 +19,17 @@ pub const String = opaque { )))) orelse Allocator.Error.OutOfMemory; } + pub fn createWithCharactersNoCopy( + unichars: []const u16, + ) *String { + return @as(*String, @ptrFromInt(@intFromPtr(c.CFStringCreateWithCharactersNoCopy( + null, + @ptrCast(unichars.ptr), + @intCast(unichars.len), + foundation.c.kCFAllocatorNull, + )))); + } + pub fn release(self: *String) void { c.CFRelease(self); } diff --git a/src/cf_release_thread.zig b/src/cf_release_thread.zig new file mode 100644 index 000000000..6a37ca692 --- /dev/null +++ b/src/cf_release_thread.zig @@ -0,0 +1,185 @@ +//! Represents the CFRelease thread. Pools of CFTypeRefs are sent to +//! this thread to be released, so that their release callback logic +//! doesn't block the execution of a high throughput thread like the +//! renderer thread. +pub const Thread = @This(); + +const std = @import("std"); +const builtin = @import("builtin"); +const xev = @import("xev"); +const macos = @import("macos"); + +const BlockingQueue = @import("./blocking_queue.zig").BlockingQueue; + +const Allocator = std.mem.Allocator; +const log = std.log.scoped(.cf_release_thread); + +pub const Message = union(enum) { + /// Release a slice of CFTypeRefs. Uses alloc to + /// free the slice after releasing all the refs. + release: struct{ + refs: []*anyopaque, + alloc: Allocator, + }, +}; + +/// The type used for sending messages to the thread. For now this is +/// hardcoded with a capacity. We can make this a comptime parameter in +/// the future if we want it configurable. +pub const Mailbox = BlockingQueue(Message, 64); + +/// Allocator used for some state +alloc: std.mem.Allocator, + +/// The main event loop for the thread. The user data of this loop +/// is always the allocator used to create the loop. This is a convenience +/// so that users of the loop always have an allocator. +loop: xev.Loop, + +/// This can be used to wake up the thread. +wakeup: xev.Async, +wakeup_c: xev.Completion = .{}, + +/// This can be used to stop the thread on the next loop iteration. +stop: xev.Async, +stop_c: xev.Completion = .{}, + +/// The mailbox that can be used to send this thread messages. Note +/// this is a blocking queue so if it is full you will get errors (or block). +mailbox: *Mailbox, + +flags: packed struct { + /// This is set to true only when an abnormal exit is detected. It + /// tells our mailbox system to drain and ignore all messages. + drain: bool = false, +} = .{}, + +/// Initialize the thread. This does not START the thread. This only sets +/// up all the internal state necessary prior to starting the thread. It +/// is up to the caller to start the thread with the threadMain entrypoint. +pub fn init( + alloc: Allocator, +) !Thread { + // Create our event loop. + var loop = try xev.Loop.init(.{}); + errdefer loop.deinit(); + + // This async handle is used to "wake up" the thread to collect objects. + var wakeup_h = try xev.Async.init(); + errdefer wakeup_h.deinit(); + + // This async handle is used to stop the loop and force the thread to end. + var stop_h = try xev.Async.init(); + errdefer stop_h.deinit(); + + // The mailbox for messaging this thread + var mailbox = try Mailbox.create(alloc); + errdefer mailbox.destroy(alloc); + + return Thread{ + .alloc = alloc, + .loop = loop, + .wakeup = wakeup_h, + .stop = stop_h, + .mailbox = mailbox, + }; +} + +/// Clean up the thread. This is only safe to call once the thread +/// completes executing; the caller must join prior to this. +pub fn deinit(self: *Thread) void { + self.stop.deinit(); + self.wakeup.deinit(); + self.loop.deinit(); + + // Nothing can possibly access the mailbox anymore, destroy it. + self.mailbox.destroy(self.alloc); +} + +/// The main entrypoint for the thread. +pub fn threadMain(self: *Thread) void { + // Call child function so we can use errors... + self.threadMain_() catch |err| { + log.warn("error in cf release thread err={}", .{err}); + }; + + // If our loop is not stopped, then we need to keep running so that + // messages are drained and we can wait for the surface to send a stop + // message. + if (!self.loop.flags.stopped) { + log.warn("abrupt cf release thread exit detected, starting xev to drain mailbox", .{}); + defer log.debug("cf release thread fully exiting after abnormal failure", .{}); + self.flags.drain = true; + self.loop.run(.until_done) catch |err| { + log.err("failed to start xev loop for draining err={}", .{err}); + }; + } +} + +fn threadMain_(self: *Thread) !void { + defer log.debug("cf release thread exited", .{}); + + // Start the async handlers. We start these first so that they're + // registered even if anything below fails so we can drain the mailbox. + self.wakeup.wait(&self.loop, &self.wakeup_c, Thread, self, wakeupCallback); + self.stop.wait(&self.loop, &self.stop_c, Thread, self, stopCallback); + + // Run + log.debug("starting cf release thread", .{}); + defer log.debug("starting cf release thread shutdown", .{}); + try self.loop.run(.until_done); +} + +/// Drain the mailbox, handling all the messages in our terminal implementation. +fn drainMailbox(self: *Thread) !void { + // If we're draining, we just drain the mailbox and return. + if (self.flags.drain) { + while (self.mailbox.pop()) |_| {} + return; + } + + while (self.mailbox.pop()) |message| { + // log.debug("mailbox message={}", .{message}); + switch (message) { + .release => |msg| { + for (msg.refs) |ref| { + macos.foundation.CFRelease(ref); + } + // log.debug("Released {} CFTypeRefs.", .{ msg.refs.len }); + msg.alloc.free(msg.refs); + } + } + } +} + +fn wakeupCallback( + self_: ?*Thread, + _: *xev.Loop, + _: *xev.Completion, + r: xev.Async.WaitError!void, +) xev.CallbackAction { + _ = r catch |err| { + log.err("error in wakeup err={}", .{err}); + return .rearm; + }; + + const t = self_.?; + + // When we wake up, we check the mailbox. Mailbox producers should + // wake up our thread after publishing. + t.drainMailbox() catch |err| + log.err("error draining mailbox err={}", .{err}); + + return .rearm; +} + +fn stopCallback( + self_: ?*Thread, + _: *xev.Loop, + _: *xev.Completion, + r: xev.Async.WaitError!void, +) xev.CallbackAction { + _ = r catch unreachable; + self_.?.loop.stop(); + return .disarm; +} diff --git a/src/font/shaper/coretext.zig b/src/font/shaper/coretext.zig index 4b4f2abad..c32145019 100644 --- a/src/font/shaper/coretext.zig +++ b/src/font/shaper/coretext.zig @@ -64,24 +64,21 @@ pub const Shaper = struct { }; const RunState = struct { - str: *macos.foundation.MutableString, codepoints: CodepointList, + unichars: std.ArrayListUnmanaged(u16), - fn init() !RunState { - var str = try macos.foundation.MutableString.create(0); - errdefer str.release(); - return .{ .str = str, .codepoints = .{} }; + fn init() RunState { + return .{ .codepoints = .{}, .unichars = .{} }; } fn deinit(self: *RunState, alloc: Allocator) void { self.codepoints.deinit(alloc); - self.str.release(); + self.unichars.deinit(alloc); } fn reset(self: *RunState) !void { self.codepoints.clearRetainingCapacity(); - self.str.release(); - self.str = try macos.foundation.MutableString.create(0); + self.unichars.clearRetainingCapacity(); } }; @@ -184,7 +181,7 @@ pub const Shaper = struct { for (hardcoded_features) |name| try feats.append(name); for (opts.features) |name| try feats.append(name); - var run_state = try RunState.init(); + var run_state = RunState.init(); errdefer run_state.deinit(alloc); // For now we only support LTR text. If we shape RTL text then @@ -259,7 +256,14 @@ pub const Shaper = struct { }; } - pub fn shape(self: *Shaper, run: font.shape.TextRun) ![]const font.shape.Cell { + /// Expects an ArrayList `cf_release_pool` in which `CFTypeRef`s + /// can be placed, which guarantees that they will be `CFRelease`d + /// eventually. + pub fn shape( + self: *Shaper, + run: font.shape.TextRun, + cf_release_pool: *std.ArrayList(*anyopaque), + ) ![]const font.shape.Cell { const state = &self.run_state; // { @@ -290,18 +294,28 @@ pub const Shaper = struct { defer arena.deinit(); const alloc = arena.allocator(); - const attr_dict: *macos.foundation.Dictionary = try self.getFont(run.grid, run.font_index); + const attr_dict: *macos.foundation.Dictionary = try self.getFont( + run.grid, + run.font_index, + cf_release_pool, + ); + + // Make room for the attributed string and the CTLine. + try cf_release_pool.ensureUnusedCapacity(3); + + const str = macos.foundation.String.createWithCharactersNoCopy(state.unichars.items); + cf_release_pool.appendAssumeCapacity(str); // Create an attributed string from our string const attr_str = try macos.foundation.AttributedString.create( - state.str.string(), + str, attr_dict, ); - defer attr_str.release(); + cf_release_pool.appendAssumeCapacity(attr_str); // We should always have one run because we do our own run splitting. const line = try macos.text.Line.createWithAttributedString(attr_str); - defer line.release(); + cf_release_pool.appendAssumeCapacity(line); // This keeps track of the current offsets within a single cell. var cell_offset: struct { @@ -393,7 +407,12 @@ pub const Shaper = struct { /// Get an attr dict for a font from a specific index. /// These items are cached, do not retain or release them. - fn getFont(self: *Shaper, grid: *font.SharedGrid, index: font.Collection.Index) !*macos.foundation.Dictionary { + fn getFont( + self: *Shaper, + grid: *font.SharedGrid, + index: font.Collection.Index, + cf_release_pool: *std.ArrayList(*anyopaque), + ) !*macos.foundation.Dictionary { const index_int = index.int(); if (self.cached_fonts.items.len <= index_int) { @@ -407,6 +426,8 @@ pub const Shaper = struct { return cached; } + try cf_release_pool.ensureUnusedCapacity(3); + const run_font = font: { // The CoreText shaper relies on CoreText and CoreText claims // that CTFonts are threadsafe. See: @@ -429,17 +450,17 @@ pub const Shaper = struct { const original = face.font; const attrs = try self.features.attrsDict(face.quirks_disable_default_font_features); - defer attrs.release(); + cf_release_pool.appendAssumeCapacity(attrs); const desc = try macos.text.FontDescriptor.createWithAttributes(attrs); - defer desc.release(); + cf_release_pool.appendAssumeCapacity(desc); const copied = try original.copyWithAttributes(0, null, desc); errdefer copied.release(); break :font copied; }; - defer run_font.release(); + cf_release_pool.appendAssumeCapacity(run_font); // Get our font and use that get the attributes to set for the // attributed string so the whole string uses the same font. @@ -470,15 +491,20 @@ pub const Shaper = struct { } pub fn addCodepoint(self: RunIteratorHook, cp: u32, cluster: u32) !void { + const state = &self.shaper.run_state; + // Build our UTF-16 string for CoreText - var unichars: [2]u16 = undefined; + try state.unichars.ensureUnusedCapacity(self.shaper.alloc, 2); + + state.unichars.appendNTimesAssumeCapacity(0, 2); + const pair = macos.foundation.stringGetSurrogatePairForLongCharacter( cp, - &unichars, + state.unichars.items[state.unichars.items.len-2..][0..2], ); - const len: usize = if (pair) 2 else 1; - const state = &self.shaper.run_state; - state.str.appendCharacters(unichars[0..len]); + if (!pair) { + state.unichars.items.len -= 1; + } // Build our reverse lookup table for codepoints to clusters try state.codepoints.append(self.shaper.alloc, .{ diff --git a/src/renderer/Metal.zig b/src/renderer/Metal.zig index ca52ea362..5b85d9a4e 100644 --- a/src/renderer/Metal.zig +++ b/src/renderer/Metal.zig @@ -28,6 +28,8 @@ const ArenaAllocator = std.heap.ArenaAllocator; const Terminal = terminal.Terminal; const Health = renderer.Health; +const CFReleaseThread = @import("../cf_release_thread.zig"); + const mtl = @import("metal/api.zig"); const mtl_buffer = @import("metal/buffer.zig"); const mtl_cell = @import("metal/cell.zig"); @@ -134,6 +136,12 @@ layer: objc.Object, // CAMetalLayer /// a display link. display_link: ?DisplayLink = null, +/// Dedicated thread for releasing CoreFoundation objects some objects, +/// such as those produced by CoreText, have excessively slow release +/// callback logic. +cf_release_thread: CFReleaseThread, +cf_release_thr: std.Thread, + /// Custom shader state. This is only set if we have custom shaders. custom_shader_state: ?CustomShaderState = null, @@ -590,6 +598,9 @@ pub fn init(alloc: Allocator, options: renderer.Options) !Metal { .cursor_color = options.config.cursor_color, .current_background_color = options.config.background, + .cf_release_thread = undefined, + .cf_release_thr = undefined, + // Render state .cells = .{}, .uniforms = .{ @@ -687,10 +698,22 @@ pub fn loopEnter(self: *Metal, thr: *renderer.Thread) !void { &thr.draw_now, ); display_link.start() catch {}; + + // Create the CF release thread. + self.cf_release_thread = try CFReleaseThread.init(self.alloc); + errdefer self.cf_release_thread.deinit(); + + // Start the CF release thread. + self.cf_release_thr = try std.Thread.spawn( + .{}, + CFReleaseThread.threadMain, + .{&self.cf_release_thread}, + ); + self.cf_release_thr.setName("cf_release") catch {}; } /// Called by renderer.Thread when it exits the main loop. -pub fn loopExit(self: *const Metal) void { +pub fn loopExit(self: *Metal) void { // If we don't support a display link we have no work to do. if (comptime DisplayLink == void) return; @@ -699,6 +722,15 @@ pub fn loopExit(self: *const Metal) void { // is gone which is fine. const display_link = self.display_link orelse return; display_link.stop() catch {}; + + // Stop the CF release thread + { + self.cf_release_thread.stop.notify() catch |err| + log.err("error notifying cf release thread to stop, may stall err={}", .{err}); + self.cf_release_thr.join(); + } + + self.cf_release_thread.deinit(); } fn displayLinkCallback( @@ -986,6 +1018,9 @@ pub fn updateFrame( if (critical.preedit) |p| p.deinit(self.alloc); } + var cf_release_pool = std.ArrayList(*anyopaque).init(self.alloc); + try cf_release_pool.ensureTotalCapacity(state.terminal.rows * 8); + // Build our GPU cells try self.rebuildCells( critical.full_rebuild, @@ -994,8 +1029,29 @@ pub fn updateFrame( critical.preedit, critical.cursor_style, &critical.color_palette, + &cf_release_pool, ); + if (cf_release_pool.items.len > 0) { + const items = try cf_release_pool.toOwnedSlice(); + if (self.cf_release_thread.mailbox.push( + .{ .release = .{ + .refs = items, + .alloc = self.alloc, + } }, + .{ .forever = {} }, + ) != 0) { + try self.cf_release_thread.wakeup.notify(); + } else { + for (items) |ref| { + macos.foundation.CFRelease(ref); + } + self.alloc.free(items); + } + } else { + cf_release_pool.deinit(); + } + // Update our viewport pin self.cells_viewport = critical.viewport_pin; @@ -1875,6 +1931,7 @@ fn rebuildCells( preedit: ?renderer.State.Preedit, cursor_style_: ?renderer.CursorStyle, color_palette: *const terminal.color.Palette, + cf_release_pool: *std.ArrayList(*anyopaque), ) !void { // const start = try std.time.Instant.now(); // const start_micro = std.time.microTimestamp(); @@ -1956,7 +2013,10 @@ fn rebuildCells( while (try iter.next(self.alloc)) |run| { // Try to read the cells from the shaping cache if we can. const shaper_cells = self.font_shaper_cache.get(run) orelse cache: { - const cells = try self.font_shaper.shape(run); + const cells = if (font.options.backend == .coretext) + try self.font_shaper.shape(run, cf_release_pool) + else + try self.font_shaper.shape(run); // Try to cache them. If caching fails for any reason we continue // because it is just a performance optimization, not a correctness From 053500da38088693d108b613d6fa595aa8de57f5 Mon Sep 17 00:00:00 2001 From: Qwerasd Date: Fri, 14 Jun 2024 01:57:46 -0400 Subject: [PATCH 06/15] shaper/coretext: update tests --- src/font/shaper/coretext.zig | 218 +++++++++++++++++++++++++++++------ 1 file changed, 185 insertions(+), 33 deletions(-) diff --git a/src/font/shaper/coretext.zig b/src/font/shaper/coretext.zig index c32145019..cbac71ec3 100644 --- a/src/font/shaper/coretext.zig +++ b/src/font/shaper/coretext.zig @@ -599,6 +599,14 @@ test "run iterator: empty cells with background set" { const testing = std.testing; const alloc = testing.allocator; + var cf_release_pool = std.ArrayList(*anyopaque).init(alloc); + defer { + for (cf_release_pool.items) |ref| { + macos.foundation.CFRelease(ref); + } + cf_release_pool.deinit(); + } + var testdata = try testShaper(alloc); defer testdata.deinit(); @@ -638,7 +646,7 @@ test "run iterator: empty cells with background set" { ); { const run = (try it.next(alloc)).?; - const cells = try shaper.shape(run); + const cells = try shaper.shape(run, &cf_release_pool); try testing.expectEqual(@as(usize, 3), cells.len); } try testing.expect(try it.next(alloc) == null); @@ -649,6 +657,14 @@ test "shape" { const testing = std.testing; const alloc = testing.allocator; + var cf_release_pool = std.ArrayList(*anyopaque).init(alloc); + defer { + for (cf_release_pool.items) |ref| { + macos.foundation.CFRelease(ref); + } + cf_release_pool.deinit(); + } + var testdata = try testShaper(alloc); defer testdata.deinit(); @@ -675,7 +691,7 @@ test "shape" { var count: usize = 0; while (try it.next(alloc)) |run| { count += 1; - _ = try shaper.shape(run); + _ = try shaper.shape(run, &cf_release_pool); } try testing.expectEqual(@as(usize, 1), count); } @@ -684,6 +700,14 @@ test "shape nerd fonts" { const testing = std.testing; const alloc = testing.allocator; + var cf_release_pool = std.ArrayList(*anyopaque).init(alloc); + defer { + for (cf_release_pool.items) |ref| { + macos.foundation.CFRelease(ref); + } + cf_release_pool.deinit(); + } + var testdata = try testShaperWithFont(alloc, .nerd_font); defer testdata.deinit(); @@ -710,7 +734,7 @@ test "shape nerd fonts" { var count: usize = 0; while (try it.next(alloc)) |run| { count += 1; - _ = try shaper.shape(run); + _ = try shaper.shape(run, &cf_release_pool); } try testing.expectEqual(@as(usize, 1), count); } @@ -719,6 +743,14 @@ test "shape inconsolata ligs" { const testing = std.testing; const alloc = testing.allocator; + var cf_release_pool = std.ArrayList(*anyopaque).init(alloc); + defer { + for (cf_release_pool.items) |ref| { + macos.foundation.CFRelease(ref); + } + cf_release_pool.deinit(); + } + var testdata = try testShaper(alloc); defer testdata.deinit(); @@ -739,7 +771,7 @@ test "shape inconsolata ligs" { while (try it.next(alloc)) |run| { count += 1; - const cells = try shaper.shape(run); + const cells = try shaper.shape(run, &cf_release_pool); try testing.expectEqual(@as(usize, 2), cells.len); try testing.expect(cells[0].glyph_index != null); try testing.expect(cells[1].glyph_index == null); @@ -764,7 +796,7 @@ test "shape inconsolata ligs" { while (try it.next(alloc)) |run| { count += 1; - const cells = try shaper.shape(run); + const cells = try shaper.shape(run, &cf_release_pool); try testing.expectEqual(@as(usize, 3), cells.len); try testing.expect(cells[0].glyph_index != null); try testing.expect(cells[1].glyph_index == null); @@ -778,6 +810,14 @@ test "shape monaspace ligs" { const testing = std.testing; const alloc = testing.allocator; + var cf_release_pool = std.ArrayList(*anyopaque).init(alloc); + defer { + for (cf_release_pool.items) |ref| { + macos.foundation.CFRelease(ref); + } + cf_release_pool.deinit(); + } + var testdata = try testShaperWithFont(alloc, .monaspace_neon); defer testdata.deinit(); @@ -798,7 +838,7 @@ test "shape monaspace ligs" { while (try it.next(alloc)) |run| { count += 1; - const cells = try shaper.shape(run); + const cells = try shaper.shape(run, &cf_release_pool); try testing.expectEqual(@as(usize, 3), cells.len); try testing.expect(cells[0].glyph_index != null); try testing.expect(cells[1].glyph_index == null); @@ -813,6 +853,14 @@ test "shape left-replaced lig in last run" { const testing = std.testing; const alloc = testing.allocator; + var cf_release_pool = std.ArrayList(*anyopaque).init(alloc); + defer { + for (cf_release_pool.items) |ref| { + macos.foundation.CFRelease(ref); + } + cf_release_pool.deinit(); + } + var testdata = try testShaperWithFont(alloc, .geist_mono); defer testdata.deinit(); @@ -833,7 +881,7 @@ test "shape left-replaced lig in last run" { while (try it.next(alloc)) |run| { count += 1; - const cells = try shaper.shape(run); + const cells = try shaper.shape(run, &cf_release_pool); try testing.expectEqual(@as(usize, 3), cells.len); try testing.expect(cells[0].glyph_index != null); try testing.expect(cells[1].glyph_index == null); @@ -848,6 +896,14 @@ test "shape left-replaced lig in early run" { const testing = std.testing; const alloc = testing.allocator; + var cf_release_pool = std.ArrayList(*anyopaque).init(alloc); + defer { + for (cf_release_pool.items) |ref| { + macos.foundation.CFRelease(ref); + } + cf_release_pool.deinit(); + } + var testdata = try testShaperWithFont(alloc, .geist_mono); defer testdata.deinit(); @@ -866,7 +922,7 @@ test "shape left-replaced lig in early run" { ); const run = (try it.next(alloc)).?; - const cells = try shaper.shape(run); + const cells = try shaper.shape(run, &cf_release_pool); try testing.expectEqual(@as(usize, 4), cells.len); try testing.expect(cells[0].glyph_index != null); try testing.expect(cells[1].glyph_index == null); @@ -880,6 +936,14 @@ test "shape U+3C9 with JB Mono" { const testing = std.testing; const alloc = testing.allocator; + var cf_release_pool = std.ArrayList(*anyopaque).init(alloc); + defer { + for (cf_release_pool.items) |ref| { + macos.foundation.CFRelease(ref); + } + cf_release_pool.deinit(); + } + var testdata = try testShaperWithFont(alloc, .jetbrains_mono); defer testdata.deinit(); @@ -901,7 +965,7 @@ test "shape U+3C9 with JB Mono" { var cell_count: usize = 0; while (try it.next(alloc)) |run| { run_count += 1; - const cells = try shaper.shape(run); + const cells = try shaper.shape(run, &cf_release_pool); cell_count += cells.len; } try testing.expectEqual(@as(usize, 1), run_count); @@ -913,6 +977,14 @@ test "shape emoji width" { const testing = std.testing; const alloc = testing.allocator; + var cf_release_pool = std.ArrayList(*anyopaque).init(alloc); + defer { + for (cf_release_pool.items) |ref| { + macos.foundation.CFRelease(ref); + } + cf_release_pool.deinit(); + } + var testdata = try testShaper(alloc); defer testdata.deinit(); @@ -933,7 +1005,7 @@ test "shape emoji width" { while (try it.next(alloc)) |run| { count += 1; - const cells = try shaper.shape(run); + const cells = try shaper.shape(run, &cf_release_pool); try testing.expectEqual(@as(usize, 1), cells.len); } try testing.expectEqual(@as(usize, 1), count); @@ -944,6 +1016,14 @@ test "shape emoji width long" { const testing = std.testing; const alloc = testing.allocator; + var cf_release_pool = std.ArrayList(*anyopaque).init(alloc); + defer { + for (cf_release_pool.items) |ref| { + macos.foundation.CFRelease(ref); + } + cf_release_pool.deinit(); + } + var testdata = try testShaper(alloc); defer testdata.deinit(); @@ -972,7 +1052,7 @@ test "shape emoji width long" { var count: usize = 0; while (try it.next(alloc)) |run| { count += 1; - const cells = try shaper.shape(run); + const cells = try shaper.shape(run, &cf_release_pool); // screen.testWriteString isn't grapheme aware, otherwise this is one try testing.expectEqual(@as(usize, 5), cells.len); @@ -984,6 +1064,14 @@ test "shape variation selector VS15" { const testing = std.testing; const alloc = testing.allocator; + var cf_release_pool = std.ArrayList(*anyopaque).init(alloc); + defer { + for (cf_release_pool.items) |ref| { + macos.foundation.CFRelease(ref); + } + cf_release_pool.deinit(); + } + var testdata = try testShaper(alloc); defer testdata.deinit(); @@ -1009,7 +1097,7 @@ test "shape variation selector VS15" { var count: usize = 0; while (try it.next(alloc)) |run| { count += 1; - const cells = try shaper.shape(run); + const cells = try shaper.shape(run, &cf_release_pool); try testing.expectEqual(@as(usize, 1), cells.len); } try testing.expectEqual(@as(usize, 1), count); @@ -1019,6 +1107,14 @@ test "shape variation selector VS16" { const testing = std.testing; const alloc = testing.allocator; + var cf_release_pool = std.ArrayList(*anyopaque).init(alloc); + defer { + for (cf_release_pool.items) |ref| { + macos.foundation.CFRelease(ref); + } + cf_release_pool.deinit(); + } + var testdata = try testShaper(alloc); defer testdata.deinit(); @@ -1044,7 +1140,7 @@ test "shape variation selector VS16" { var count: usize = 0; while (try it.next(alloc)) |run| { count += 1; - const cells = try shaper.shape(run); + const cells = try shaper.shape(run, &cf_release_pool); try testing.expectEqual(@as(usize, 1), cells.len); } try testing.expectEqual(@as(usize, 1), count); @@ -1054,6 +1150,14 @@ test "shape with empty cells in between" { const testing = std.testing; const alloc = testing.allocator; + var cf_release_pool = std.ArrayList(*anyopaque).init(alloc); + defer { + for (cf_release_pool.items) |ref| { + macos.foundation.CFRelease(ref); + } + cf_release_pool.deinit(); + } + var testdata = try testShaper(alloc); defer testdata.deinit(); @@ -1077,7 +1181,7 @@ test "shape with empty cells in between" { while (try it.next(alloc)) |run| { count += 1; - const cells = try shaper.shape(run); + const cells = try shaper.shape(run, &cf_release_pool); try testing.expectEqual(@as(usize, 1), count); try testing.expectEqual(@as(usize, 7), cells.len); } @@ -1087,6 +1191,14 @@ test "shape Chinese characters" { const testing = std.testing; const alloc = testing.allocator; + var cf_release_pool = std.ArrayList(*anyopaque).init(alloc); + defer { + for (cf_release_pool.items) |ref| { + macos.foundation.CFRelease(ref); + } + cf_release_pool.deinit(); + } + var testdata = try testShaper(alloc); defer testdata.deinit(); @@ -1115,7 +1227,7 @@ test "shape Chinese characters" { while (try it.next(alloc)) |run| { count += 1; - const cells = try shaper.shape(run); + const cells = try shaper.shape(run, &cf_release_pool); try testing.expectEqual(@as(usize, 4), cells.len); try testing.expectEqual(@as(u16, 0), cells[0].x); try testing.expectEqual(@as(u16, 0), cells[1].x); @@ -1129,6 +1241,14 @@ test "shape box glyphs" { const testing = std.testing; const alloc = testing.allocator; + var cf_release_pool = std.ArrayList(*anyopaque).init(alloc); + defer { + for (cf_release_pool.items) |ref| { + macos.foundation.CFRelease(ref); + } + cf_release_pool.deinit(); + } + var testdata = try testShaper(alloc); defer testdata.deinit(); @@ -1154,7 +1274,7 @@ test "shape box glyphs" { var count: usize = 0; while (try it.next(alloc)) |run| { count += 1; - const cells = try shaper.shape(run); + const cells = try shaper.shape(run, &cf_release_pool); try testing.expectEqual(@as(usize, 2), cells.len); try testing.expectEqual(@as(u32, 0x2500), cells[0].glyph_index.?); try testing.expectEqual(@as(u16, 0), cells[0].x); @@ -1168,6 +1288,14 @@ test "shape selection boundary" { const testing = std.testing; const alloc = testing.allocator; + var cf_release_pool = std.ArrayList(*anyopaque).init(alloc); + defer { + for (cf_release_pool.items) |ref| { + macos.foundation.CFRelease(ref); + } + cf_release_pool.deinit(); + } + var testdata = try testShaper(alloc); defer testdata.deinit(); @@ -1194,7 +1322,7 @@ test "shape selection boundary" { var count: usize = 0; while (try it.next(alloc)) |run| { count += 1; - _ = try shaper.shape(run); + _ = try shaper.shape(run, &cf_release_pool); } try testing.expectEqual(@as(usize, 1), count); } @@ -1217,7 +1345,7 @@ test "shape selection boundary" { var count: usize = 0; while (try it.next(alloc)) |run| { count += 1; - _ = try shaper.shape(run); + _ = try shaper.shape(run, &cf_release_pool); } try testing.expectEqual(@as(usize, 2), count); } @@ -1240,7 +1368,7 @@ test "shape selection boundary" { var count: usize = 0; while (try it.next(alloc)) |run| { count += 1; - _ = try shaper.shape(run); + _ = try shaper.shape(run, &cf_release_pool); } try testing.expectEqual(@as(usize, 2), count); } @@ -1263,7 +1391,7 @@ test "shape selection boundary" { var count: usize = 0; while (try it.next(alloc)) |run| { count += 1; - _ = try shaper.shape(run); + _ = try shaper.shape(run, &cf_release_pool); } try testing.expectEqual(@as(usize, 3), count); } @@ -1286,7 +1414,7 @@ test "shape selection boundary" { var count: usize = 0; while (try it.next(alloc)) |run| { count += 1; - _ = try shaper.shape(run); + _ = try shaper.shape(run, &cf_release_pool); } try testing.expectEqual(@as(usize, 3), count); } @@ -1296,6 +1424,14 @@ test "shape cursor boundary" { const testing = std.testing; const alloc = testing.allocator; + var cf_release_pool = std.ArrayList(*anyopaque).init(alloc); + defer { + for (cf_release_pool.items) |ref| { + macos.foundation.CFRelease(ref); + } + cf_release_pool.deinit(); + } + var testdata = try testShaper(alloc); defer testdata.deinit(); @@ -1318,7 +1454,7 @@ test "shape cursor boundary" { var count: usize = 0; while (try it.next(alloc)) |run| { count += 1; - _ = try shaper.shape(run); + _ = try shaper.shape(run, &cf_release_pool); } try testing.expectEqual(@as(usize, 1), count); } @@ -1337,7 +1473,7 @@ test "shape cursor boundary" { var count: usize = 0; while (try it.next(alloc)) |run| { count += 1; - _ = try shaper.shape(run); + _ = try shaper.shape(run, &cf_release_pool); } try testing.expectEqual(@as(usize, 2), count); } @@ -1356,7 +1492,7 @@ test "shape cursor boundary" { var count: usize = 0; while (try it.next(alloc)) |run| { count += 1; - _ = try shaper.shape(run); + _ = try shaper.shape(run, &cf_release_pool); } try testing.expectEqual(@as(usize, 3), count); } @@ -1375,7 +1511,7 @@ test "shape cursor boundary" { var count: usize = 0; while (try it.next(alloc)) |run| { count += 1; - _ = try shaper.shape(run); + _ = try shaper.shape(run, &cf_release_pool); } try testing.expectEqual(@as(usize, 2), count); } @@ -1385,6 +1521,14 @@ test "shape cursor boundary and colored emoji" { const testing = std.testing; const alloc = testing.allocator; + var cf_release_pool = std.ArrayList(*anyopaque).init(alloc); + defer { + for (cf_release_pool.items) |ref| { + macos.foundation.CFRelease(ref); + } + cf_release_pool.deinit(); + } + var testdata = try testShaper(alloc); defer testdata.deinit(); @@ -1407,7 +1551,7 @@ test "shape cursor boundary and colored emoji" { var count: usize = 0; while (try it.next(alloc)) |run| { count += 1; - _ = try shaper.shape(run); + _ = try shaper.shape(run, &cf_release_pool); } try testing.expectEqual(@as(usize, 1), count); } @@ -1426,7 +1570,7 @@ test "shape cursor boundary and colored emoji" { var count: usize = 0; while (try it.next(alloc)) |run| { count += 1; - _ = try shaper.shape(run); + _ = try shaper.shape(run, &cf_release_pool); } try testing.expectEqual(@as(usize, 1), count); } @@ -1443,7 +1587,7 @@ test "shape cursor boundary and colored emoji" { var count: usize = 0; while (try it.next(alloc)) |run| { count += 1; - _ = try shaper.shape(run); + _ = try shaper.shape(run, &cf_release_pool); } try testing.expectEqual(@as(usize, 1), count); } @@ -1453,6 +1597,14 @@ test "shape cell attribute change" { const testing = std.testing; const alloc = testing.allocator; + var cf_release_pool = std.ArrayList(*anyopaque).init(alloc); + defer { + for (cf_release_pool.items) |ref| { + macos.foundation.CFRelease(ref); + } + cf_release_pool.deinit(); + } + var testdata = try testShaper(alloc); defer testdata.deinit(); @@ -1473,7 +1625,7 @@ test "shape cell attribute change" { var count: usize = 0; while (try it.next(alloc)) |run| { count += 1; - _ = try shaper.shape(run); + _ = try shaper.shape(run, &cf_release_pool); } try testing.expectEqual(@as(usize, 1), count); } @@ -1497,7 +1649,7 @@ test "shape cell attribute change" { var count: usize = 0; while (try it.next(alloc)) |run| { count += 1; - _ = try shaper.shape(run); + _ = try shaper.shape(run, &cf_release_pool); } try testing.expectEqual(@as(usize, 2), count); } @@ -1522,7 +1674,7 @@ test "shape cell attribute change" { var count: usize = 0; while (try it.next(alloc)) |run| { count += 1; - _ = try shaper.shape(run); + _ = try shaper.shape(run, &cf_release_pool); } try testing.expectEqual(@as(usize, 2), count); } @@ -1547,7 +1699,7 @@ test "shape cell attribute change" { var count: usize = 0; while (try it.next(alloc)) |run| { count += 1; - _ = try shaper.shape(run); + _ = try shaper.shape(run, &cf_release_pool); } try testing.expectEqual(@as(usize, 1), count); } @@ -1571,7 +1723,7 @@ test "shape cell attribute change" { var count: usize = 0; while (try it.next(alloc)) |run| { count += 1; - _ = try shaper.shape(run); + _ = try shaper.shape(run, &cf_release_pool); } try testing.expectEqual(@as(usize, 1), count); } From 3f3db4896ba1360522ade4c859a586a9d7d7752f Mon Sep 17 00:00:00 2001 From: Qwerasd Date: Fri, 14 Jun 2024 02:07:17 -0400 Subject: [PATCH 07/15] add CFReleasePool handling to OpenGL renderer --- src/renderer/OpenGL.zig | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/renderer/OpenGL.zig b/src/renderer/OpenGL.zig index 8389dbae6..5dd8d9345 100644 --- a/src/renderer/OpenGL.zig +++ b/src/renderer/OpenGL.zig @@ -1025,7 +1025,16 @@ pub fn rebuildCells( while (try iter.next(self.alloc)) |run| { // Try to read the cells from the shaping cache if we can. const shaper_cells = self.font_shaper_cache.get(run) orelse cache: { - const cells = try self.font_shaper.shape(run); + const cells = if (font.options.backend == .coretext) ct: { + var cf_release_pool = std.ArrayList(*anyopaque).init(self.alloc); + defer { + for (cf_release_pool.items) |ref| { + @import("macos").foundation.CFRelease(ref); + } + cf_release_pool.deinit(); + } + break :ct try self.font_shaper.shape(run, &cf_release_pool); + } else try self.font_shaper.shape(run); // Try to cache them. If caching fails for any reason we continue // because it is just a performance optimization, not a correctness From 4aa130b0d1ec70ed1b13c1d951d0a8fde94a128c Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sat, 22 Jun 2024 20:08:02 -0700 Subject: [PATCH 08/15] CacheTable tests, style changes --- src/cache_table.zig | 70 ++++++++++++++++++++++++++++++++++++-------- src/main_ghostty.zig | 1 + 2 files changed, 59 insertions(+), 12 deletions(-) diff --git a/src/cache_table.zig b/src/cache_table.zig index bdc99cd14..2e2a290d5 100644 --- a/src/cache_table.zig +++ b/src/cache_table.zig @@ -29,9 +29,9 @@ const assert = std.debug.assert; /// /// `bucket_count` /// Should ideally be close to the median number of important items that -/// you expect to be cached at any given point. -/// -/// Performance will suffer if this is not a power of 2. +/// you expect to be cached at any given point. This is required to be a +/// power of 2 since performance suffers if it's not and there's no good +/// reason to allow it to be anything else. /// /// `bucket_size` /// should be larger if you expect a large number of unimportant items to @@ -53,6 +53,10 @@ pub fn CacheTable( value: V, }; + comptime { + assert(std.math.isPowerOfTwo(bucket_count)); + } + /// `bucket_count` buckets containing `bucket_size` KV pairs each. /// /// We don't need to initialize this memory because we don't use it @@ -74,26 +78,25 @@ pub fn CacheTable( /// Adds an item to the cache table. If an old value was removed to /// make room then it is returned in a struct with its key and value. pub fn put(self: *Self, key: K, value: V) ?KV { + const kv: KV = .{ .key = key, .value = value }; const idx: u64 = self.context.hash(key) % bucket_count; - const kv = .{ - .key = key, - .value = value, - }; - + // If we have space available in the bucket then we just append if (self.lengths[idx] < bucket_size) { self.buckets[idx][self.lengths[idx]] = kv; self.lengths[idx] += 1; return null; } - assert(self.lengths[idx] == bucket_size); + // Append our new item and return the oldest const evicted = fastmem.rotateIn(KV, &self.buckets[idx], kv); - if (comptime @hasDecl(Context, "evicted")) { - self.context.evicted(evicted.key, evicted.value); - } + // The Context is allowed to register an eviction hook. + if (comptime @hasDecl(Context, "evicted")) self.context.evicted( + evicted.key, + evicted.value, + ); return evicted; } @@ -129,7 +132,50 @@ pub fn CacheTable( } } } + @memset(&self.lengths, 0); } }; } + +/// Creates a Context automatically for the given key type. This uses the +/// same logic as std.hash_map.AutoContext today since the API matches. +fn AutoContext(comptime K: type) type { + return std.hash_map.AutoContext(K); +} + +test CacheTable { + const testing = std.testing; + + // Construct a table that purposely has a predictable hash so we can + // test all edge cases. + const T = CacheTable(u32, u32, struct { + pub fn hash(self: *const @This(), key: u32) u64 { + _ = self; + return @intCast(key); + } + + pub fn eql(self: *const @This(), a: u32, b: u32) bool { + _ = self; + return a == b; + } + }, 2, 2); + var t: T = .{ .context = .{} }; + + // Fill the table + try testing.expect(t.put(0, 0) == null); + try testing.expect(t.put(1, 0) == null); + try testing.expect(t.put(2, 0) == null); + try testing.expect(t.put(3, 0) == null); + + // It should now be full, so any insert should evict the oldest item. + // NOTE: For the sake of this test, we're assuming that the first item + // is evicted but we don't need to promise this. + try testing.expectEqual(T.KV{ + .key = 0, + .value = 0, + }, t.put(4, 0).?); + + // The first item should now be gone + try testing.expect(t.get(0) == null); +} diff --git a/src/main_ghostty.zig b/src/main_ghostty.zig index ff38b800d..afc0ae791 100644 --- a/src/main_ghostty.zig +++ b/src/main_ghostty.zig @@ -319,6 +319,7 @@ test { // TODO _ = @import("blocking_queue.zig"); + _ = @import("cache_table.zig"); _ = @import("config.zig"); _ = @import("lru.zig"); } From 3038cb4979e4b8d5c3ab434af5d7d0f3b41c4ec9 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sat, 22 Jun 2024 20:15:59 -0700 Subject: [PATCH 09/15] Move CFReleaseThread to os package --- src/{ => os}/cf_release_thread.zig | 14 ++++++-------- src/os/main.zig | 4 +++- src/renderer/Metal.zig | 4 ++-- 3 files changed, 11 insertions(+), 11 deletions(-) rename src/{ => os}/cf_release_thread.zig (94%) diff --git a/src/cf_release_thread.zig b/src/os/cf_release_thread.zig similarity index 94% rename from src/cf_release_thread.zig rename to src/os/cf_release_thread.zig index 6a37ca692..32069163b 100644 --- a/src/cf_release_thread.zig +++ b/src/os/cf_release_thread.zig @@ -9,15 +9,15 @@ const builtin = @import("builtin"); const xev = @import("xev"); const macos = @import("macos"); -const BlockingQueue = @import("./blocking_queue.zig").BlockingQueue; +const BlockingQueue = @import("../blocking_queue.zig").BlockingQueue; const Allocator = std.mem.Allocator; const log = std.log.scoped(.cf_release_thread); pub const Message = union(enum) { - /// Release a slice of CFTypeRefs. Uses alloc to - /// free the slice after releasing all the refs. - release: struct{ + /// Release a slice of CFTypeRefs. Uses alloc to free the slice after + /// releasing all the refs. + release: struct { refs: []*anyopaque, alloc: Allocator, }, @@ -142,12 +142,10 @@ fn drainMailbox(self: *Thread) !void { // log.debug("mailbox message={}", .{message}); switch (message) { .release => |msg| { - for (msg.refs) |ref| { - macos.foundation.CFRelease(ref); - } + for (msg.refs) |ref| macos.foundation.CFRelease(ref); // log.debug("Released {} CFTypeRefs.", .{ msg.refs.len }); msg.alloc.free(msg.refs); - } + }, } } } diff --git a/src/os/main.zig b/src/os/main.zig index cee592027..04d53d752 100644 --- a/src/os/main.zig +++ b/src/os/main.zig @@ -1,5 +1,6 @@ //! The "os" package contains utilities for interfacing with the operating -//! system. +//! system. These aren't restricted to syscalls or low-level operations, but +//! also OS-specific features and conventions. pub usingnamespace @import("env.zig"); pub usingnamespace @import("desktop.zig"); @@ -12,6 +13,7 @@ pub usingnamespace @import("mouse.zig"); pub usingnamespace @import("open.zig"); pub usingnamespace @import("pipe.zig"); pub usingnamespace @import("resourcesdir.zig"); +pub const CFReleaseThread = @import("cf_release_thread.zig"); pub const TempDir = @import("TempDir.zig"); pub const cgroup = @import("cgroup.zig"); pub const passwd = @import("passwd.zig"); diff --git a/src/renderer/Metal.zig b/src/renderer/Metal.zig index 5b85d9a4e..f4813fc17 100644 --- a/src/renderer/Metal.zig +++ b/src/renderer/Metal.zig @@ -15,6 +15,7 @@ const xev = @import("xev"); const apprt = @import("../apprt.zig"); const configpkg = @import("../config.zig"); const font = @import("../font/main.zig"); +const os = @import("../os/main.zig"); const terminal = @import("../terminal/main.zig"); const renderer = @import("../renderer.zig"); const math = @import("../math.zig"); @@ -25,11 +26,10 @@ const shadertoy = @import("shadertoy.zig"); const assert = std.debug.assert; const Allocator = std.mem.Allocator; const ArenaAllocator = std.heap.ArenaAllocator; +const CFReleaseThread = os.CFReleaseThread; const Terminal = terminal.Terminal; const Health = renderer.Health; -const CFReleaseThread = @import("../cf_release_thread.zig"); - const mtl = @import("metal/api.zig"); const mtl_buffer = @import("metal/buffer.zig"); const mtl_cell = @import("metal/cell.zig"); From 4325dc51bc47eea67238bcbcd8f556dab9562342 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sat, 22 Jun 2024 20:32:24 -0700 Subject: [PATCH 10/15] font: coretext shaper owns the cf release pool --- src/font/shaper/coretext.zig | 276 +++++++++-------------------------- src/renderer/Metal.zig | 25 +--- src/renderer/OpenGL.zig | 11 +- 3 files changed, 79 insertions(+), 233 deletions(-) diff --git a/src/font/shaper/coretext.zig b/src/font/shaper/coretext.zig index cbac71ec3..5da08b214 100644 --- a/src/font/shaper/coretext.zig +++ b/src/font/shaper/coretext.zig @@ -1,4 +1,5 @@ const std = @import("std"); +const builtin = @import("builtin"); const assert = std.debug.assert; const Allocator = std.mem.Allocator; const macos = @import("macos"); @@ -49,13 +50,18 @@ pub const Shaper = struct { /// and releasing many objects when shaping. writing_direction: *macos.foundation.Array, - /// List where we cache fonts, so we don't have to - /// remake them for every single shaping operation. + /// List where we cache fonts, so we don't have to remake them for + /// every single shaping operation. /// - /// Fonts are cached as attribute dictionaries to - /// be applied directly to attributed strings. + /// Fonts are cached as attribute dictionaries to be applied directly to + /// attributed strings. cached_fonts: std.ArrayList(?*macos.foundation.Dictionary), + /// The list of CoreFoundation objects to release on the dedicated + /// release thread. This is built up over the course of shaping and + /// sent to the release thread when endFrame is called. + cf_release_pool: std.ArrayListUnmanaged(*anyopaque), + const CellBuf = std.ArrayListUnmanaged(font.shape.Cell); const CodepointList = std.ArrayListUnmanaged(Codepoint); const Codepoint = struct { @@ -209,13 +215,14 @@ pub const Shaper = struct { const cached_fonts = std.ArrayList(?*macos.foundation.Dictionary).init(alloc); errdefer cached_fonts.deinit(); - return Shaper{ + return .{ .alloc = alloc, .cell_buf = .{}, .run_state = run_state, .features = feats, .writing_direction = writing_direction, .cached_fonts = cached_fonts, + .cf_release_pool = .{}, }; } @@ -227,6 +234,19 @@ pub const Shaper = struct { self.releaseCachedFonts(); self.cached_fonts.deinit(); + + if (self.cf_release_pool.items.len > 0) { + for (self.cf_release_pool.items) |ref| macos.foundation.CFRelease(ref); + + // For tests this logic is normal because we don't want to + // wait for a release thread. But in production this is a bug + // and we should warn. + if (comptime !builtin.is_test) log.warn( + "BUG: CFRelease pool was not empty, releasing remaining objects", + .{}, + ); + } + self.cf_release_pool.deinit(self.alloc); } /// Release all cached fonts. @@ -256,13 +276,12 @@ pub const Shaper = struct { }; } - /// Expects an ArrayList `cf_release_pool` in which `CFTypeRef`s - /// can be placed, which guarantees that they will be `CFRelease`d - /// eventually. + /// Note that this will accumulate garbage in the release pool. The + /// caller must ensure you're properly calling endFrame to release + /// all the objects. pub fn shape( self: *Shaper, run: font.shape.TextRun, - cf_release_pool: *std.ArrayList(*anyopaque), ) ![]const font.shape.Cell { const state = &self.run_state; @@ -297,25 +316,24 @@ pub const Shaper = struct { const attr_dict: *macos.foundation.Dictionary = try self.getFont( run.grid, run.font_index, - cf_release_pool, ); // Make room for the attributed string and the CTLine. - try cf_release_pool.ensureUnusedCapacity(3); + try self.cf_release_pool.ensureUnusedCapacity(self.alloc, 3); const str = macos.foundation.String.createWithCharactersNoCopy(state.unichars.items); - cf_release_pool.appendAssumeCapacity(str); + self.cf_release_pool.appendAssumeCapacity(str); // Create an attributed string from our string const attr_str = try macos.foundation.AttributedString.create( str, attr_dict, ); - cf_release_pool.appendAssumeCapacity(attr_str); + self.cf_release_pool.appendAssumeCapacity(attr_str); // We should always have one run because we do our own run splitting. const line = try macos.text.Line.createWithAttributedString(attr_str); - cf_release_pool.appendAssumeCapacity(line); + self.cf_release_pool.appendAssumeCapacity(line); // This keeps track of the current offsets within a single cell. var cell_offset: struct { @@ -411,7 +429,6 @@ pub const Shaper = struct { self: *Shaper, grid: *font.SharedGrid, index: font.Collection.Index, - cf_release_pool: *std.ArrayList(*anyopaque), ) !*macos.foundation.Dictionary { const index_int = index.int(); @@ -426,7 +443,8 @@ pub const Shaper = struct { return cached; } - try cf_release_pool.ensureUnusedCapacity(3); + // Features dictionary, font descriptor, font + try self.cf_release_pool.ensureUnusedCapacity(self.alloc, 3); const run_font = font: { // The CoreText shaper relies on CoreText and CoreText claims @@ -450,17 +468,17 @@ pub const Shaper = struct { const original = face.font; const attrs = try self.features.attrsDict(face.quirks_disable_default_font_features); - cf_release_pool.appendAssumeCapacity(attrs); + self.cf_release_pool.appendAssumeCapacity(attrs); const desc = try macos.text.FontDescriptor.createWithAttributes(attrs); - cf_release_pool.appendAssumeCapacity(desc); + self.cf_release_pool.appendAssumeCapacity(desc); const copied = try original.copyWithAttributes(0, null, desc); errdefer copied.release(); break :font copied; }; - cf_release_pool.appendAssumeCapacity(run_font); + self.cf_release_pool.appendAssumeCapacity(run_font); // Get our font and use that get the attributes to set for the // attributed string so the whole string uses the same font. @@ -500,7 +518,7 @@ pub const Shaper = struct { const pair = macos.foundation.stringGetSurrogatePairForLongCharacter( cp, - state.unichars.items[state.unichars.items.len-2..][0..2], + state.unichars.items[state.unichars.items.len - 2 ..][0..2], ); if (!pair) { state.unichars.items.len -= 1; @@ -599,14 +617,6 @@ test "run iterator: empty cells with background set" { const testing = std.testing; const alloc = testing.allocator; - var cf_release_pool = std.ArrayList(*anyopaque).init(alloc); - defer { - for (cf_release_pool.items) |ref| { - macos.foundation.CFRelease(ref); - } - cf_release_pool.deinit(); - } - var testdata = try testShaper(alloc); defer testdata.deinit(); @@ -646,7 +656,7 @@ test "run iterator: empty cells with background set" { ); { const run = (try it.next(alloc)).?; - const cells = try shaper.shape(run, &cf_release_pool); + const cells = try shaper.shape(run); try testing.expectEqual(@as(usize, 3), cells.len); } try testing.expect(try it.next(alloc) == null); @@ -657,14 +667,6 @@ test "shape" { const testing = std.testing; const alloc = testing.allocator; - var cf_release_pool = std.ArrayList(*anyopaque).init(alloc); - defer { - for (cf_release_pool.items) |ref| { - macos.foundation.CFRelease(ref); - } - cf_release_pool.deinit(); - } - var testdata = try testShaper(alloc); defer testdata.deinit(); @@ -691,7 +693,7 @@ test "shape" { var count: usize = 0; while (try it.next(alloc)) |run| { count += 1; - _ = try shaper.shape(run, &cf_release_pool); + _ = try shaper.shape(run); } try testing.expectEqual(@as(usize, 1), count); } @@ -700,14 +702,6 @@ test "shape nerd fonts" { const testing = std.testing; const alloc = testing.allocator; - var cf_release_pool = std.ArrayList(*anyopaque).init(alloc); - defer { - for (cf_release_pool.items) |ref| { - macos.foundation.CFRelease(ref); - } - cf_release_pool.deinit(); - } - var testdata = try testShaperWithFont(alloc, .nerd_font); defer testdata.deinit(); @@ -734,7 +728,7 @@ test "shape nerd fonts" { var count: usize = 0; while (try it.next(alloc)) |run| { count += 1; - _ = try shaper.shape(run, &cf_release_pool); + _ = try shaper.shape(run); } try testing.expectEqual(@as(usize, 1), count); } @@ -743,14 +737,6 @@ test "shape inconsolata ligs" { const testing = std.testing; const alloc = testing.allocator; - var cf_release_pool = std.ArrayList(*anyopaque).init(alloc); - defer { - for (cf_release_pool.items) |ref| { - macos.foundation.CFRelease(ref); - } - cf_release_pool.deinit(); - } - var testdata = try testShaper(alloc); defer testdata.deinit(); @@ -771,7 +757,7 @@ test "shape inconsolata ligs" { while (try it.next(alloc)) |run| { count += 1; - const cells = try shaper.shape(run, &cf_release_pool); + const cells = try shaper.shape(run); try testing.expectEqual(@as(usize, 2), cells.len); try testing.expect(cells[0].glyph_index != null); try testing.expect(cells[1].glyph_index == null); @@ -796,7 +782,7 @@ test "shape inconsolata ligs" { while (try it.next(alloc)) |run| { count += 1; - const cells = try shaper.shape(run, &cf_release_pool); + const cells = try shaper.shape(run); try testing.expectEqual(@as(usize, 3), cells.len); try testing.expect(cells[0].glyph_index != null); try testing.expect(cells[1].glyph_index == null); @@ -810,14 +796,6 @@ test "shape monaspace ligs" { const testing = std.testing; const alloc = testing.allocator; - var cf_release_pool = std.ArrayList(*anyopaque).init(alloc); - defer { - for (cf_release_pool.items) |ref| { - macos.foundation.CFRelease(ref); - } - cf_release_pool.deinit(); - } - var testdata = try testShaperWithFont(alloc, .monaspace_neon); defer testdata.deinit(); @@ -838,7 +816,7 @@ test "shape monaspace ligs" { while (try it.next(alloc)) |run| { count += 1; - const cells = try shaper.shape(run, &cf_release_pool); + const cells = try shaper.shape(run); try testing.expectEqual(@as(usize, 3), cells.len); try testing.expect(cells[0].glyph_index != null); try testing.expect(cells[1].glyph_index == null); @@ -853,14 +831,6 @@ test "shape left-replaced lig in last run" { const testing = std.testing; const alloc = testing.allocator; - var cf_release_pool = std.ArrayList(*anyopaque).init(alloc); - defer { - for (cf_release_pool.items) |ref| { - macos.foundation.CFRelease(ref); - } - cf_release_pool.deinit(); - } - var testdata = try testShaperWithFont(alloc, .geist_mono); defer testdata.deinit(); @@ -881,7 +851,7 @@ test "shape left-replaced lig in last run" { while (try it.next(alloc)) |run| { count += 1; - const cells = try shaper.shape(run, &cf_release_pool); + const cells = try shaper.shape(run); try testing.expectEqual(@as(usize, 3), cells.len); try testing.expect(cells[0].glyph_index != null); try testing.expect(cells[1].glyph_index == null); @@ -896,14 +866,6 @@ test "shape left-replaced lig in early run" { const testing = std.testing; const alloc = testing.allocator; - var cf_release_pool = std.ArrayList(*anyopaque).init(alloc); - defer { - for (cf_release_pool.items) |ref| { - macos.foundation.CFRelease(ref); - } - cf_release_pool.deinit(); - } - var testdata = try testShaperWithFont(alloc, .geist_mono); defer testdata.deinit(); @@ -922,7 +884,7 @@ test "shape left-replaced lig in early run" { ); const run = (try it.next(alloc)).?; - const cells = try shaper.shape(run, &cf_release_pool); + const cells = try shaper.shape(run); try testing.expectEqual(@as(usize, 4), cells.len); try testing.expect(cells[0].glyph_index != null); try testing.expect(cells[1].glyph_index == null); @@ -936,14 +898,6 @@ test "shape U+3C9 with JB Mono" { const testing = std.testing; const alloc = testing.allocator; - var cf_release_pool = std.ArrayList(*anyopaque).init(alloc); - defer { - for (cf_release_pool.items) |ref| { - macos.foundation.CFRelease(ref); - } - cf_release_pool.deinit(); - } - var testdata = try testShaperWithFont(alloc, .jetbrains_mono); defer testdata.deinit(); @@ -965,7 +919,7 @@ test "shape U+3C9 with JB Mono" { var cell_count: usize = 0; while (try it.next(alloc)) |run| { run_count += 1; - const cells = try shaper.shape(run, &cf_release_pool); + const cells = try shaper.shape(run); cell_count += cells.len; } try testing.expectEqual(@as(usize, 1), run_count); @@ -977,14 +931,6 @@ test "shape emoji width" { const testing = std.testing; const alloc = testing.allocator; - var cf_release_pool = std.ArrayList(*anyopaque).init(alloc); - defer { - for (cf_release_pool.items) |ref| { - macos.foundation.CFRelease(ref); - } - cf_release_pool.deinit(); - } - var testdata = try testShaper(alloc); defer testdata.deinit(); @@ -1005,7 +951,7 @@ test "shape emoji width" { while (try it.next(alloc)) |run| { count += 1; - const cells = try shaper.shape(run, &cf_release_pool); + const cells = try shaper.shape(run); try testing.expectEqual(@as(usize, 1), cells.len); } try testing.expectEqual(@as(usize, 1), count); @@ -1016,14 +962,6 @@ test "shape emoji width long" { const testing = std.testing; const alloc = testing.allocator; - var cf_release_pool = std.ArrayList(*anyopaque).init(alloc); - defer { - for (cf_release_pool.items) |ref| { - macos.foundation.CFRelease(ref); - } - cf_release_pool.deinit(); - } - var testdata = try testShaper(alloc); defer testdata.deinit(); @@ -1052,7 +990,7 @@ test "shape emoji width long" { var count: usize = 0; while (try it.next(alloc)) |run| { count += 1; - const cells = try shaper.shape(run, &cf_release_pool); + const cells = try shaper.shape(run); // screen.testWriteString isn't grapheme aware, otherwise this is one try testing.expectEqual(@as(usize, 5), cells.len); @@ -1064,14 +1002,6 @@ test "shape variation selector VS15" { const testing = std.testing; const alloc = testing.allocator; - var cf_release_pool = std.ArrayList(*anyopaque).init(alloc); - defer { - for (cf_release_pool.items) |ref| { - macos.foundation.CFRelease(ref); - } - cf_release_pool.deinit(); - } - var testdata = try testShaper(alloc); defer testdata.deinit(); @@ -1097,7 +1027,7 @@ test "shape variation selector VS15" { var count: usize = 0; while (try it.next(alloc)) |run| { count += 1; - const cells = try shaper.shape(run, &cf_release_pool); + const cells = try shaper.shape(run); try testing.expectEqual(@as(usize, 1), cells.len); } try testing.expectEqual(@as(usize, 1), count); @@ -1107,14 +1037,6 @@ test "shape variation selector VS16" { const testing = std.testing; const alloc = testing.allocator; - var cf_release_pool = std.ArrayList(*anyopaque).init(alloc); - defer { - for (cf_release_pool.items) |ref| { - macos.foundation.CFRelease(ref); - } - cf_release_pool.deinit(); - } - var testdata = try testShaper(alloc); defer testdata.deinit(); @@ -1140,7 +1062,7 @@ test "shape variation selector VS16" { var count: usize = 0; while (try it.next(alloc)) |run| { count += 1; - const cells = try shaper.shape(run, &cf_release_pool); + const cells = try shaper.shape(run); try testing.expectEqual(@as(usize, 1), cells.len); } try testing.expectEqual(@as(usize, 1), count); @@ -1150,14 +1072,6 @@ test "shape with empty cells in between" { const testing = std.testing; const alloc = testing.allocator; - var cf_release_pool = std.ArrayList(*anyopaque).init(alloc); - defer { - for (cf_release_pool.items) |ref| { - macos.foundation.CFRelease(ref); - } - cf_release_pool.deinit(); - } - var testdata = try testShaper(alloc); defer testdata.deinit(); @@ -1181,7 +1095,7 @@ test "shape with empty cells in between" { while (try it.next(alloc)) |run| { count += 1; - const cells = try shaper.shape(run, &cf_release_pool); + const cells = try shaper.shape(run); try testing.expectEqual(@as(usize, 1), count); try testing.expectEqual(@as(usize, 7), cells.len); } @@ -1191,14 +1105,6 @@ test "shape Chinese characters" { const testing = std.testing; const alloc = testing.allocator; - var cf_release_pool = std.ArrayList(*anyopaque).init(alloc); - defer { - for (cf_release_pool.items) |ref| { - macos.foundation.CFRelease(ref); - } - cf_release_pool.deinit(); - } - var testdata = try testShaper(alloc); defer testdata.deinit(); @@ -1227,7 +1133,7 @@ test "shape Chinese characters" { while (try it.next(alloc)) |run| { count += 1; - const cells = try shaper.shape(run, &cf_release_pool); + const cells = try shaper.shape(run); try testing.expectEqual(@as(usize, 4), cells.len); try testing.expectEqual(@as(u16, 0), cells[0].x); try testing.expectEqual(@as(u16, 0), cells[1].x); @@ -1241,14 +1147,6 @@ test "shape box glyphs" { const testing = std.testing; const alloc = testing.allocator; - var cf_release_pool = std.ArrayList(*anyopaque).init(alloc); - defer { - for (cf_release_pool.items) |ref| { - macos.foundation.CFRelease(ref); - } - cf_release_pool.deinit(); - } - var testdata = try testShaper(alloc); defer testdata.deinit(); @@ -1274,7 +1172,7 @@ test "shape box glyphs" { var count: usize = 0; while (try it.next(alloc)) |run| { count += 1; - const cells = try shaper.shape(run, &cf_release_pool); + const cells = try shaper.shape(run); try testing.expectEqual(@as(usize, 2), cells.len); try testing.expectEqual(@as(u32, 0x2500), cells[0].glyph_index.?); try testing.expectEqual(@as(u16, 0), cells[0].x); @@ -1288,14 +1186,6 @@ test "shape selection boundary" { const testing = std.testing; const alloc = testing.allocator; - var cf_release_pool = std.ArrayList(*anyopaque).init(alloc); - defer { - for (cf_release_pool.items) |ref| { - macos.foundation.CFRelease(ref); - } - cf_release_pool.deinit(); - } - var testdata = try testShaper(alloc); defer testdata.deinit(); @@ -1322,7 +1212,7 @@ test "shape selection boundary" { var count: usize = 0; while (try it.next(alloc)) |run| { count += 1; - _ = try shaper.shape(run, &cf_release_pool); + _ = try shaper.shape(run); } try testing.expectEqual(@as(usize, 1), count); } @@ -1345,7 +1235,7 @@ test "shape selection boundary" { var count: usize = 0; while (try it.next(alloc)) |run| { count += 1; - _ = try shaper.shape(run, &cf_release_pool); + _ = try shaper.shape(run); } try testing.expectEqual(@as(usize, 2), count); } @@ -1368,7 +1258,7 @@ test "shape selection boundary" { var count: usize = 0; while (try it.next(alloc)) |run| { count += 1; - _ = try shaper.shape(run, &cf_release_pool); + _ = try shaper.shape(run); } try testing.expectEqual(@as(usize, 2), count); } @@ -1391,7 +1281,7 @@ test "shape selection boundary" { var count: usize = 0; while (try it.next(alloc)) |run| { count += 1; - _ = try shaper.shape(run, &cf_release_pool); + _ = try shaper.shape(run); } try testing.expectEqual(@as(usize, 3), count); } @@ -1414,7 +1304,7 @@ test "shape selection boundary" { var count: usize = 0; while (try it.next(alloc)) |run| { count += 1; - _ = try shaper.shape(run, &cf_release_pool); + _ = try shaper.shape(run); } try testing.expectEqual(@as(usize, 3), count); } @@ -1424,14 +1314,6 @@ test "shape cursor boundary" { const testing = std.testing; const alloc = testing.allocator; - var cf_release_pool = std.ArrayList(*anyopaque).init(alloc); - defer { - for (cf_release_pool.items) |ref| { - macos.foundation.CFRelease(ref); - } - cf_release_pool.deinit(); - } - var testdata = try testShaper(alloc); defer testdata.deinit(); @@ -1454,7 +1336,7 @@ test "shape cursor boundary" { var count: usize = 0; while (try it.next(alloc)) |run| { count += 1; - _ = try shaper.shape(run, &cf_release_pool); + _ = try shaper.shape(run); } try testing.expectEqual(@as(usize, 1), count); } @@ -1473,7 +1355,7 @@ test "shape cursor boundary" { var count: usize = 0; while (try it.next(alloc)) |run| { count += 1; - _ = try shaper.shape(run, &cf_release_pool); + _ = try shaper.shape(run); } try testing.expectEqual(@as(usize, 2), count); } @@ -1492,7 +1374,7 @@ test "shape cursor boundary" { var count: usize = 0; while (try it.next(alloc)) |run| { count += 1; - _ = try shaper.shape(run, &cf_release_pool); + _ = try shaper.shape(run); } try testing.expectEqual(@as(usize, 3), count); } @@ -1511,7 +1393,7 @@ test "shape cursor boundary" { var count: usize = 0; while (try it.next(alloc)) |run| { count += 1; - _ = try shaper.shape(run, &cf_release_pool); + _ = try shaper.shape(run); } try testing.expectEqual(@as(usize, 2), count); } @@ -1521,14 +1403,6 @@ test "shape cursor boundary and colored emoji" { const testing = std.testing; const alloc = testing.allocator; - var cf_release_pool = std.ArrayList(*anyopaque).init(alloc); - defer { - for (cf_release_pool.items) |ref| { - macos.foundation.CFRelease(ref); - } - cf_release_pool.deinit(); - } - var testdata = try testShaper(alloc); defer testdata.deinit(); @@ -1551,7 +1425,7 @@ test "shape cursor boundary and colored emoji" { var count: usize = 0; while (try it.next(alloc)) |run| { count += 1; - _ = try shaper.shape(run, &cf_release_pool); + _ = try shaper.shape(run); } try testing.expectEqual(@as(usize, 1), count); } @@ -1570,7 +1444,7 @@ test "shape cursor boundary and colored emoji" { var count: usize = 0; while (try it.next(alloc)) |run| { count += 1; - _ = try shaper.shape(run, &cf_release_pool); + _ = try shaper.shape(run); } try testing.expectEqual(@as(usize, 1), count); } @@ -1587,7 +1461,7 @@ test "shape cursor boundary and colored emoji" { var count: usize = 0; while (try it.next(alloc)) |run| { count += 1; - _ = try shaper.shape(run, &cf_release_pool); + _ = try shaper.shape(run); } try testing.expectEqual(@as(usize, 1), count); } @@ -1597,14 +1471,6 @@ test "shape cell attribute change" { const testing = std.testing; const alloc = testing.allocator; - var cf_release_pool = std.ArrayList(*anyopaque).init(alloc); - defer { - for (cf_release_pool.items) |ref| { - macos.foundation.CFRelease(ref); - } - cf_release_pool.deinit(); - } - var testdata = try testShaper(alloc); defer testdata.deinit(); @@ -1625,7 +1491,7 @@ test "shape cell attribute change" { var count: usize = 0; while (try it.next(alloc)) |run| { count += 1; - _ = try shaper.shape(run, &cf_release_pool); + _ = try shaper.shape(run); } try testing.expectEqual(@as(usize, 1), count); } @@ -1649,7 +1515,7 @@ test "shape cell attribute change" { var count: usize = 0; while (try it.next(alloc)) |run| { count += 1; - _ = try shaper.shape(run, &cf_release_pool); + _ = try shaper.shape(run); } try testing.expectEqual(@as(usize, 2), count); } @@ -1674,7 +1540,7 @@ test "shape cell attribute change" { var count: usize = 0; while (try it.next(alloc)) |run| { count += 1; - _ = try shaper.shape(run, &cf_release_pool); + _ = try shaper.shape(run); } try testing.expectEqual(@as(usize, 2), count); } @@ -1699,7 +1565,7 @@ test "shape cell attribute change" { var count: usize = 0; while (try it.next(alloc)) |run| { count += 1; - _ = try shaper.shape(run, &cf_release_pool); + _ = try shaper.shape(run); } try testing.expectEqual(@as(usize, 1), count); } @@ -1723,7 +1589,7 @@ test "shape cell attribute change" { var count: usize = 0; while (try it.next(alloc)) |run| { count += 1; - _ = try shaper.shape(run, &cf_release_pool); + _ = try shaper.shape(run); } try testing.expectEqual(@as(usize, 1), count); } diff --git a/src/renderer/Metal.zig b/src/renderer/Metal.zig index f4813fc17..73afdbd6e 100644 --- a/src/renderer/Metal.zig +++ b/src/renderer/Metal.zig @@ -1018,9 +1018,6 @@ pub fn updateFrame( if (critical.preedit) |p| p.deinit(self.alloc); } - var cf_release_pool = std.ArrayList(*anyopaque).init(self.alloc); - try cf_release_pool.ensureTotalCapacity(state.terminal.rows * 8); - // Build our GPU cells try self.rebuildCells( critical.full_rebuild, @@ -1029,27 +1026,23 @@ pub fn updateFrame( critical.preedit, critical.cursor_style, &critical.color_palette, - &cf_release_pool, ); - if (cf_release_pool.items.len > 0) { - const items = try cf_release_pool.toOwnedSlice(); + if (self.font_shaper.cf_release_pool.items.len > 0) { + const alloc = self.font_shaper.alloc; + const items = try self.font_shaper.cf_release_pool.toOwnedSlice(alloc); if (self.cf_release_thread.mailbox.push( .{ .release = .{ .refs = items, - .alloc = self.alloc, + .alloc = alloc, } }, .{ .forever = {} }, ) != 0) { try self.cf_release_thread.wakeup.notify(); } else { - for (items) |ref| { - macos.foundation.CFRelease(ref); - } - self.alloc.free(items); + for (items) |ref| macos.foundation.CFRelease(ref); + alloc.free(items); } - } else { - cf_release_pool.deinit(); } // Update our viewport pin @@ -1931,7 +1924,6 @@ fn rebuildCells( preedit: ?renderer.State.Preedit, cursor_style_: ?renderer.CursorStyle, color_palette: *const terminal.color.Palette, - cf_release_pool: *std.ArrayList(*anyopaque), ) !void { // const start = try std.time.Instant.now(); // const start_micro = std.time.microTimestamp(); @@ -2013,10 +2005,7 @@ fn rebuildCells( while (try iter.next(self.alloc)) |run| { // Try to read the cells from the shaping cache if we can. const shaper_cells = self.font_shaper_cache.get(run) orelse cache: { - const cells = if (font.options.backend == .coretext) - try self.font_shaper.shape(run, cf_release_pool) - else - try self.font_shaper.shape(run); + const cells = try self.font_shaper.shape(run); // Try to cache them. If caching fails for any reason we continue // because it is just a performance optimization, not a correctness diff --git a/src/renderer/OpenGL.zig b/src/renderer/OpenGL.zig index 5dd8d9345..8389dbae6 100644 --- a/src/renderer/OpenGL.zig +++ b/src/renderer/OpenGL.zig @@ -1025,16 +1025,7 @@ pub fn rebuildCells( while (try iter.next(self.alloc)) |run| { // Try to read the cells from the shaping cache if we can. const shaper_cells = self.font_shaper_cache.get(run) orelse cache: { - const cells = if (font.options.backend == .coretext) ct: { - var cf_release_pool = std.ArrayList(*anyopaque).init(self.alloc); - defer { - for (cf_release_pool.items) |ref| { - @import("macos").foundation.CFRelease(ref); - } - cf_release_pool.deinit(); - } - break :ct try self.font_shaper.shape(run, &cf_release_pool); - } else try self.font_shaper.shape(run); + const cells = try self.font_shaper.shape(run); // Try to cache them. If caching fails for any reason we continue // because it is just a performance optimization, not a correctness From 71353d016e45336078ba95f8fc139d394c106351 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sat, 22 Jun 2024 20:42:59 -0700 Subject: [PATCH 11/15] coretext shaper owns CFReleaseThread, works on both Metal and OpenGL now --- src/font/shaper/coretext.zig | 67 +++++++++++++++++++++++++++++++++++- src/renderer/Metal.zig | 49 ++------------------------ src/renderer/OpenGL.zig | 4 +++ 3 files changed, 73 insertions(+), 47 deletions(-) diff --git a/src/font/shaper/coretext.zig b/src/font/shaper/coretext.zig index 5da08b214..19ed2d7bb 100644 --- a/src/font/shaper/coretext.zig +++ b/src/font/shaper/coretext.zig @@ -5,6 +5,8 @@ const Allocator = std.mem.Allocator; const macos = @import("macos"); const trace = @import("tracy").trace; const font = @import("../main.zig"); +const os = @import("../../os/main.zig"); +const terminal = @import("../../terminal/main.zig"); const Face = font.Face; const Collection = font.Collection; const DeferredFace = font.DeferredFace; @@ -14,7 +16,7 @@ const Library = font.Library; const SharedGrid = font.SharedGrid; const Style = font.Style; const Presentation = font.Presentation; -const terminal = @import("../../terminal/main.zig"); +const CFReleaseThread = os.CFReleaseThread; const log = std.log.scoped(.font_shaper); @@ -62,6 +64,12 @@ pub const Shaper = struct { /// sent to the release thread when endFrame is called. cf_release_pool: std.ArrayListUnmanaged(*anyopaque), + /// Dedicated thread for releasing CoreFoundation objects. Some objects, + /// such as those produced by CoreText, have excessively slow release + /// callback logic. + cf_release_thread: *CFReleaseThread, + cf_release_thr: std.Thread, + const CellBuf = std.ArrayListUnmanaged(font.shape.Cell); const CodepointList = std.ArrayListUnmanaged(Codepoint); const Codepoint = struct { @@ -215,6 +223,20 @@ pub const Shaper = struct { const cached_fonts = std.ArrayList(?*macos.foundation.Dictionary).init(alloc); errdefer cached_fonts.deinit(); + // Create the CF release thread. + var cf_release_thread = try alloc.create(CFReleaseThread); + errdefer alloc.destroy(cf_release_thread); + cf_release_thread.* = try CFReleaseThread.init(alloc); + errdefer cf_release_thread.deinit(); + + // Start the CF release thread. + var cf_release_thr = try std.Thread.spawn( + .{}, + CFReleaseThread.threadMain, + .{cf_release_thread}, + ); + cf_release_thr.setName("cf_release") catch {}; + return .{ .alloc = alloc, .cell_buf = .{}, @@ -223,6 +245,8 @@ pub const Shaper = struct { .writing_direction = writing_direction, .cached_fonts = cached_fonts, .cf_release_pool = .{}, + .cf_release_thread = cf_release_thread, + .cf_release_thr = cf_release_thr, }; } @@ -247,6 +271,15 @@ pub const Shaper = struct { ); } self.cf_release_pool.deinit(self.alloc); + + // Stop the CF release thread + { + self.cf_release_thread.stop.notify() catch |err| + log.err("error notifying cf release thread to stop, may stall err={}", .{err}); + self.cf_release_thr.join(); + } + self.cf_release_thread.deinit(); + self.alloc.destroy(self.cf_release_thread); } /// Release all cached fonts. @@ -258,6 +291,38 @@ pub const Shaper = struct { } } + pub fn endFrame(self: *Shaper) void { + if (self.cf_release_pool.items.len == 0) return; + + // Get all the items in the pool as an owned slice so we can + // send it to the dedicated release thread. + const items = self.cf_release_pool.toOwnedSlice(self.alloc) catch |err| { + log.warn("error converting release pool to owned slice, slow release err={}", .{err}); + for (self.cf_release_pool.items) |ref| macos.foundation.CFRelease(ref); + self.cf_release_pool.clearRetainingCapacity(); + return; + }; + + // Send the items. If the send succeeds then we wake up the + // thread to process the items. If the send fails then do a manual + // cleanup. + if (self.cf_release_thread.mailbox.push(.{ .release = .{ + .refs = items, + .alloc = self.alloc, + } }, .{ .forever = {} }) != 0) { + self.cf_release_thread.wakeup.notify() catch |err| { + log.warn( + "error notifying cf release thread to wake up, may stall err={}", + .{err}, + ); + }; + return; + } + + for (items) |ref| macos.foundation.CFRelease(ref); + self.alloc.free(items); + } + pub fn runIterator( self: *Shaper, grid: *SharedGrid, diff --git a/src/renderer/Metal.zig b/src/renderer/Metal.zig index 73afdbd6e..fc5ad382c 100644 --- a/src/renderer/Metal.zig +++ b/src/renderer/Metal.zig @@ -136,12 +136,6 @@ layer: objc.Object, // CAMetalLayer /// a display link. display_link: ?DisplayLink = null, -/// Dedicated thread for releasing CoreFoundation objects some objects, -/// such as those produced by CoreText, have excessively slow release -/// callback logic. -cf_release_thread: CFReleaseThread, -cf_release_thr: std.Thread, - /// Custom shader state. This is only set if we have custom shaders. custom_shader_state: ?CustomShaderState = null, @@ -598,9 +592,6 @@ pub fn init(alloc: Allocator, options: renderer.Options) !Metal { .cursor_color = options.config.cursor_color, .current_background_color = options.config.background, - .cf_release_thread = undefined, - .cf_release_thr = undefined, - // Render state .cells = .{}, .uniforms = .{ @@ -698,18 +689,6 @@ pub fn loopEnter(self: *Metal, thr: *renderer.Thread) !void { &thr.draw_now, ); display_link.start() catch {}; - - // Create the CF release thread. - self.cf_release_thread = try CFReleaseThread.init(self.alloc); - errdefer self.cf_release_thread.deinit(); - - // Start the CF release thread. - self.cf_release_thr = try std.Thread.spawn( - .{}, - CFReleaseThread.threadMain, - .{&self.cf_release_thread}, - ); - self.cf_release_thr.setName("cf_release") catch {}; } /// Called by renderer.Thread when it exits the main loop. @@ -722,15 +701,6 @@ pub fn loopExit(self: *Metal) void { // is gone which is fine. const display_link = self.display_link orelse return; display_link.stop() catch {}; - - // Stop the CF release thread - { - self.cf_release_thread.stop.notify() catch |err| - log.err("error notifying cf release thread to stop, may stall err={}", .{err}); - self.cf_release_thr.join(); - } - - self.cf_release_thread.deinit(); } fn displayLinkCallback( @@ -1028,22 +998,9 @@ pub fn updateFrame( &critical.color_palette, ); - if (self.font_shaper.cf_release_pool.items.len > 0) { - const alloc = self.font_shaper.alloc; - const items = try self.font_shaper.cf_release_pool.toOwnedSlice(alloc); - if (self.cf_release_thread.mailbox.push( - .{ .release = .{ - .refs = items, - .alloc = alloc, - } }, - .{ .forever = {} }, - ) != 0) { - try self.cf_release_thread.wakeup.notify(); - } else { - for (items) |ref| macos.foundation.CFRelease(ref); - alloc.free(items); - } - } + // Notify our shaper we're done for the frame. For some shapers like + // CoreText this triggers off-thread cleanup logic. + self.font_shaper.endFrame(); // Update our viewport pin self.cells_viewport = critical.viewport_pin; diff --git a/src/renderer/OpenGL.zig b/src/renderer/OpenGL.zig index 8389dbae6..3443fb316 100644 --- a/src/renderer/OpenGL.zig +++ b/src/renderer/OpenGL.zig @@ -732,6 +732,10 @@ pub fn updateFrame( critical.cursor_style, &critical.color_palette, ); + + // Notify our shaper we're done for the frame. For some shapers like + // CoreText this triggers off-thread cleanup logic. + self.font_shaper.endFrame(); } } From 9271fd50b6ab6ee606c49771745c586baac87003 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sat, 22 Jun 2024 20:45:30 -0700 Subject: [PATCH 12/15] cache_table and ref_counted_set work on 32-bit machines --- src/cache_table.zig | 6 +++--- src/terminal/ref_counted_set.zig | 3 +-- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/cache_table.zig b/src/cache_table.zig index 2e2a290d5..9f63672e3 100644 --- a/src/cache_table.zig +++ b/src/cache_table.zig @@ -55,6 +55,7 @@ pub fn CacheTable( comptime { assert(std.math.isPowerOfTwo(bucket_count)); + assert(bucket_count <= std.math.maxInt(usize)); } /// `bucket_count` buckets containing `bucket_size` KV pairs each. @@ -79,7 +80,7 @@ pub fn CacheTable( /// make room then it is returned in a struct with its key and value. pub fn put(self: *Self, key: K, value: V) ?KV { const kv: KV = .{ .key = key, .value = value }; - const idx: u64 = self.context.hash(key) % bucket_count; + const idx: usize = @intCast(self.context.hash(key) % bucket_count); // If we have space available in the bucket then we just append if (self.lengths[idx] < bucket_size) { @@ -105,8 +106,7 @@ pub fn CacheTable( /// /// Returns null if no item is found with the provided key. pub fn get(self: *Self, key: K) ?V { - const idx = self.context.hash(key) % bucket_count; - + const idx: usize = @intCast(self.context.hash(key) % bucket_count); const len = self.lengths[idx]; var i: usize = len; while (i > 0) { diff --git a/src/terminal/ref_counted_set.zig b/src/terminal/ref_counted_set.zig index 19d938cfc..65abf5fb1 100644 --- a/src/terminal/ref_counted_set.zig +++ b/src/terminal/ref_counted_set.zig @@ -431,8 +431,7 @@ pub fn RefCountedSet( const hash: u64 = self.context.hash(value); for (0..self.max_psl + 1) |i| { - const p = (hash + i) & self.layout.table_mask; - + const p: usize = @intCast((hash + i) & self.layout.table_mask); const id = table[p]; // Empty bucket, our item cannot have probed to From 260744623514ab0bcac914c7c5de7f126a21b9ee Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sat, 22 Jun 2024 20:49:10 -0700 Subject: [PATCH 13/15] font: add noop endFrame calls to all other shapers --- src/font/shaper/harfbuzz.zig | 4 ++++ src/font/shaper/noop.zig | 4 ++++ src/font/shaper/web_canvas.zig | 4 ++++ 3 files changed, 12 insertions(+) diff --git a/src/font/shaper/harfbuzz.zig b/src/font/shaper/harfbuzz.zig index 11adecb76..7599a624d 100644 --- a/src/font/shaper/harfbuzz.zig +++ b/src/font/shaper/harfbuzz.zig @@ -74,6 +74,10 @@ pub const Shaper = struct { self.hb_feats.deinit(self.alloc); } + pub fn endFrame(self: *const Shaper) void { + _ = self; + } + /// Returns an iterator that returns one text run at a time for the /// given terminal row. Note that text runs are are only valid one at a time /// for a Shaper struct since they share state. diff --git a/src/font/shaper/noop.zig b/src/font/shaper/noop.zig index 310b5cf40..25b9a055f 100644 --- a/src/font/shaper/noop.zig +++ b/src/font/shaper/noop.zig @@ -64,6 +64,10 @@ pub const Shaper = struct { self.run_state.deinit(self.alloc); } + pub fn endFrame(self: *const Shaper) void { + _ = self; + } + pub fn runIterator( self: *Shaper, grid: *SharedGrid, diff --git a/src/font/shaper/web_canvas.zig b/src/font/shaper/web_canvas.zig index 53fb77f4b..f38ab885a 100644 --- a/src/font/shaper/web_canvas.zig +++ b/src/font/shaper/web_canvas.zig @@ -52,6 +52,10 @@ pub const Shaper = struct { self.* = undefined; } + pub fn endFrame(self: *const Shaper) void { + _ = self; + } + /// Returns an iterator that returns one text run at a time for the /// given terminal row. Note that text runs are are only valid one at a time /// for a Shaper struct since they share state. From 3b36dbb53e2bb2dcda54e80c05889b6156723534 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sat, 22 Jun 2024 20:56:23 -0700 Subject: [PATCH 14/15] font/coretext: cached fonts uses unmanaged arraylist --- src/font/shaper/coretext.zig | 35 ++++++++++++++--------------------- 1 file changed, 14 insertions(+), 21 deletions(-) diff --git a/src/font/shaper/coretext.zig b/src/font/shaper/coretext.zig index 19ed2d7bb..8ae98e1d5 100644 --- a/src/font/shaper/coretext.zig +++ b/src/font/shaper/coretext.zig @@ -57,7 +57,7 @@ pub const Shaper = struct { /// /// Fonts are cached as attribute dictionaries to be applied directly to /// attributed strings. - cached_fonts: std.ArrayList(?*macos.foundation.Dictionary), + cached_fonts: std.ArrayListUnmanaged(?*macos.foundation.Dictionary), /// The list of CoreFoundation objects to release on the dedicated /// release thread. This is built up over the course of shaping and @@ -220,9 +220,6 @@ pub const Shaper = struct { }; errdefer writing_direction.release(); - const cached_fonts = std.ArrayList(?*macos.foundation.Dictionary).init(alloc); - errdefer cached_fonts.deinit(); - // Create the CF release thread. var cf_release_thread = try alloc.create(CFReleaseThread); errdefer alloc.destroy(cf_release_thread); @@ -243,7 +240,7 @@ pub const Shaper = struct { .run_state = run_state, .features = feats, .writing_direction = writing_direction, - .cached_fonts = cached_fonts, + .cached_fonts = .{}, .cf_release_pool = .{}, .cf_release_thread = cf_release_thread, .cf_release_thr = cf_release_thr, @@ -256,8 +253,12 @@ pub const Shaper = struct { self.features.deinit(); self.writing_direction.release(); - self.releaseCachedFonts(); - self.cached_fonts.deinit(); + { + for (self.cached_fonts.items) |ft| { + if (ft) |f| f.release(); + } + self.cached_fonts.deinit(self.alloc); + } if (self.cf_release_pool.items.len > 0) { for (self.cf_release_pool.items) |ref| macos.foundation.CFRelease(ref); @@ -282,15 +283,6 @@ pub const Shaper = struct { self.alloc.destroy(self.cf_release_thread); } - /// Release all cached fonts. - pub fn releaseCachedFonts(self: *Shaper) void { - for (self.cached_fonts.items) |ft| { - if (ft) |f| { - f.release(); - } - } - } - pub fn endFrame(self: *Shaper) void { if (self.cf_release_pool.items.len == 0) return; @@ -497,16 +489,18 @@ pub const Shaper = struct { ) !*macos.foundation.Dictionary { const index_int = index.int(); + // The cached fonts are indexed directly by the font index, since + // this number is usually low. Therefore, we set any index we haven't + // seen to null. if (self.cached_fonts.items.len <= index_int) { - try self.cached_fonts.ensureTotalCapacity(index_int + 1); + try self.cached_fonts.ensureTotalCapacity(self.alloc, index_int + 1); while (self.cached_fonts.items.len <= index_int) { self.cached_fonts.appendAssumeCapacity(null); } } - if (self.cached_fonts.items[index_int]) |cached| { - return cached; - } + // If we have it, return the cached attr dict. + if (self.cached_fonts.items[index_int]) |cached| return cached; // Features dictionary, font descriptor, font try self.cf_release_pool.ensureUnusedCapacity(self.alloc, 3); @@ -560,7 +554,6 @@ pub const Shaper = struct { }; self.cached_fonts.items[index_int] = attr_dict; - return attr_dict; } From eebc7d4c3a917c90f85aacb7ec0063f85c43352a Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sun, 23 Jun 2024 09:44:54 -0700 Subject: [PATCH 15/15] small stylistic changes --- src/cache_table.zig | 2 +- src/terminal/page.zig | 2 +- src/terminal/ref_counted_set.zig | 70 ++++++++++++-------------------- src/terminal/style.zig | 30 +++++++------- 4 files changed, 41 insertions(+), 63 deletions(-) diff --git a/src/cache_table.zig b/src/cache_table.zig index 9f63672e3..7a1a10b2b 100644 --- a/src/cache_table.zig +++ b/src/cache_table.zig @@ -46,7 +46,7 @@ pub fn CacheTable( comptime bucket_size: u8, ) type { return struct { - const Self = CacheTable(K, V, Context, bucket_count, bucket_size); + const Self = @This(); const KV = struct { key: K, diff --git a/src/terminal/page.zig b/src/terminal/page.zig index b14632e46..59d400e4e 100644 --- a/src/terminal/page.zig +++ b/src/terminal/page.zig @@ -197,7 +197,7 @@ pub const Page = struct { .styles = style.Set.init( buf.add(l.styles_start), l.styles_layout, - style.StyleSetContext{}, + .{}, ), .grapheme_alloc = GraphemeAlloc.init( buf.add(l.grapheme_alloc_start), diff --git a/src/terminal/ref_counted_set.zig b/src/terminal/ref_counted_set.zig index 65abf5fb1..c0395bdc3 100644 --- a/src/terminal/ref_counted_set.zig +++ b/src/terminal/ref_counted_set.zig @@ -1,12 +1,12 @@ +const std = @import("std"); +const assert = std.debug.assert; + const size = @import("size.zig"); const Offset = size.Offset; const OffsetBuf = size.OffsetBuf; const fastmem = @import("../fastmem.zig"); -const std = @import("std"); -const assert = std.debug.assert; - /// A reference counted set. /// /// This set is created with some capacity in mind. You can determine @@ -40,19 +40,18 @@ const assert = std.debug.assert; /// A type containing methods to define behaviors. /// - `fn hash(*Context, T) u64` - Return a hash for an item. /// - `fn eql(*Context, T, T) bool` - Check two items for equality. -/// /// - `fn deleted(*Context, T) void` - [OPTIONAL] Deletion callback. /// If present, called whenever an item is finally deleted. /// Useful if the item has memory that needs to be freed. /// pub fn RefCountedSet( comptime T: type, - comptime Id: type, + comptime IdT: type, comptime RefCountInt: type, - comptime Context: type, + comptime ContextT: type, ) type { return struct { - const Self = RefCountedSet(T, Id, RefCountInt, Context); + const Self = @This(); pub const base_align = @max( @alignOf(Context), @@ -65,6 +64,7 @@ pub fn RefCountedSet( pub const Item = struct { /// The value this item represents. value: T = undefined, + /// Metadata for this item. meta: Metadata = .{}, @@ -83,6 +83,10 @@ pub fn RefCountedSet( }; }; + // Re-export these types so they can be referenced by the caller. + pub const Id = IdT; + pub const Context = ContextT; + /// A hash table of item indices table: Offset(Id), @@ -178,47 +182,37 @@ pub fn RefCountedSet( }; } - /// Add an item to the set if not present - /// and increment its reference count. + /// Add an item to the set if not present and increment its ref count. /// /// Returns the item's ID. /// - /// If the set has no more room, then an - /// OutOfMemory error is returned instead. + /// If the set has no more room, then an OutOfMemory error is returned. pub fn add(self: *Self, base: anytype, value: T) error{OutOfMemory}!Id { const items = self.items.ptr(base); // Trim dead items from the end of the list. while (self.next_id > 1 and items[self.next_id - 1].meta.ref == 0) { self.next_id -= 1; - self.deleteItem(base, self.next_id); } // If we still don't have an available ID, we're out of memory. - if (self.next_id >= self.layout.cap) { - return error.OutOfMemory; - } + if (self.next_id >= self.layout.cap) return error.OutOfMemory; const id = self.upsert(base, value, self.next_id); items[id].meta.ref += 1; - if (id == self.next_id) { - self.next_id += 1; - } + if (id == self.next_id) self.next_id += 1; return id; } - /// Add an item to the set if not present - /// and increment its reference count. - /// If possible, use the provided ID. + /// Add an item to the set if not present and increment its + /// ref count. If possible, use the provided ID. /// - /// Returns the item's ID, or null - /// if the provided ID was used. + /// Returns the item's ID, or null if the provided ID was used. /// - /// If the set has no more room, then an - /// OutOfMemory error is returned instead. + /// If the set has no more room, then an OutOfMemory error is returned. pub fn addWithId(self: *Self, base: anytype, value: T, id: Id) error{OutOfMemory}!?Id { const items = self.items.ptr(base); @@ -371,11 +365,6 @@ pub fn RefCountedSet( return tb_ct; } - //================================================// - // The functions below are all internal functions // - // for performing operations on the hash table. // - //================================================// - /// Delete an item, removing any references from /// the table, and freeing its ID to be re-used. fn deleteItem(self: *Self, base: anytype, id: Id) void { @@ -384,13 +373,9 @@ pub fn RefCountedSet( const item = items[id]; - if (item.meta.bucket > self.layout.table_cap) { - return; - } + if (item.meta.bucket > self.layout.table_cap) return; - if (table[item.meta.bucket] != id) { - return; - } + if (table[item.meta.bucket] != id) return; if (comptime @hasDecl(Context, "deleted")) { // Inform the context struct that we're @@ -471,12 +456,11 @@ pub fn RefCountedSet( /// Find the provided value in the hash table, or add a new item /// for it if not present. If a new item is added, `new_id` will - /// be used as the ID. + /// be used as the ID. If an existing item is found, the `new_id` + /// is ignored and the existing item's ID is returned. fn upsert(self: *Self, base: anytype, value: T, new_id: Id) Id { // If the item already exists, return it. - if (self.lookup(base, value)) |id| { - return id; - } + if (self.lookup(base, value)) |id| return id; const table = self.table.ptr(base); const items = self.items.ptr(base); @@ -484,10 +468,7 @@ pub fn RefCountedSet( // The new item that we'll put in to the table. var new_item: Item = .{ .value = value, - .meta = .{ - .psl = 0, - .ref = 0, - }, + .meta = .{ .psl = 0, .ref = 0 }, }; const hash: u64 = self.context.hash(value); @@ -508,7 +489,6 @@ pub fn RefCountedSet( held_item.meta.bucket = p; self.psl_stats[held_item.meta.psl] += 1; self.max_psl = @max(self.max_psl, held_item.meta.psl); - break; } diff --git a/src/terminal/style.zig b/src/terminal/style.zig index b5bfff6e0..c1b6fcf54 100644 --- a/src/terminal/style.zig +++ b/src/terminal/style.zig @@ -62,11 +62,11 @@ pub const Style = struct { _ = try writer.write("Color.none"); }, .palette => |p| { - _ = try writer.print("Color.palette{{ {} }}", .{ p }); + _ = try writer.print("Color.palette{{ {} }}", .{p}); }, .rgb => |rgb| { _ = try writer.print("Color.rgb{{ {}, {}, {} }}", .{ rgb.r, rgb.g, rgb.b }); - } + }, } } }; @@ -243,23 +243,21 @@ pub const Style = struct { } }; -pub const StyleSetContext = struct { - pub fn hash(self: *StyleSetContext, style: Style) u64 { - _ = self; - return style.hash(); - } - - pub fn eql(self: *StyleSetContext, a: Style, b: Style) bool { - _ = self; - return a.eql(b); - } -}; - pub const Set = RefCountedSet( Style, Id, size.CellCountInt, - StyleSetContext, + struct { + pub fn hash(self: *const @This(), style: Style) u64 { + _ = self; + return style.hash(); + } + + pub fn eql(self: *const @This(), a: Style, b: Style) bool { + _ = self; + return a.eql(b); + } + }, ); test "Set basic usage" { @@ -272,7 +270,7 @@ test "Set basic usage" { const style: Style = .{ .flags = .{ .bold = true } }; const style2: Style = .{ .flags = .{ .italic = true } }; - var set = Set.init(OffsetBuf.init(buf), layout, StyleSetContext{}); + var set = Set.init(OffsetBuf.init(buf), layout, .{}); // Add style const id = try set.add(buf, style);