From a797a9783470d326bf28fbbeb853b9f694845a7d Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sat, 11 Jul 2026 16:39:37 -0700 Subject: [PATCH] terminal: fix RefCountedSet capacity boundaries RefCountedSet used maxInt(Id) both as an empty bucket marker and as the final valid bucket in its largest table. A dead-ID reuse sequence at maximum capacity could therefore treat a reset item as table-resident. The floating-point capacity calculation could also produce 65,536 for a u16 page capacity. Mark empty items with the probe sequence field, whose valid range is already capped below maxInt, and calculate the 13/16 load factor exactly. Represent the largest table request as maxInt(Id), which Layout.init rounds to the required power of two. Add maximum-capacity regressions and a randomized live-value oracle for ID reuse and displacement. --- src/terminal/page.zig | 6 +- src/terminal/ref_counted_set.zig | 189 +++++++++++++++++++++++++++++-- src/terminal/size.zig | 6 +- 3 files changed, 187 insertions(+), 14 deletions(-) diff --git a/src/terminal/page.zig b/src/terminal/page.zig index ca6d9854c..653a95c44 100644 --- a/src/terminal/page.zig +++ b/src/terminal/page.zig @@ -3860,11 +3860,11 @@ test "Page exactRowCapacity styles max single row" { const row = &page.rows.ptr(page.memory)[0]; row.styled = true; - // Fill cells with styles until we get OOM, but limit to a reasonable count - // to avoid overflow when computing capacityForCount near maxInt + // Fill cells with styles until we get OOM, but limit the count to keep + // this maximum-dimension regression test reasonably fast. const cells = row.cells.ptr(page.memory)[0..page.size.cols]; var count: usize = 0; - const max_count: usize = 1000; // Limit to avoid overflow in capacity calculation + const max_count: usize = 1000; // Keep test runtime bounded. for (cells, 0..) |*cell, i| { if (count >= max_count) break; const style_id = page.styles.add(page.memory, .{ diff --git a/src/terminal/ref_counted_set.zig b/src/terminal/ref_counted_set.zig index dd76079f4..f978c268e 100644 --- a/src/terminal/ref_counted_set.zig +++ b/src/terminal/ref_counted_set.zig @@ -68,14 +68,34 @@ pub fn RefCountedSet( /// requires more capacity. /// /// Experimentally, this load factor works quite well. - pub const load_factor = 0.8125; + const load_numerator = 13; + const load_denominator = 16; + pub const load_factor: f64 = + @as(f64, load_numerator) / @as(f64, load_denominator); + + /// Maximum number of living items representable by this set. One + /// item-array slot is reserved for ID 0. + pub const max_count: usize = + ((@as(usize, std.math.maxInt(Id)) + 1) * load_numerator / + load_denominator) - 1; /// Returns the minimum capacity needed to store `n` items, /// accounting for the load factor and the reserved ID 0. pub fn capacityForCount(n: usize) usize { if (n == 0) return 0; + assert(n <= max_count); + // +1 because ID 0 is reserved, so we need at least n+1 slots. - return @intFromFloat(@ceil(@as(f64, @floatFromInt(n + 1)) / load_factor)); + const requested = std.math.divCeil( + usize, + (n + 1) * load_denominator, + load_numerator, + ) catch unreachable; + + // The largest supported table has maxInt(Id)+1 slots. Capacity + // is stored in Id by page callers, so represent that request as + // maxInt(Id); Layout.init rounds it up to the same table size. + return @min(requested, std.math.maxInt(Id)); } /// Set item @@ -93,8 +113,11 @@ pub fn RefCountedSet( /// 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, + /// used for Robin Hood hashing. maxInt is reserved to mark + /// items that are not present in the table. We cannot use + /// maxInt(bucket) for this because the largest supported + /// table has that bucket. + psl: Id = std.math.maxInt(Id), /// The reference count for this item. ref: RefCountInt = 0, @@ -105,6 +128,12 @@ pub fn RefCountedSet( pub const Id = IdT; pub const Context = ContextT; + comptime { + // psl_stats caps valid probe lengths at 31, leaving maxInt(Id) + // available as the out-of-table sentinel. + assert(std.math.maxInt(Id) >= 32); + } + /// A hash table of item indices table: Offset(Id), @@ -181,7 +210,8 @@ pub fn RefCountedSet( }; const table_cap: usize = std.math.ceilPowerOfTwoAssert(usize, cap); - const items_cap: usize = @intFromFloat(load_factor * @as(f64, @floatFromInt(table_cap))); + const items_cap = table_cap * load_numerator / + load_denominator; const table_mask: Id = @intCast((@as(usize, 1) << std.math.log2_int(usize, table_cap)) - 1); @@ -455,8 +485,9 @@ pub fn RefCountedSet( const item = items[id]; - if (item.meta.bucket > self.layout.table_cap) return; + if (item.meta.psl == std.math.maxInt(Id)) return; + assert(item.meta.bucket < self.layout.table_cap); assert(table[item.meta.bucket] == id); if (comptime @hasDecl(Context, "deleted")) { @@ -691,7 +722,7 @@ pub fn RefCountedSet( var psl_stats: [32]Id = @splat(0); for (items[0..self.layout.cap], 0..) |item, id| { - if (item.meta.bucket < std.math.maxInt(Id)) { + if (item.meta.psl != std.math.maxInt(Id)) { assert(table[item.meta.bucket] == id); psl_stats[item.meta.psl] += 1; } @@ -705,7 +736,7 @@ pub fn RefCountedSet( for (table[0..self.layout.table_cap], 0..) |id, bucket| { const item = items[id]; - if (item.meta.bucket < std.math.maxInt(Id)) { + if (item.meta.psl != std.math.maxInt(Id)) { assert(item.meta.bucket == bucket); const hash: u64 = ctx.hash(item.value); @@ -721,3 +752,145 @@ pub fn RefCountedSet( } }; } + +test "RefCountedSet random operations against live-value oracle" { + const testing = std.testing; + const Value = u8; + const Id = u8; + const value_count = 16; + + const Context = struct { + pub fn hash(_: *const @This(), value: Value) u64 { + return std.hash.int(@as(u64, value)); + } + + pub fn eql(_: *const @This(), a: Value, b: Value) bool { + return a == b; + } + }; + const Set = RefCountedSet(Value, Id, u16, Context); + + const layout: Set.Layout = .init(64); + const buf = try testing.allocator.alignedAlloc( + u8, + Set.base_align, + layout.total_size, + ); + defer testing.allocator.free(buf); + var set = Set.init(.init(buf), layout, .{}); + + var refs: [value_count]u16 = @splat(0); + var ids: [value_count]?Id = @splat(null); + var living: usize = 0; + var prng = std.Random.DefaultPrng.init(0x5E7); + const random = prng.random(); + + for (0..20_000) |iteration| { + const value: Value = random.uintLessThan(Value, value_count); + switch (random.uintLessThan(u8, 4)) { + 0, 1 => { + const id = try set.add(buf, value); + if (refs[value] == 0) { + ids[value] = id; + living += 1; + } else { + try testing.expectEqual(ids[value].?, id); + } + refs[value] += 1; + }, + 2 => if (refs[value] > 0) { + set.release(buf, ids[value].?); + refs[value] -= 1; + if (refs[value] == 0) { + ids[value] = null; + living -= 1; + } + }, + 3 => try testing.expectEqual(ids[value], set.lookup(buf, value)), + else => unreachable, + } + + // Periodically validate every live ID and value, not only the value + // selected for this operation. This catches displacement, deletion, + // dead-ID reuse, and max-PSL bookkeeping errors that surface later. + if (iteration % 64 == 0) { + try testing.expectEqual(living, set.count()); + for (0..value_count) |i| { + const v: Value = @intCast(i); + try testing.expectEqual(ids[i], set.lookup(buf, v)); + if (ids[i]) |id| { + try testing.expectEqual(v, set.get(buf, id).*); + try testing.expectEqual(refs[i], set.refCount(buf, id)); + } + } + } + } +} + +test "RefCountedSet empty marker at maximum table capacity" { + const testing = std.testing; + const Context = struct { + pub fn hash(_: *const @This(), _: u8) u64 { + // Force one cluster so insertion deterministically reaps the + // higher dead ID used by the reproduction below. + return 0; + } + + pub fn eql(_: *const @This(), a: u8, b: u8) bool { + return a == b; + } + }; + const Set = RefCountedSet(u8, u8, u16, Context); + + // u8 IDs support buckets 0...255. This exercises the same boundary as + // the production u16 set's 65,536-slot maximum without a large buffer. + const layout: Set.Layout = .init(256); + try testing.expectEqual(256, layout.table_cap); + const buf = try testing.allocator.alignedAlloc( + u8, + Set.base_align, + layout.total_size, + ); + defer testing.allocator.free(buf); + var set = Set.init(.init(buf), layout, .{}); + + const first = try set.add(buf, 1); + const second = try set.add(buf, 2); + const third = try set.add(buf, 3); + try testing.expectEqual(1, first); + try testing.expectEqual(2, second); + try testing.expectEqual(3, third); + + // Replacing the dead second ID encounters and reaps the higher dead + // third ID, leaving it reset but still below next_id. + set.release(buf, second); + set.release(buf, third); + try testing.expectEqual(null, try set.addWithId(buf, 4, second)); + + // Reusing that reset gap must recognize that it is not in the table. + // A bucket-based maxInt sentinel aliases bucket 255 at this capacity. + try testing.expectEqual(null, try set.addWithId(buf, 5, third)); + try testing.expectEqual(third, set.lookup(buf, 5).?); + try testing.expectEqual(@as(usize, 3), set.count()); +} + +test "RefCountedSet capacityForCount maximum" { + const testing = std.testing; + const Context = struct { + pub fn hash(_: *const @This(), value: u16) u64 { + return std.hash.int(@as(u64, value)); + } + + pub fn eql(_: *const @This(), a: u16, b: u16) bool { + return a == b; + } + }; + const Set = RefCountedSet(u16, u16, u16, Context); + + const capacity = Set.capacityForCount(Set.max_count); + try testing.expectEqual(std.math.maxInt(u16), capacity); + + const layout: Set.Layout = .init(capacity); + try testing.expectEqual(@as(usize, 65_536), layout.table_cap); + try testing.expect(layout.cap - 1 >= Set.max_count); +} diff --git a/src/terminal/size.zig b/src/terminal/size.zig index 7be09739e..e1f5243b5 100644 --- a/src/terminal/size.zig +++ b/src/terminal/size.zig @@ -25,9 +25,9 @@ pub const CellCountInt = u16; // We match CellCountInt here because each cell in a single row can have at // most one style, making it simple to split a page by splitting rows. // -// Note due to the way RefCountedSet works, we are short one value, but -// this is a theoretical limit we accept. A page with a single row max -// columns wide would be one short of having every cell have a unique style. +// RefCountedSet reserves both hash-table load headroom and ID 0, so its +// maximum live-item count is lower than maxInt of these types. This is a +// theoretical limit we accept; normal pages split long before reaching it. pub const StyleCountInt = CellCountInt; pub const HyperlinkCountInt = CellCountInt;