From a7e6f1a07040c150ac69bed24b8e9841425d4c95 Mon Sep 17 00:00:00 2001 From: Qwerasd Date: Mon, 24 Jun 2024 20:09:37 -0400 Subject: [PATCH 1/7] fix(terminal/PageList): clear cells in truncated rows during clone Previously this was a memory leak, styles and graphemes in these rows were never reclaimed. --- src/terminal/PageList.zig | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/terminal/PageList.zig b/src/terminal/PageList.zig index 4d1ac5d2c..523ca3897 100644 --- a/src/terminal/PageList.zig +++ b/src/terminal/PageList.zig @@ -490,6 +490,12 @@ pub fn clone( src.* = old_dst; dirty.setValue(i, dirty.isSet(i + chunk.start)); } + + // We need to clear the rows we're about to truncate. + for (len..page.data.size.rows) |i| { + page.data.clearCells(&rows[i], 0, page.data.size.cols); + } + page.data.size.rows = @intCast(len); total_rows += len; From 6f732cca5503f50e8f8ef3fd5e934a8006aab675 Mon Sep 17 00:00:00 2001 From: Qwerasd Date: Mon, 24 Jun 2024 20:30:37 -0400 Subject: [PATCH 2/7] RefCountedSet: use usize for cap to allow up to max `Id`+1 --- src/terminal/PageList.zig | 4 ++-- src/terminal/page.zig | 2 +- src/terminal/ref_counted_set.zig | 25 +++++++++++++------------ 3 files changed, 16 insertions(+), 15 deletions(-) diff --git a/src/terminal/PageList.zig b/src/terminal/PageList.zig index 523ca3897..2d2973a03 100644 --- a/src/terminal/PageList.zig +++ b/src/terminal/PageList.zig @@ -1845,7 +1845,7 @@ pub const AdjustCapacity = struct { /// Adjust the number of styles in the page. This may be /// rounded up if necessary to fit alignment requirements, /// but it will never be rounded down. - styles: ?u16 = null, + styles: ?usize = null, /// Adjust the number of available grapheme bytes in the page. grapheme_bytes: ?usize = null, @@ -1877,7 +1877,7 @@ pub fn adjustCapacity( var cap = page.data.capacity; if (adjustment.styles) |v| { - const aligned = try std.math.ceilPowerOfTwo(u16, v); + const aligned = try std.math.ceilPowerOfTwo(usize, v); cap.styles = @max(cap.styles, aligned); } if (adjustment.grapheme_bytes) |v| { diff --git a/src/terminal/page.zig b/src/terminal/page.zig index 59d400e4e..82776b3ec 100644 --- a/src/terminal/page.zig +++ b/src/terminal/page.zig @@ -1027,7 +1027,7 @@ pub const Capacity = struct { rows: size.CellCountInt, /// Number of unique styles that can be used on this page. - styles: u16 = 16, + styles: usize = 16, /// Number of bytes to allocate for grapheme data. grapheme_bytes: usize = grapheme_bytes_default, diff --git a/src/terminal/ref_counted_set.zig b/src/terminal/ref_counted_set.zig index 39c3ec55a..6c7c65451 100644 --- a/src/terminal/ref_counted_set.zig +++ b/src/terminal/ref_counted_set.zig @@ -132,21 +132,22 @@ pub fn RefCountedSet( /// /// 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 { + pub fn layout(cap: usize) 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))); + assert(cap <= @as(usize, @intCast(std.math.maxInt(Id))) + 1); + + const table_cap: usize = std.math.ceilPowerOfTwoAssert(usize, cap); + const items_cap: usize = @intFromFloat(load_factor * @as(f64, @floatFromInt(table_cap))); + + const table_mask: Id = @intCast((@as(usize, 1) << std.math.log2_int(usize, table_cap)) - 1); const table_start = 0; - const table_cap_usize: usize = @intCast(table_cap); - const table_end = table_start + table_cap_usize * @sizeOf(Id); + const table_end = table_start + table_cap * @sizeOf(Id); const items_start = std.mem.alignForward(usize, table_end, @alignOf(Item)); - const items_cap_usize: usize = @intCast(items_cap); - const items_end = items_start + items_cap_usize * @sizeOf(Item); + const items_end = items_start + items_cap * @sizeOf(Item); const total_size = items_end; @@ -161,8 +162,8 @@ pub fn RefCountedSet( } pub const Layout = struct { - cap: Id, - table_cap: Id, + cap: usize, + table_cap: usize, table_mask: Id, table_start: usize, items_start: usize, @@ -390,7 +391,7 @@ pub fn RefCountedSet( items[id] = .{}; var p: Id = item.meta.bucket; - var n: Id = (p + 1) & self.layout.table_mask; + var n: Id = @addWithOverflow(p, 1)[0] & self.layout.table_mask; while (table[n] != 0 and items[table[n]].meta.psl > 0) { items[table[n]].meta.bucket = p; @@ -399,7 +400,7 @@ pub fn RefCountedSet( self.psl_stats[items[table[n]].meta.psl] += 1; table[p] = table[n]; p = n; - n = (n + 1) & self.layout.table_mask; + n = @addWithOverflow(n, 1)[0] & self.layout.table_mask; } while (self.max_psl > 0 and self.psl_stats[self.max_psl] == 0) { From 35793ee7cc91881f82eb9751a957fddaf16dbd90 Mon Sep 17 00:00:00 2001 From: Qwerasd Date: Mon, 24 Jun 2024 20:31:42 -0400 Subject: [PATCH 3/7] page integrity checks: detect zombie styles --- src/terminal/page.zig | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/src/terminal/page.zig b/src/terminal/page.zig index 82776b3ec..f48a0ed0c 100644 --- a/src/terminal/page.zig +++ b/src/terminal/page.zig @@ -237,6 +237,7 @@ pub const Page = struct { MissingStyle, UnmarkedStyleRow, MismatchedStyleRef, + ZombieStyles, InvalidStyleCount, InvalidSpacerTailLocation, InvalidSpacerHeadLocation, @@ -429,6 +430,37 @@ pub const Page = struct { } } } + + // Verify there are no zombie styles, that is, styles in the + // set with ref counts > 0, which are not present in the page. + { + const styles_table = self.styles.table.ptr(self.memory)[0..self.styles.layout.table_cap]; + const styles_items = self.styles.items.ptr(self.memory)[0..self.styles.layout.cap]; + + var zombies: usize = 0; + + for (styles_table) |id| { + if (id == 0) continue; + const item = styles_items[id]; + if (item.meta.ref == 0) continue; + + const expected = styles_seen.get(id) orelse 0; + if (expected > 0) continue; + + if (item.meta.ref > expected) { + zombies += 1; + } + } + + // Just 1 zombie style might be the cursor style, so ignore it. + if (zombies > 1) { + log.warn( + "page integrity violation zombie styles count={}", + .{zombies}, + ); + return IntegrityError.ZombieStyles; + } + } } /// Clone the contents of this page. This will allocate new memory From 93b038f490859e515de5f8454f58e1d7dcf886c4 Mon Sep 17 00:00:00 2001 From: Qwerasd Date: Mon, 24 Jun 2024 20:40:05 -0400 Subject: [PATCH 4/7] fix(RefCountedSet): add NeedsRehash error and fix PSL counting bug Prevent bad input from causing repeated OutOfMemory errors by erroring with NeedsRehash instead when there are unused dead IDs available. Additionally, properly decrement PSL stats when reviving dead IDs. --- src/inspector/page.zig | 2 +- src/terminal/Screen.zig | 55 ++++++++++-------- src/terminal/Terminal.zig | 46 +++++++-------- src/terminal/page.zig | 2 +- src/terminal/ref_counted_set.zig | 97 ++++++++++++++++++-------------- 5 files changed, 109 insertions(+), 93 deletions(-) diff --git a/src/inspector/page.zig b/src/inspector/page.zig index 29e76be78..d74f07b1c 100644 --- a/src/inspector/page.zig +++ b/src/inspector/page.zig @@ -37,7 +37,7 @@ pub fn render(page: *const terminal.Page) void { } { _ = cimgui.c.igTableSetColumnIndex(1); - cimgui.c.igText("%d", page.styles.count(page.memory)); + cimgui.c.igText("%d", page.styles.count()); } } { diff --git a/src/terminal/Screen.zig b/src/terminal/Screen.zig index cbe278ec5..09232d2ae 100644 --- a/src/terminal/Screen.zig +++ b/src/terminal/Screen.zig @@ -1175,7 +1175,7 @@ pub fn setAttribute(self: *Screen, attr: sgr.Attribute) !void { pub fn manualStyleUpdate(self: *Screen) !void { var page = &self.cursor.page_pin.page.data; - // std.log.warn("active styles={}", .{page.styles.count(page.memory)}); + // std.log.warn("active styles={}", .{page.styles.count()}); // Release our previous style if it was not default. if (self.cursor.style_id != style.default_id) { @@ -1201,12 +1201,17 @@ pub fn manualStyleUpdate(self: *Screen) !void { const id = page.styles.add( page.memory, self.cursor.style, - ) catch id: { - // Our style map is full. Let's allocate a new - // page by doubling the size and then try again. + ) catch |err| id: { + // Our style map is full or needs to be rehashed, + // so we allocate a new page, which will rehash, + // and double the style capacity for it if it was + // full. const node = try self.pages.adjustCapacity( self.cursor.page_pin.page, - .{ .styles = page.capacity.styles * 2 }, + switch (err) { + error.OutOfMemory => .{ .styles = page.capacity.styles * 2 }, + error.NeedsRehash => .{}, + }, ); page = &node.data; @@ -2388,17 +2393,17 @@ test "Screen cursorCopy style deref" { var s2 = try Screen.init(alloc, 10, 10, 0); defer s2.deinit(); - const page = s2.cursor.page_pin.page.data; + const page = &s2.cursor.page_pin.page.data; // Bold should create our style try s2.setAttribute(.{ .bold = {} }); - try testing.expectEqual(@as(usize, 1), page.styles.count(page.memory)); + try testing.expectEqual(@as(usize, 1), page.styles.count()); try testing.expect(s2.cursor.style.flags.bold); // Copy default style, should release our style try s2.cursorCopy(s.cursor); try testing.expect(!s2.cursor.style.flags.bold); - try testing.expectEqual(@as(usize, 0), page.styles.count(page.memory)); + try testing.expectEqual(@as(usize, 0), page.styles.count()); } test "Screen cursorCopy style copy" { @@ -2411,10 +2416,10 @@ test "Screen cursorCopy style copy" { var s2 = try Screen.init(alloc, 10, 10, 0); defer s2.deinit(); - const page = s2.cursor.page_pin.page.data; + const page = &s2.cursor.page_pin.page.data; try s2.cursorCopy(s.cursor); try testing.expect(s2.cursor.style.flags.bold); - try testing.expectEqual(@as(usize, 1), page.styles.count(page.memory)); + try testing.expectEqual(@as(usize, 1), page.styles.count()); } test "Screen style basics" { @@ -2423,19 +2428,19 @@ test "Screen style basics" { var s = try Screen.init(alloc, 80, 24, 1000); defer s.deinit(); - const page = s.cursor.page_pin.page.data; - try testing.expectEqual(@as(usize, 0), page.styles.count(page.memory)); + const page = &s.cursor.page_pin.page.data; + try testing.expectEqual(@as(usize, 0), page.styles.count()); // Set a new style try s.setAttribute(.{ .bold = {} }); try testing.expect(s.cursor.style_id != 0); - try testing.expectEqual(@as(usize, 1), page.styles.count(page.memory)); + try testing.expectEqual(@as(usize, 1), page.styles.count()); try testing.expect(s.cursor.style.flags.bold); // Set another style, we should still only have one since it was unused try s.setAttribute(.{ .italic = {} }); try testing.expect(s.cursor.style_id != 0); - try testing.expectEqual(@as(usize, 1), page.styles.count(page.memory)); + try testing.expectEqual(@as(usize, 1), page.styles.count()); try testing.expect(s.cursor.style.flags.italic); } @@ -2445,18 +2450,18 @@ test "Screen style reset to default" { var s = try Screen.init(alloc, 80, 24, 1000); defer s.deinit(); - const page = s.cursor.page_pin.page.data; - try testing.expectEqual(@as(usize, 0), page.styles.count(page.memory)); + const page = &s.cursor.page_pin.page.data; + try testing.expectEqual(@as(usize, 0), page.styles.count()); // Set a new style try s.setAttribute(.{ .bold = {} }); try testing.expect(s.cursor.style_id != 0); - try testing.expectEqual(@as(usize, 1), page.styles.count(page.memory)); + try testing.expectEqual(@as(usize, 1), page.styles.count()); // Reset to default try s.setAttribute(.{ .reset_bold = {} }); try testing.expect(s.cursor.style_id == 0); - try testing.expectEqual(@as(usize, 0), page.styles.count(page.memory)); + try testing.expectEqual(@as(usize, 0), page.styles.count()); } test "Screen style reset with unset" { @@ -2465,18 +2470,18 @@ test "Screen style reset with unset" { var s = try Screen.init(alloc, 80, 24, 1000); defer s.deinit(); - const page = s.cursor.page_pin.page.data; - try testing.expectEqual(@as(usize, 0), page.styles.count(page.memory)); + const page = &s.cursor.page_pin.page.data; + try testing.expectEqual(@as(usize, 0), page.styles.count()); // Set a new style try s.setAttribute(.{ .bold = {} }); try testing.expect(s.cursor.style_id != 0); - try testing.expectEqual(@as(usize, 1), page.styles.count(page.memory)); + try testing.expectEqual(@as(usize, 1), page.styles.count()); // Reset to default try s.setAttribute(.{ .unset = {} }); try testing.expect(s.cursor.style_id == 0); - try testing.expectEqual(@as(usize, 0), page.styles.count(page.memory)); + try testing.expectEqual(@as(usize, 0), page.styles.count()); } test "Screen clearRows active one line" { @@ -2522,13 +2527,13 @@ test "Screen clearRows active styled line" { try s.setAttribute(.{ .unset = {} }); // We should have one style - const page = s.cursor.page_pin.page.data; - try testing.expectEqual(@as(usize, 1), page.styles.count(page.memory)); + const page = &s.cursor.page_pin.page.data; + try testing.expectEqual(@as(usize, 1), page.styles.count()); s.clearRows(.{ .active = .{} }, null, false); // We should have none because active cleared it - try testing.expectEqual(@as(usize, 0), page.styles.count(page.memory)); + try testing.expectEqual(@as(usize, 0), page.styles.count()); const str = try s.dumpStringAlloc(alloc, .{ .screen = .{} }); defer alloc.free(str); diff --git a/src/terminal/Terminal.zig b/src/terminal/Terminal.zig index 09223aeeb..8dcb46133 100644 --- a/src/terminal/Terminal.zig +++ b/src/terminal/Terminal.zig @@ -2826,8 +2826,8 @@ test "Terminal: print over wide char with bold" { try t.print(0x1F600); // Smiley face // verify we have styles in our style map { - const page = t.screen.cursor.page_pin.page.data; - try testing.expectEqual(@as(usize, 1), page.styles.count(page.memory)); + const page = &t.screen.cursor.page_pin.page.data; + try testing.expectEqual(@as(usize, 1), page.styles.count()); } // Go back and overwrite with no style @@ -2837,8 +2837,8 @@ test "Terminal: print over wide char with bold" { // verify our style is gone { - const page = t.screen.cursor.page_pin.page.data; - try testing.expectEqual(@as(usize, 0), page.styles.count(page.memory)); + const page = &t.screen.cursor.page_pin.page.data; + try testing.expectEqual(@as(usize, 0), page.styles.count()); } try testing.expect(t.isDirty(.{ .screen = .{ .x = 0, .y = 0 } })); @@ -2856,8 +2856,8 @@ test "Terminal: print over wide char with bg color" { try t.print(0x1F600); // Smiley face // verify we have styles in our style map { - const page = t.screen.cursor.page_pin.page.data; - try testing.expectEqual(@as(usize, 1), page.styles.count(page.memory)); + const page = &t.screen.cursor.page_pin.page.data; + try testing.expectEqual(@as(usize, 1), page.styles.count()); } // Go back and overwrite with no style @@ -2867,8 +2867,8 @@ test "Terminal: print over wide char with bg color" { // verify our style is gone { - const page = t.screen.cursor.page_pin.page.data; - try testing.expectEqual(@as(usize, 0), page.styles.count(page.memory)); + const page = &t.screen.cursor.page_pin.page.data; + try testing.expectEqual(@as(usize, 0), page.styles.count()); } try testing.expect(t.isDirty(.{ .screen = .{ .x = 0, .y = 0 } })); @@ -3322,7 +3322,7 @@ test "Terminal: overwrite multicodepoint grapheme clears grapheme data" { try testing.expectEqual(@as(usize, 2), t.screen.cursor.x); // We should have one cell with graphemes - const page = t.screen.cursor.page_pin.page.data; + const page = &t.screen.cursor.page_pin.page.data; try testing.expectEqual(@as(usize, 1), page.graphemeCount()); // Move back and overwrite wide @@ -3362,7 +3362,7 @@ test "Terminal: overwrite multicodepoint grapheme tail clears grapheme data" { try testing.expectEqual(@as(usize, 2), t.screen.cursor.x); // We should have one cell with graphemes - const page = t.screen.cursor.page_pin.page.data; + const page = &t.screen.cursor.page_pin.page.data; try testing.expectEqual(@as(usize, 1), page.graphemeCount()); // Move back and overwrite wide @@ -4534,8 +4534,8 @@ test "Terminal: insertLines handles style refs" { try t.setAttribute(.{ .unset = {} }); // verify we have styles in our style map - const page = t.screen.cursor.page_pin.page.data; - try testing.expectEqual(@as(usize, 1), page.styles.count(page.memory)); + const page = &t.screen.cursor.page_pin.page.data; + try testing.expectEqual(@as(usize, 1), page.styles.count()); t.setCursorPos(2, 2); t.insertLines(1); @@ -4547,7 +4547,7 @@ test "Terminal: insertLines handles style refs" { } // verify we have no styles in our style map - try testing.expectEqual(@as(usize, 0), page.styles.count(page.memory)); + try testing.expectEqual(@as(usize, 0), page.styles.count()); } test "Terminal: insertLines outside of scroll region" { @@ -5336,14 +5336,14 @@ test "Terminal: eraseChars handles refcounted styles" { try t.print('C'); // verify we have styles in our style map - const page = t.screen.cursor.page_pin.page.data; - try testing.expectEqual(@as(usize, 1), page.styles.count(page.memory)); + const page = &t.screen.cursor.page_pin.page.data; + try testing.expectEqual(@as(usize, 1), page.styles.count()); t.setCursorPos(1, 1); t.eraseChars(2); // verify we have no styles in our style map - try testing.expectEqual(@as(usize, 0), page.styles.count(page.memory)); + try testing.expectEqual(@as(usize, 0), page.styles.count()); } test "Terminal: eraseChars protected attributes respected with iso" { @@ -7043,7 +7043,7 @@ 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); - const page = t.screen.cursor.page_pin.page.data; + const page = &t.screen.cursor.page_pin.page.data; try testing.expect(page.styles.refCount(page.memory, t.screen.cursor.style_id) > 1); } } @@ -7067,8 +7067,8 @@ test "Terminal: garbage collect overwritten" { } // verify we have no styles in our style map - const page = t.screen.cursor.page_pin.page.data; - try testing.expectEqual(@as(usize, 0), page.styles.count(page.memory)); + const page = &t.screen.cursor.page_pin.page.data; + try testing.expectEqual(@as(usize, 0), page.styles.count()); } test "Terminal: do not garbage collect old styles in use" { @@ -7089,8 +7089,8 @@ test "Terminal: do not garbage collect old styles in use" { } // verify we have no styles in our style map - const page = t.screen.cursor.page_pin.page.data; - try testing.expectEqual(@as(usize, 1), page.styles.count(page.memory)); + const page = &t.screen.cursor.page_pin.page.data; + try testing.expectEqual(@as(usize, 1), page.styles.count()); } test "Terminal: print with style marks the row as styled" { @@ -7425,7 +7425,7 @@ test "Terminal: insertBlanks deleting graphemes" { try t.print(0x1F467); // We should have one cell with graphemes - const page = t.screen.cursor.page_pin.page.data; + const page = &t.screen.cursor.page_pin.page.data; try testing.expectEqual(@as(usize, 1), page.graphemeCount()); t.setCursorPos(1, 1); @@ -7461,7 +7461,7 @@ test "Terminal: insertBlanks shift graphemes" { try t.print(0x1F467); // We should have one cell with graphemes - const page = t.screen.cursor.page_pin.page.data; + const page = &t.screen.cursor.page_pin.page.data; try testing.expectEqual(@as(usize, 1), page.graphemeCount()); t.setCursorPos(1, 1); diff --git a/src/terminal/page.zig b/src/terminal/page.zig index f48a0ed0c..65334460f 100644 --- a/src/terminal/page.zig +++ b/src/terminal/page.zig @@ -500,7 +500,7 @@ pub const Page = struct { return result; } - pub const CloneFromError = Allocator.Error || error{OutOfMemory}; + pub const CloneFromError = Allocator.Error || style.Set.AddError; /// Clone the contents of another page into this page. The capacities /// can be different, but the size of the other page must fit into diff --git a/src/terminal/ref_counted_set.zig b/src/terminal/ref_counted_set.zig index 6c7c65451..a2b9da7cf 100644 --- a/src/terminal/ref_counted_set.zig +++ b/src/terminal/ref_counted_set.zig @@ -13,9 +13,9 @@ const fastmem = @import("../fastmem.zig"); /// 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. +/// When the set exceeds capacity, an `OutOfMemory` or `NeedsRehash` error +/// 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 @@ -108,6 +108,9 @@ pub fn RefCountedSet( /// The backing store of items items: Offset(Item), + /// The number of living items currently stored in the set. + living: Id = 0, + /// The next index to store an item at. /// Id 0 is reserved for unused items. next_id: Id = 1, @@ -185,12 +188,23 @@ pub fn RefCountedSet( }; } + /// Possible errors for `add` and `addWithId`. + pub const AddError = error{ + /// There is not enough memory to add a new item. + /// Remove items or grow and reinitialize. + OutOfMemory, + + /// The set needs to be rehashed, as there are many dead + /// items with lower IDs which are inaccessible for re-use. + NeedsRehash, + }; + /// 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. - pub fn add(self: *Self, base: anytype, value: T) error{OutOfMemory}!Id { + pub fn add(self: *Self, base: anytype, value: T) AddError!Id { const items = self.items.ptr(base); // Trim dead items from the end of the list. @@ -199,14 +213,34 @@ pub fn RefCountedSet( 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 we still don't have an available ID, we can't continue. + if (self.next_id >= self.layout.cap) { + // Arbitrarily chosen, threshold for rehashing. + // If less than 90% of currently allocated IDs + // correspond to living items, we should rehash. + // Otherwise, claim we're out of memory because + // we assume that we'll end up running out of + // memory or rehashing again very soon if we + // rehash with only a few IDs left. + const rehash_threshold = 0.9; + if (self.living < @as(Id, @intFromFloat(@as(f64, @floatFromInt(self.layout.cap)) * rehash_threshold))) { + return AddError.NeedsRehash; + } + + // If we don't have at least 10% dead items then + // we claim we're out of memory. + return AddError.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 (items[id].meta.ref == 1) { + self.living += 1; + } + return id; } @@ -216,7 +250,7 @@ pub fn RefCountedSet( /// 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. - pub fn addWithId(self: *Self, base: anytype, value: T, id: Id) error{OutOfMemory}!?Id { + pub fn addWithId(self: *Self, base: anytype, value: T, id: Id) AddError!?Id { const items = self.items.ptr(base); if (id < self.next_id) { @@ -227,6 +261,8 @@ pub fn RefCountedSet( items[added_id].meta.ref += 1; + self.living += 1; + return if (added_id == id) null else added_id; } else if (self.context.eql(value, items[id].value)) { items[id].meta.ref += 1; @@ -303,6 +339,10 @@ pub fn RefCountedSet( assert(item.meta.ref > 0); item.meta.ref -= 1; + + if (item.meta.ref == 0) { + self.living -= 1; + } } /// Release a specified number of references to an item by its ID. @@ -317,6 +357,10 @@ pub fn RefCountedSet( assert(item.meta.ref >= n); item.meta.ref -= n; + + if (item.meta.ref == 0) { + self.living -= 1; + } } /// Get the ref count for an item by its ID. @@ -330,42 +374,8 @@ pub fn RefCountedSet( } /// 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; + pub fn count(self: *Self) usize { + return self.living; } /// Delete an item, removing any references from @@ -509,6 +519,7 @@ pub fn RefCountedSet( chosen_id = id; held_item.meta.bucket = p; + self.psl_stats[item.meta.psl] -= 1; self.psl_stats[held_item.meta.psl] += 1; self.max_psl = @max(self.max_psl, held_item.meta.psl); From 44c75931b423d8291d77e16aa57463ce6340aa18 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 24 Jun 2024 20:33:28 -0700 Subject: [PATCH 5/7] terminal: ref counted set count is const --- src/terminal/ref_counted_set.zig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/terminal/ref_counted_set.zig b/src/terminal/ref_counted_set.zig index a2b9da7cf..d99a0baca 100644 --- a/src/terminal/ref_counted_set.zig +++ b/src/terminal/ref_counted_set.zig @@ -374,7 +374,7 @@ pub fn RefCountedSet( } /// Get the current number of non-dead items in the set. - pub fn count(self: *Self) usize { + pub fn count(self: *const Self) usize { return self.living; } From 368960d76ad52139b4dac9b656adc1977bc35ebd Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 24 Jun 2024 20:37:11 -0700 Subject: [PATCH 6/7] use +% for overflow --- src/terminal/ref_counted_set.zig | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/terminal/ref_counted_set.zig b/src/terminal/ref_counted_set.zig index d99a0baca..ae434a149 100644 --- a/src/terminal/ref_counted_set.zig +++ b/src/terminal/ref_counted_set.zig @@ -401,7 +401,7 @@ pub fn RefCountedSet( items[id] = .{}; var p: Id = item.meta.bucket; - var n: Id = @addWithOverflow(p, 1)[0] & self.layout.table_mask; + 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; @@ -410,7 +410,7 @@ pub fn RefCountedSet( self.psl_stats[items[table[n]].meta.psl] += 1; table[p] = table[n]; p = n; - n = @addWithOverflow(n, 1)[0] & self.layout.table_mask; + n = (p +% 1) & self.layout.table_mask; } while (self.max_psl > 0 and self.psl_stats[self.max_psl] == 0) { From b62806360b6a566a1791023fc50e51043b097219 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 24 Jun 2024 20:58:11 -0700 Subject: [PATCH 7/7] terminal: add test for pagelist to clear styles --- src/terminal/PageList.zig | 50 ++++++++++++++++++++++++++++++++ src/terminal/ref_counted_set.zig | 5 +--- 2 files changed, 51 insertions(+), 4 deletions(-) diff --git a/src/terminal/PageList.zig b/src/terminal/PageList.zig index 2d2973a03..f5a2817c6 100644 --- a/src/terminal/PageList.zig +++ b/src/terminal/PageList.zig @@ -4765,6 +4765,56 @@ test "PageList clone partial trimmed left" { try testing.expectEqual(@as(usize, 40), s2.totalRows()); } +test "PageList clone partial trimmed left reclaims styles" { + const testing = std.testing; + const alloc = testing.allocator; + + var s = try init(alloc, 80, 20, null); + defer s.deinit(); + try testing.expectEqual(@as(usize, s.rows), s.totalRows()); + try s.growRows(30); + + // Style the rows we're trimming + { + try testing.expect(s.pages.first == s.pages.last); + const page = &s.pages.first.?.data; + + const style: stylepkg.Style = .{ .flags = .{ .bold = true } }; + const style_id = try page.styles.add(page.memory, style); + + var it = s.rowIterator(.left_up, .{ .screen = .{} }, .{ .screen = .{ .y = 9 } }); + while (it.next()) |p| { + const rac = p.rowAndCell(); + rac.row.styled = true; + rac.cell.* = .{ + .content_tag = .codepoint, + .content = .{ .codepoint = 'A' }, + .style_id = style_id, + }; + page.styles.use(page.memory, style_id); + } + + // We're over-counted by 1 because `add` implies `use`. + page.styles.release(page.memory, style_id); + + // Expect to have one style + try testing.expectEqual(1, page.styles.count()); + } + + var s2 = try s.clone(.{ + .top = .{ .screen = .{ .y = 10 } }, + .memory = .{ .alloc = alloc }, + }); + defer s2.deinit(); + try testing.expectEqual(@as(usize, 40), s2.totalRows()); + + { + try testing.expect(s2.pages.first == s2.pages.last); + const page = &s2.pages.first.?.data; + try testing.expectEqual(0, page.styles.count()); + } +} + test "PageList clone partial trimmed both" { const testing = std.testing; const alloc = testing.allocator; diff --git a/src/terminal/ref_counted_set.zig b/src/terminal/ref_counted_set.zig index ae434a149..c6cf12db5 100644 --- a/src/terminal/ref_counted_set.zig +++ b/src/terminal/ref_counted_set.zig @@ -339,10 +339,7 @@ pub fn RefCountedSet( assert(item.meta.ref > 0); item.meta.ref -= 1; - - if (item.meta.ref == 0) { - self.living -= 1; - } + if (item.meta.ref == 0) self.living -= 1; } /// Release a specified number of references to an item by its ID.