From d5ab772b666678aeb6aa609d571eee6949cac538 Mon Sep 17 00:00:00 2001 From: Khang Nguyen Duy Date: Sat, 21 Sep 2024 21:46:25 +0700 Subject: [PATCH 1/3] fix(terminal/PageList): ensure enough pages before first page reuse Running alacritty/vtebench on some machines causes Ghostty to fail on `assert(first != last)` when trying to grow scrollback. We now make sure we have enough pages before trying to reuse pages. --- src/terminal/PageList.zig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/terminal/PageList.zig b/src/terminal/PageList.zig index 705d6ba2d..64580dd9a 100644 --- a/src/terminal/PageList.zig +++ b/src/terminal/PageList.zig @@ -1694,7 +1694,7 @@ pub fn grow(self: *PageList) !?*List.Node { // If allocation would exceed our max size, we prune the first page. // We don't need to reallocate because we can simply reuse that first // page. - if (self.page_size + PagePool.item_size > self.maxSize()) prune: { + if (self.pages.len > 1 and self.page_size + PagePool.item_size > self.maxSize()) prune: { // If we need to add more memory to ensure our active area is // satisfied then we do not prune. if (self.growRequiredForActive()) break :prune; From f0eb5d0d43833c1b7537f3ab0422e7a69c51003c Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sat, 21 Sep 2024 20:54:32 -0700 Subject: [PATCH 2/3] terminal: test for scenario where grow() has to prune with a single page See comments in the test, also this: > The scenario is if you adjustCapacity a page beyond a std_size then our > precondition no longer holds. PageList.adjustCapacity makes no guarantee > that the resulting capacity fits within a standard page size. It is in fact > documented this way and that it is slow since we have to mmap out of our > memory pool. --- src/terminal/PageList.zig | 45 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/src/terminal/PageList.zig b/src/terminal/PageList.zig index 64580dd9a..9ea15944c 100644 --- a/src/terminal/PageList.zig +++ b/src/terminal/PageList.zig @@ -3772,6 +3772,51 @@ test "PageList grow allows exceeding max size for active area" { try testing.expectEqual(start_pages + 1, s.totalPages()); } +test "PageList grow prune required with a single page" { + const testing = std.testing; + const alloc = testing.allocator; + + var s = try init(alloc, 80, 24, 0); + defer s.deinit(); + + // This block is all test setup. There is nothing required about this + // behavior during a refactor. This is setting up a scenario that is + // possible to trigger a bug (#2280). + { + // Adjust our capacity until our page is larger than the standard size. + // This is important because it triggers a scenario where our calculated + // minSize() which is supposed to accommodate 2 pages is no longer true. + var cap = std_capacity; + while (true) { + cap.grapheme_bytes *= 2; + const layout = Page.layout(cap); + if (layout.total_size > std_size) break; + } + + // Adjust to that capacity. After we should still have one page. + _ = try s.adjustCapacity( + s.pages.first.?, + .{ .grapheme_bytes = cap.grapheme_bytes }, + ); + try testing.expect(s.pages.first != null); + try testing.expect(s.pages.first == s.pages.last); + } + + // Figure out the remaining number of rows. This is the amount that + // can be added to the current page before we need to allocate a new + // page. + const rem = rem: { + const page = s.pages.first.?; + break :rem page.data.capacity.rows - page.data.size.rows; + }; + for (0..rem) |_| try testing.expect(try s.grow() == null); + + // The next one we add will trigger a new page. + const new = try s.grow(); + try testing.expect(new != null); + try testing.expect(new != s.pages.first); +} + test "PageList scroll top" { const testing = std.testing; const alloc = testing.allocator; From f858ae13ae3152cb0d1e0e3add6f8b5e19ef76e7 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sat, 21 Sep 2024 20:59:00 -0700 Subject: [PATCH 3/3] terminal: clarify comment --- src/terminal/PageList.zig | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/terminal/PageList.zig b/src/terminal/PageList.zig index 9ea15944c..8d9641bfa 100644 --- a/src/terminal/PageList.zig +++ b/src/terminal/PageList.zig @@ -1694,7 +1694,14 @@ pub fn grow(self: *PageList) !?*List.Node { // If allocation would exceed our max size, we prune the first page. // We don't need to reallocate because we can simply reuse that first // page. - if (self.pages.len > 1 and self.page_size + PagePool.item_size > self.maxSize()) prune: { + // + // We only take this path if we have more than one page since pruning + // reuses the popped page. It is possible to have a single page and + // exceed the max size if that page was adjusted to be larger after + // initial allocation. + if (self.pages.len > 1 and + self.page_size + PagePool.item_size > self.maxSize()) + prune: { // If we need to add more memory to ensure our active area is // satisfied then we do not prune. if (self.growRequiredForActive()) break :prune;