From d1cd56a56c4244d9d9fadae028cbffc23a1d3d1a Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Thu, 3 Sep 2026 15:17:51 -0700 Subject: [PATCH] terminal: dynamic palette shares the built-in default instead of copying it per terminal --- src/terminal/Terminal.zig | 1 + src/terminal/c/terminal.zig | 5 +- src/terminal/color.zig | 167 ++++++++++++++++++++++++----- src/terminal/snapshot/terminal.zig | 22 ++-- src/termio/Termio.zig | 18 +++- 5 files changed, 169 insertions(+), 44 deletions(-) diff --git a/src/terminal/Terminal.zig b/src/terminal/Terminal.zig index 93cad0ca8..e9cc3b8e6 100644 --- a/src/terminal/Terminal.zig +++ b/src/terminal/Terminal.zig @@ -356,6 +356,7 @@ pub fn init( pub fn deinit(self: *Terminal, alloc: Allocator) void { self.tabstops.deinit(alloc); self.screens.deinit(alloc); + self.colors.palette.deinit(alloc); self.pwd.deinit(alloc); self.title.deinit(alloc); self.glyph_glossary.deinit(alloc); diff --git a/src/terminal/c/terminal.zig b/src/terminal/c/terminal.zig index aed48e25f..4f72bcbdd 100644 --- a/src/terminal/c/terminal.zig +++ b/src/terminal/c/terminal.zig @@ -1363,8 +1363,9 @@ fn setTyped( }, .color_palette => { wrapper.terminal.colors.palette.changeDefault( + wrapper.terminal.gpa(), if (value) |v| color.paletteZval(v) else color.default, - ); + ) catch return .out_of_memory; wrapper.terminal.flags.dirty.palette = true; }, .kitty_image_storage_limit => { @@ -1736,7 +1737,7 @@ fn getTyped( .color_background_default => out.* = (t.colors.background.default orelse return .no_value).cval(), .color_cursor_default => out.* = (t.colors.cursor.default orelse return .no_value).cval(), .color_palette => out.* = color.paletteCval(&t.colors.palette.current), - .color_palette_default => out.* = color.paletteCval(&t.colors.palette.original), + .color_palette_default => out.* = color.paletteCval(t.colors.palette.original), .kitty_image_storage_limit => { if (comptime !build_options.kitty_graphics) return .no_value; out.* = @intCast(t.screens.active.kitty_images.total_limit); diff --git a/src/terminal/color.zig b/src/terminal/color.zig index ce4433bbc..99d749890 100644 --- a/src/terminal/color.zig +++ b/src/terminal/color.zig @@ -2,6 +2,7 @@ const colorpkg = @This(); const std = @import("std"); const builtin = @import("builtin"); +const Allocator = std.mem.Allocator; const assert = @import("../quirks.zig").inlineAssert; const fraction = @import("fraction.zig"); const x11_color = @import("x11_color.zig"); @@ -322,22 +323,37 @@ pub const DynamicPalette = struct { /// The current palette including any user modifications. current: Palette, - /// The original/default palette values. - original: Palette, + /// The original/default palette values. This points at the shared + /// built-in default palette unless a different default was given + /// (see `changeDefault`), in which case it points at an + /// allocator-owned copy that `deinit` frees. Sharing the built-in + /// default saves a full palette copy for every terminal. + original: *const Palette, /// A bitset where each bit represents whether the corresponding /// palette index has been modified from its default value. mask: PaletteMask, - pub const default: DynamicPalette = .init(colorpkg.default); + /// A dynamic palette using the built-in default palette. This owns + /// no memory, but `deinit` is still safe to call on it. + pub const default: DynamicPalette = .{ + .current = colorpkg.default, + .original = &colorpkg.default, + .mask = .initEmpty(), + }; - /// Initialize a dynamic palette with a default palette. - pub fn init(def: Palette) DynamicPalette { - return .{ - .current = def, - .original = def, - .mask = .initEmpty(), - }; + /// Initialize a dynamic palette with a default palette. The result + /// must be released with `deinit`. No memory is allocated if `def` + /// is the built-in default palette. + pub fn init(alloc: Allocator, def: Palette) Allocator.Error!DynamicPalette { + var result: DynamicPalette = .default; + try result.changeDefault(alloc, def); + return result; + } + + pub fn deinit(self: *DynamicPalette, alloc: Allocator) void { + if (self.ownedOriginal()) |owned| alloc.destroy(owned); + self.* = undefined; } /// Set a custom color at the given palette index. @@ -354,16 +370,46 @@ pub const DynamicPalette = struct { /// Reset all colors to their original values. pub fn resetAll(self: *DynamicPalette) void { - self.* = .init(self.original); + self.current = self.original.*; + self.mask = .initEmpty(); } /// Change the default palette, but preserve the changed values. - pub fn changeDefault(self: *DynamicPalette, def: Palette) void { - self.original = def; + /// + /// The built-in default palette is shared rather than copied, so + /// changing back to it releases any owned copy (see `resetDefault`). + /// Any other default is copied into memory owned by this palette, + /// allocated on the first change and reused after that. On allocation + /// failure nothing changes. + pub fn changeDefault( + self: *DynamicPalette, + alloc: Allocator, + def: Palette, + ) Allocator.Error!void { + if (std.meta.eql(def, colorpkg.default)) return self.resetDefault(alloc); + const owned = self.ownedOriginal() orelse try alloc.create(Palette); + owned.* = def; + self.original = owned; + self.applyDefault(def); + } + + /// Change the default palette back to the built-in default palette, + /// preserving the changed values. This releases any owned copy and + /// never allocates, so it is safe to use as the fallback when + /// `changeDefault` fails. + pub fn resetDefault(self: *DynamicPalette, alloc: Allocator) void { + if (self.ownedOriginal()) |owned| alloc.destroy(owned); + self.original = &colorpkg.default; + self.applyDefault(colorpkg.default); + } + + /// Rebuild the current palette from a new default, preserving the + /// changed values. + fn applyDefault(self: *DynamicPalette, def: Palette) void { // Fast path, the palette is usually not changed. if (self.mask.count() == 0) { - self.current = self.original; + self.current = def; return; } @@ -374,6 +420,16 @@ pub const DynamicPalette = struct { while (it.next()) |idx| current[idx] = self.current[idx]; self.current = current; } + + /// The allocator-owned copy of the original palette, or null if the + /// original is the shared built-in default. + fn ownedOriginal(self: *const DynamicPalette) ?*Palette { + if (self.original == &colorpkg.default) return null; + + // Only the shared built-in default is ever const; anything else + // is a copy that we allocated ourselves. + return @constCast(self.original); + } }; /// RGB value that can be changed and reset. This can also be totally unset @@ -991,16 +1047,18 @@ test "RGB: encode" { test "DynamicPalette: init" { const testing = std.testing; - var p: DynamicPalette = .init(default); + var p: DynamicPalette = .default; + defer p.deinit(testing.allocator); try testing.expectEqual(default, p.current); - try testing.expectEqual(default, p.original); + try testing.expectEqual(default, p.original.*); + try testing.expectEqual(&default, p.original); try testing.expectEqual(@as(usize, 0), p.mask.count()); } test "DynamicPalette: set" { const testing = std.testing; - var p: DynamicPalette = .init(default); + var p: DynamicPalette = .default; const new_color = RGB{ .r = 255, .g = 0, .b = 0 }; p.set(0, new_color); @@ -1014,7 +1072,7 @@ test "DynamicPalette: set" { test "DynamicPalette: reset" { const testing = std.testing; - var p: DynamicPalette = .init(default); + var p: DynamicPalette = .default; const new_color = RGB{ .r = 255, .g = 0, .b = 0 }; p.set(0, new_color); @@ -1029,7 +1087,7 @@ test "DynamicPalette: reset" { test "DynamicPalette: resetAll" { const testing = std.testing; - var p: DynamicPalette = .init(default); + var p: DynamicPalette = .default; const new_color = RGB{ .r = 255, .g = 0, .b = 0 }; p.set(0, new_color); @@ -1039,27 +1097,79 @@ test "DynamicPalette: resetAll" { p.resetAll(); try testing.expectEqual(default, p.current); - try testing.expectEqual(default, p.original); + try testing.expectEqual(default, p.original.*); try testing.expectEqual(@as(usize, 0), p.mask.count()); } test "DynamicPalette: changeDefault with no changes" { const testing = std.testing; - var p: DynamicPalette = .init(default); + var p: DynamicPalette = .default; + defer p.deinit(testing.allocator); var new_palette = default; new_palette[0] = RGB{ .r = 100, .g = 100, .b = 100 }; - p.changeDefault(new_palette); - try testing.expectEqual(new_palette, p.original); + try p.changeDefault(testing.allocator, new_palette); + try testing.expectEqual(new_palette, p.original.*); try testing.expectEqual(new_palette, p.current); try testing.expectEqual(@as(usize, 0), p.mask.count()); } +test "DynamicPalette: changeDefault shares the built-in default" { + const testing = std.testing; + const alloc = testing.allocator; + + var new_palette = default; + new_palette[0] = RGB{ .r = 100, .g = 100, .b = 100 }; + + // A custom default is an owned copy. + var p: DynamicPalette = try .init(alloc, new_palette); + defer p.deinit(alloc); + try testing.expect(p.original != &default); + try testing.expectEqual(new_palette, p.original.*); + + // Changing the custom default reuses the copy. + const owned = p.original; + new_palette[1] = RGB{ .r = 101, .g = 101, .b = 101 }; + try p.changeDefault(alloc, new_palette); + try testing.expectEqual(owned, p.original); + try testing.expectEqual(new_palette, p.original.*); + + // Changing back to the built-in default releases the copy (the + // testing allocator would report the leak) and shares it again. + try p.changeDefault(alloc, default); + try testing.expectEqual(&default, p.original); + try testing.expectEqual(default, p.current); + + // resetDefault does the same directly, preserving changed values, + // and never allocates. + try p.changeDefault(alloc, new_palette); + p.set(2, RGB{ .r = 1, .g = 2, .b = 3 }); + var reset_failing: std.testing.FailingAllocator = .init(alloc, .{ + .fail_index = 0, + }); + p.resetDefault(reset_failing.allocator()); + try testing.expect(!reset_failing.has_induced_failure); + try testing.expectEqual(&default, p.original); + try testing.expectEqual(default[0], p.current[0]); + try testing.expectEqual(RGB{ .r = 1, .g = 2, .b = 3 }, p.current[2]); + try testing.expect(p.mask.isSet(2)); + + // The built-in default never allocates. + var failing: std.testing.FailingAllocator = .init(alloc, .{ + .fail_index = 0, + }); + var q: DynamicPalette = try .init(failing.allocator(), default); + defer q.deinit(failing.allocator()); + try testing.expectEqual(&default, q.original); + try testing.expect(!failing.has_induced_failure); +} + test "DynamicPalette: changeDefault preserves changes" { const testing = std.testing; - var p: DynamicPalette = .init(default); + var p: DynamicPalette = .default; + defer p.deinit(testing.allocator); const custom_color = RGB{ .r = 255, .g = 0, .b = 0 }; p.set(5, custom_color); @@ -1069,9 +1179,9 @@ test "DynamicPalette: changeDefault preserves changes" { new_palette[0] = RGB{ .r = 100, .g = 100, .b = 100 }; new_palette[5] = RGB{ .r = 50, .g = 50, .b = 50 }; - p.changeDefault(new_palette); + try p.changeDefault(testing.allocator, new_palette); - try testing.expectEqual(new_palette, p.original); + try testing.expectEqual(new_palette, p.original.*); try testing.expectEqual(new_palette[0], p.current[0]); try testing.expectEqual(custom_color, p.current[5]); try testing.expect(p.mask.isSet(5)); @@ -1081,7 +1191,8 @@ test "DynamicPalette: changeDefault preserves changes" { test "DynamicPalette: changeDefault with multiple changes" { const testing = std.testing; - var p: DynamicPalette = .init(default); + var p: DynamicPalette = .default; + defer p.deinit(testing.allocator); const red = RGB{ .r = 255, .g = 0, .b = 0 }; const green = RGB{ .r = 0, .g = 255, .b = 0 }; const blue = RGB{ .r = 0, .g = 0, .b = 255 }; @@ -1094,7 +1205,7 @@ test "DynamicPalette: changeDefault with multiple changes" { new_palette[0] = RGB{ .r = 50, .g = 50, .b = 50 }; new_palette[1] = RGB{ .r = 60, .g = 60, .b = 60 }; - p.changeDefault(new_palette); + try p.changeDefault(testing.allocator, new_palette); try testing.expectEqual(new_palette[0], p.current[0]); try testing.expectEqual(red, p.current[1]); diff --git a/src/terminal/snapshot/terminal.zig b/src/terminal/snapshot/terminal.zig index 11c9004bb..19305372a 100644 --- a/src/terminal/snapshot/terminal.zig +++ b/src/terminal/snapshot/terminal.zig @@ -761,7 +761,7 @@ pub const Header = struct { /// Allocator-owned semantic state decoded from one TERMINAL payload. /// -/// The palette and fixed header are values. `tabstops`, `pwd`, and `title` +/// The fixed header is a value. `tabstops`, `palette`, `pwd`, and `title` /// own allocations and are released by `deinit`. const DecodedPayload = struct { header: Header, @@ -772,6 +772,7 @@ const DecodedPayload = struct { fn deinit(self: *DecodedPayload, alloc: Allocator) void { self.tabstops.deinit(alloc); + self.palette.deinit(alloc); alloc.free(self.pwd); alloc.free(self.title); self.* = undefined; @@ -874,7 +875,8 @@ fn decodePayload( var override_mask: [32]u8 = undefined; try reader.readSliceAll(&override_mask); - var palette: terminal_color.DynamicPalette = .init(original); + var palette: terminal_color.DynamicPalette = try .init(alloc, original); + errdefer palette.deinit(alloc); for (0..256) |index| { const mask = @as(u8, 1) << @intCast(index % 8); if (override_mask[index / 8] & mask != 0) { @@ -992,6 +994,10 @@ pub fn decode( }); errdefer result.deinit(alloc); + // The terminal owns the decoded palette now, so the payload must not + // release it. + payload.palette = .default; + // Recreate the optional screen now so ScreenSet routing is complete before // its empty screens are replaced by the following SCREEN records. if (header.screen_count == 2) { @@ -1445,9 +1451,7 @@ test "TERMINAL payload round trip" { tabstops.set(test_header.columns - 1); // Use overrides at both ends of the palette to make ordering explicit. - var palette: terminal_color.DynamicPalette = .init( - terminal_color.default, - ); + var palette: terminal_color.DynamicPalette = .default; palette.set(0, .{ .r = 1, .g = 2, .b = 3 }); palette.set(255, .{ .r = 4, .g = 5, .b = 6 }); @@ -1512,9 +1516,7 @@ test "TERMINAL payload rejects noncanonical state" { 0, ); defer tabstops.deinit(testing.allocator); - var palette: terminal_color.DynamicPalette = .init( - terminal_color.default, - ); + var palette: terminal_color.DynamicPalette = .default; var encoded: [2048]u8 = undefined; var writer: std.Io.Writer = .fixed(&encoded); try testing.expectError( @@ -1557,9 +1559,7 @@ test "TERMINAL payload ignores tab-stop padding" { 0, ); defer tabstops.deinit(testing.allocator); - var palette: terminal_color.DynamicPalette = .init( - terminal_color.default, - ); + var palette: terminal_color.DynamicPalette = .default; palette.set(1, .{ .r = 1, .g = 2, .b = 3 }); var destination: std.Io.Writer.Allocating = .init(testing.allocator); diff --git a/src/termio/Termio.zig b/src/termio/Termio.zig index 7d9fd45ff..d08f2e5ff 100644 --- a/src/termio/Termio.zig +++ b/src/termio/Termio.zig @@ -270,7 +270,7 @@ pub fn init(self: *Termio, alloc: Allocator, opts: termio.Options) !void { const rgb = color.toTerminalRGB() orelse break :cursor .unset; break :cursor .init(rgb); }, - .palette = .init(opts.config.palette), + .palette = .default, }, .kitty_image_storage_limit = opts.config.image_storage_limit, .kitty_image_loading_limits = .allWithTempDir(global.tmpDirPath()), @@ -278,6 +278,10 @@ pub fn init(self: *Termio, alloc: Allocator, opts: termio.Options) !void { }); errdefer term.deinit(alloc); + // The default palette may be an allocator-owned copy, so it is set + // once the terminal owns its memory and can release it on deinit. + try term.colors.palette.changeDefault(alloc, opts.config.palette); + // Setup our terminal size in pixels for certain requests. term.width_px = term.cols * opts.size.cell.width; term.height_px = term.rows * opts.size.cell.height; @@ -463,8 +467,16 @@ pub fn changeConfig(self: *Termio, td: *ThreadData, config: *DerivedConfig) !voi // - command, working-directory: we never restart the underlying // process so we don't care or need to know about these. - // Update the default palette. - self.terminal.colors.palette.changeDefault(config.palette); + // Update the default palette. A config change must not fail here, so + // if we can't allocate the copy of the configured palette we fall back + // to the built-in default, which never allocates. + self.terminal.colors.palette.changeDefault( + self.alloc, + config.palette, + ) catch |err| { + log.warn("error changing default palette, using built-in default err={}", .{err}); + self.terminal.colors.palette.resetDefault(self.alloc); + }; self.terminal.flags.dirty.palette = true; // Update all our other colors