Skip to content

Commit

Permalink
Improve resize performance by switching to AutoArrayHashMapUnmanaged
Browse files Browse the repository at this point in the history 
I noticed that the HashMap iterator showed up prominently in Instruments when quickly
resizing Ghostty.

I think this is related to the [tombstone issue](ziglang/zig#17851),
where the `next()` function has to skip unused meta-nodes.

In that same issue, Andrew is suggesting that the non-array hashmap might get deleted from the
standard library.

After switching to `AutoArrayHashMapUnmanaged`, iteration barely shows up anymore.

Deletion from the pin list should also be fast as swapRemove is used (order does not need to be preserved).

Question is if insertion performance is negatively affected, though I'm not seeing anything obvious.
Still, checking this PR for any perf regressions might be a good idea.

If this pans out, there are more places where this switch might be beneficial.
  • Loading branch information
@cryptocode
cryptocode committed Aug 10, 2024
1 parent edea928 commit 2e88ff1
Showing 1 changed file with 40 additions and 59 deletions.
99 changes: 40 additions & 59 deletions src/terminal/PageList.zig
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ const PagePool = std.heap.MemoryPoolAligned(

/// List of pins, known as "tracked" pins. These are pins that are kept
/// up to date automatically through page-modifying operations.
const PinSet = std.AutoHashMapUnmanaged(*Pin, void);
const PinSet = std.AutoArrayHashMapUnmanaged(*Pin, void);
const PinPool = std.heap.MemoryPool(Pin);

/// The pool of memory used for a pagelist. This can be shared between
Expand Down Expand Up @@ -433,9 +433,8 @@ pub fn clone(
// Updating tracked pins is easy, we just change the page
// pointer but all offsets remain the same.
if (opts.tracked_pins) |remap| {
var pin_it = self.tracked_pins.keyIterator();
while (pin_it.next()) |p_ptr| {
const p = p_ptr.*;
const pin_keys = self.tracked_pins.keys();
for (pin_keys) |p| {
if (p.page != chunk.page) continue;
const new_p = try pool.pins.create();
new_p.* = p.*;
Expand All @@ -458,9 +457,8 @@ pub fn clone(

// Updating tracked pins for the pins that are in the shortened chunk.
if (opts.tracked_pins) |remap| {
var pin_it = self.tracked_pins.keyIterator();
while (pin_it.next()) |p_ptr| {
const p = p_ptr.*;
const pin_keys = self.tracked_pins.keys();
for (pin_keys) |p| {
if (p.page != chunk.page or
p.y >= chunk.end) continue;
const new_p = try pool.pins.create();
Expand Down Expand Up @@ -502,9 +500,8 @@ pub fn clone(

// Updating tracked pins
if (opts.tracked_pins) |remap| {
var pin_it = self.tracked_pins.keyIterator();
while (pin_it.next()) |p_ptr| {
const p = p_ptr.*;
const pin_keys = self.tracked_pins.keys();
for (pin_keys) |p| {
if (p.page != chunk.page or
p.y < chunk.start or
p.y >= chunk.end) continue;
Expand Down Expand Up @@ -810,9 +807,8 @@ const ReflowCursor = struct {

// Handle tracked pin adjustments.
{
var it = list.tracked_pins.keyIterator();
while (it.next()) |p_ptr| {
const p = p_ptr.*;
const pin_keys = list.tracked_pins.keys();
for (pin_keys) |p| {
if (&p.page.data != src_page or
p.y != src_y) continue;

Expand Down Expand Up @@ -861,9 +857,8 @@ const ReflowCursor = struct {

// Move any tracked pins from the source.
{
var it = list.tracked_pins.keyIterator();
while (it.next()) |p_ptr| {
const p = p_ptr.*;
const pin_keys = list.tracked_pins.keys();
for (pin_keys) |p| {
if (&p.page.data != src_page or
p.y != src_y or
p.x != x) continue;
Expand Down Expand Up @@ -1276,9 +1271,8 @@ fn resizeWithoutReflow(self: *PageList, opts: Resize) !void {

// Update all our tracked pins. If they have an X
// beyond the edge, clamp it.
var pin_it = self.tracked_pins.keyIterator();
while (pin_it.next()) |p_ptr| {
const p = p_ptr.*;
const pin_keys = self.tracked_pins.keys();
for (pin_keys) |p| {
if (p.x >= cols) p.x = cols - 1;
}

Expand Down Expand Up @@ -1495,9 +1489,8 @@ fn resizeWithoutReflowGrowCols(
self.pages.insertBefore(chunk.page, new_page);

// Update our tracked pins that pointed to this previous page.
var pin_it = self.tracked_pins.keyIterator();
while (pin_it.next()) |p_ptr| {
const p = p_ptr.*;
const pin_keys = self.tracked_pins.keys();
for (pin_keys) |p| {
if (p.page != chunk.page or
p.y < y_start or
p.y >= y_end) continue;
Expand Down Expand Up @@ -1561,9 +1554,8 @@ fn trimTrailingBlankRows(
// If our tracked pins are in this row then we cannot trim it
// because it implies some sort of importance. If we trimmed this
// we'd invalidate this pin, as well.
var tracked_it = self.tracked_pins.keyIterator();
while (tracked_it.next()) |p_ptr| {
const p = p_ptr.*;
const pin_keys = self.tracked_pins.keys();
for (pin_keys) |p| {
if (p.page != row_pin.page or
p.y != row_pin.y) continue;
return trimmed;
Expand Down Expand Up @@ -1793,9 +1785,8 @@ pub fn grow(self: *PageList) !?*List.Node {

// Update any tracked pins that point to this page to point to the
// new first page to the top-left.
var it = self.tracked_pins.keyIterator();
while (it.next()) |p_ptr| {
const p = p_ptr.*;
const pin_keys = self.tracked_pins.keys();
for (pin_keys) |p| {
if (p.page != first) continue;
p.page = self.pages.first.?;
p.y = 0;
Expand Down Expand Up @@ -1892,9 +1883,8 @@ pub fn adjustCapacity(
try new_page.data.cloneFrom(&page.data, 0, page.data.size.rows);

// Fix up all our tracked pins to point to the new page.
var it = self.tracked_pins.keyIterator();
while (it.next()) |p_ptr| {
const p = p_ptr.*;
const pin_keys = self.tracked_pins.keys();
for (pin_keys) |p| {
if (p.page != page) continue;
p.page = new_page;
}
Expand Down Expand Up @@ -2013,9 +2003,8 @@ pub fn eraseRow(
// We adjust the tracked pins in this page, moving up any that were below
// the removed row.
{
var pin_it = self.tracked_pins.keyIterator();
while (pin_it.next()) |p_ptr| {
const p = p_ptr.*;
const pin_keys = self.tracked_pins.keys();
for (pin_keys) |p| {
if (p.page == page and p.y > pn.y) p.y -= 1;
}
}
Expand Down Expand Up @@ -2064,9 +2053,8 @@ pub fn eraseRow(
// Our tracked pins for this page need to be updated.
// If the pin is in row 0 that means the corresponding row has
// been moved to the previous page. Otherwise, move it up by 1.
var pin_it = self.tracked_pins.keyIterator();
while (pin_it.next()) |p_ptr| {
const p = p_ptr.*;
const pin_keys = self.tracked_pins.keys();
for (pin_keys) |p| {
if (p.page != page) continue;
if (p.y == 0) {
p.page = page.prev.?;
Expand Down Expand Up @@ -2114,9 +2102,8 @@ pub fn eraseRowBounded(
dirty.setRangeValue(.{ .start = pn.y, .end = pn.y + limit }, true);

// Update pins in the shifted region.
var pin_it = self.tracked_pins.keyIterator();
while (pin_it.next()) |p_ptr| {
const p = p_ptr.*;
const pin_keys = self.tracked_pins.keys();
for (pin_keys) |p| {
if (p.page == page and
p.y >= pn.y and
p.y <= pn.y + limit)
Expand Down Expand Up @@ -2147,9 +2134,8 @@ pub fn eraseRowBounded(

// Update tracked pins.
{
var pin_it = self.tracked_pins.keyIterator();
while (pin_it.next()) |p_ptr| {
const p = p_ptr.*;
const pin_keys = self.tracked_pins.keys();
for (pin_keys) |p| {
if (p.page == page and p.y >= pn.y) {
if (p.y == 0) {
p.x = 0;
Expand Down Expand Up @@ -2187,9 +2173,8 @@ pub fn eraseRowBounded(
dirty.setRangeValue(.{ .start = 0, .end = shifted_limit }, true);

// Update pins in the shifted region.
var pin_it = self.tracked_pins.keyIterator();
while (pin_it.next()) |p_ptr| {
const p = p_ptr.*;
const pin_keys = self.tracked_pins.keys();
for (pin_keys) |p| {
if (p.page != page or p.y > shifted_limit) continue;
if (p.y == 0) {
p.page = page.prev.?;
Expand All @@ -2212,9 +2197,8 @@ pub fn eraseRowBounded(
shifted += page.data.size.rows;

// Update tracked pins.
var pin_it = self.tracked_pins.keyIterator();
while (pin_it.next()) |p_ptr| {
const p = p_ptr.*;
const pin_keys = self.tracked_pins.keys();
for (pin_keys) |p| {
if (p.page != page) continue;
if (p.y == 0) {
p.page = page.prev.?;
Expand Down Expand Up @@ -2298,9 +2282,8 @@ pub fn eraseRows(

// Update any tracked pins to shift their y. If it was in the erased
// row then we move it to the top of this page.
var pin_it = self.tracked_pins.keyIterator();
while (pin_it.next()) |p_ptr| {
const p = p_ptr.*;
const pin_keys = self.tracked_pins.keys();
for (pin_keys) |p| {
if (p.page != chunk.page) continue;
if (p.y >= chunk.end) {
p.y -= chunk.end;
Expand Down Expand Up @@ -2357,9 +2340,8 @@ fn erasePage(self: *PageList, page: *List.Node) void {
assert(page.next != null or page.prev != null);

// Update any tracked pins to move to the next page.
var it = self.tracked_pins.keyIterator();
while (it.next()) |p_ptr| {
const p = p_ptr.*;
const pin_keys = self.tracked_pins.keys();
for (pin_keys) |p| {
if (p.page != page) continue;
p.page = page.next orelse page.prev orelse unreachable;
p.y = 0;
Expand Down Expand Up @@ -2401,7 +2383,7 @@ pub fn trackPin(self: *PageList, p: Pin) !*Pin {
/// Untrack a previously tracked pin. This will deallocate the pin.
pub fn untrackPin(self: *PageList, p: *Pin) void {
assert(p != self.viewport_pin);
if (self.tracked_pins.remove(p)) {
if (self.tracked_pins.swapRemove(p)) {
self.pool.pins.destroy(p);
}
}
Expand Down Expand Up @@ -2644,9 +2626,8 @@ pub fn diagram(self: *const PageList, writer: anytype) !void {
// don't wanna bother making this function allocating.
var pin_buf: [16]*Pin = undefined;
var pin_count: usize = 0;
var pin_it = self.tracked_pins.keyIterator();
while (pin_it.next()) |p_ptr| {
const p = p_ptr.*;
const pin_keys = self.tracked_pins.keys();
for (pin_keys) |p| {
if (p.page != chunk.page) continue;
if (p.y != y) continue;
pin_buf[pin_count] = p;
Expand Down

0 comments on commit 2e88ff1

Please sign in to comment.