mirror of
https://github.com/ghostty-org/ghostty.git
synced 2026-07-28 11:36:34 +00:00
unicode: switch to uucode grapheme break to (mostly) match unicode spec (#9680)
This PR builds on https://github.com/ghostty-org/ghostty/pull/9678 ~so the diff from there is included here (it's not possible to stack PRs unless it's a PR against my own fork)--review that one first!~ This PR updates the `graphemeBreak` calculation to use `uucode`'s `computeGraphemeBreakNoControl`, which has [tests in uucode](215ff09730/src/x/grapheme.zig (L753)) that confirm it passes the `GraphemeBreakTest.txt` (minus some exceptions). Note that the `grapheme_break` (and `grapheme_break_no_control`) property in `uucode` incorporates `emoji_modifier` and `emoji_modifier_base`, diverging from UAX #29 but matching UTS #51. See [this comment in uucode](215ff09730/src/grapheme.zig (L420-L434)) for details. The `grapheme_break_no_control` property and `computeGraphemeBreakNoControl` both assume `control`, `cr`, and `lf` have been filtered out, matching the current grapheme break logic in Ghostty. This PR keeps the `Precompute.data` logic mostly equivalent, since the `uucode` `precomputedGraphemeBreak` lacks benchmarks in the `uucode` repository (it was benchmarked in [the original PR adding `uucode` to Ghostty](https://github.com/ghostty-org/ghostty/pull/8757)). Note however, that due to `grapheme_break` being one bit larger than `grapheme_boundary_class` and the new `BreakState` also being one bit larger, the state jumps up by a factor of 8 (u10 -> u13), to 8KB. ## Benchmarks ~I benchmarked the old `main` version versus this PR for `+grapheme-break` and surprisingly this PR is 2% faster (?). Looking at the assembly though, I'm thinking something else might be causing that. Once I get to the bottom of that I'll remove the below TODO and include the benchmark results here.~ When seeing the speedup with `data.txt` and maybe a tiny speedup on English wiki, I was surprised given the 1KB -> 8KB tables. Here's what AI said when I asked it to inspect the assembly: https://ampcode.com/threads/T-979b1743-19e7-47c9-8074-9778b4b2a61e, and here's what it said when I asked it to predict the faster version: https://ampcode.com/threads/T-3291dcd3-7a21-4d24-a192-7b3f6e18cd31 It looks like two loads got reordered and that put the load that depended on stage1 -> stage2 -> stage3 second, "hiding memory latency". So that makes the new one faster when looking up the `grapheme_break` property. These gains go away with the Japanese and Arabic benchmarks, which spend more time processing utf8, and may even have more grapheme clusters too. ### with data.txt (200 MB ghostty-gen random utf8) <img width="1822" height="464" alt="CleanShot 2025-11-26 at 08 42 03@2x" src="https://github.com/user-attachments/assets/56d4ee98-21db-4eab-93ab-a0463a653883" /> ### with English wiki dump <img width="2012" height="506" alt="CleanShot 2025-11-26 at 08 43 15@2x" src="https://github.com/user-attachments/assets/230fbfb7-272d-4a2a-93e7-7268962a9814" /> ### with Japanese wiki dump <img width="2008" height="518" alt="CleanShot 2025-11-26 at 08 43 49@2x" src="https://github.com/user-attachments/assets/edb408c8-a604-4a8f-bd5b-80f19e3d65ee" /> ### with Arabic wiki dump <img width="2010" height="512" alt="CleanShot 2025-11-26 at 08 44 25@2x" src="https://github.com/user-attachments/assets/81a29ac8-0586-4e82-8276-d7fa90c31c90" /> TODO: * [x] Take a closer look at the assembly and understand why this PR (8 KB vs 1 KB table) is faster on my machine. * [x] _(**edit**: checking this off because it seems unnecessary)_ If this turns out to actually be unacceptably slower, one possibility is to switch to `uucode`'s `precomputedGraphemeBreak` which uses a 1445 byte table since it uses a dense table (indexed using multiplication instead of bitCast, though, which did show up in the initial benchmarks from https://github.com/ghostty-org/ghostty/pull/8757 a small amount.) AI was used in some of the uucode changes in https://github.com/ghostty-org/ghostty/pull/9678 (Amp--primarily for tests), but everything was carefully vetted and much of it done by hand. This PR was made without AI with the exception of consulting AI about whether the "Prepend + ASCII" scenario is common (hopefully it's right about that being uncommon).
This commit is contained in:
@@ -9,6 +9,7 @@ const assert = @import("../quirks.zig").inlineAssert;
|
||||
const testing = std.testing;
|
||||
const Allocator = std.mem.Allocator;
|
||||
const unicode = @import("../unicode/main.zig");
|
||||
const uucode = @import("uucode");
|
||||
|
||||
const ansi = @import("ansi.zig");
|
||||
const modespkg = @import("modes.zig");
|
||||
@@ -361,7 +362,7 @@ pub fn print(self: *Terminal, c: u21) !void {
|
||||
if (prev.cell.codepoint() == 0) break :grapheme;
|
||||
|
||||
const grapheme_break = brk: {
|
||||
var state: unicode.GraphemeBreakState = .{};
|
||||
var state: uucode.grapheme.BreakState = .default;
|
||||
var cp1: u21 = prev.cell.content.codepoint;
|
||||
if (prev.cell.hasGrapheme()) {
|
||||
const cps = self.screens.active.cursor.page_pin.node.data.lookupGrapheme(prev.cell).?;
|
||||
@@ -512,7 +513,7 @@ pub fn print(self: *Terminal, c: u21) !void {
|
||||
// If this is a emoji variation selector, prev must be an emoji
|
||||
if (c == 0xFE0F or c == 0xFE0E) {
|
||||
const prev_props = unicode.table.get(prev.content.codepoint);
|
||||
const emoji = prev_props.grapheme_boundary_class == .extended_pictographic;
|
||||
const emoji = prev_props.grapheme_break == .extended_pictographic;
|
||||
if (!emoji) return;
|
||||
}
|
||||
|
||||
@@ -3996,6 +3997,53 @@ test "Terminal: overwrite multicodepoint grapheme tail clears grapheme data" {
|
||||
try testing.expectEqual(@as(usize, 0), page.graphemeCount());
|
||||
}
|
||||
|
||||
test "Terminal: print breaks valid grapheme cluster with Prepend + ASCII for speed" {
|
||||
const alloc = testing.allocator;
|
||||
var t = try init(alloc, .{ .rows = 5, .cols = 5 });
|
||||
defer t.deinit(alloc);
|
||||
t.modes.set(.grapheme_cluster, true);
|
||||
|
||||
// Make sure we're not at cursor.x == 0 for the next char.
|
||||
try t.print('_');
|
||||
|
||||
// U+0600 ARABIC NUMBER SIGN (Prepend)
|
||||
try t.print(0x0600);
|
||||
try t.print('1');
|
||||
|
||||
// We should have 3 cells taken up, each narrow. Note that this is
|
||||
// **incorrect** grapheme break behavior, since a Prepend code point should
|
||||
// not break with the one following it per UAX #29 GB9b. However, as an
|
||||
// optimization we assume a grapheme break when c <= 255, and note that
|
||||
// this deviation only affects these very uncommon scenarios (e.g. the
|
||||
// Arabic number sign should precede Arabic-script digits).
|
||||
try testing.expectEqual(@as(usize, 0), t.screens.active.cursor.y);
|
||||
try testing.expectEqual(@as(usize, 3), t.screens.active.cursor.x);
|
||||
// This is what we'd expect if we did break correctly:
|
||||
//try testing.expectEqual(@as(usize, 2), t.screens.active.cursor.x);
|
||||
|
||||
// Assert various properties about our screen to verify
|
||||
// we have all expected cells.
|
||||
{
|
||||
const list_cell = t.screens.active.pages.getCell(.{ .screen = .{ .x = 1, .y = 0 } }).?;
|
||||
const cell = list_cell.cell;
|
||||
try testing.expectEqual(@as(u21, 0x0600), cell.content.codepoint);
|
||||
try testing.expect(!cell.hasGrapheme());
|
||||
// This is what we'd expect if we did break correctly:
|
||||
//try testing.expect(cell.hasGrapheme());
|
||||
//try testing.expectEqualSlices(u21, &.{'1'}, list_cell.node.data.lookupGrapheme(cell).?);
|
||||
try testing.expectEqual(Cell.Wide.narrow, cell.wide);
|
||||
}
|
||||
{
|
||||
const list_cell = t.screens.active.pages.getCell(.{ .screen = .{ .x = 2, .y = 0 } }).?;
|
||||
const cell = list_cell.cell;
|
||||
try testing.expectEqual(@as(u21, '1'), cell.content.codepoint);
|
||||
// This is what we'd expect if we did break correctly:
|
||||
//try testing.expectEqual(@as(u21, 0), cell.content.codepoint);
|
||||
try testing.expect(!cell.hasGrapheme());
|
||||
try testing.expectEqual(Cell.Wide.narrow, cell.wide);
|
||||
}
|
||||
}
|
||||
|
||||
test "Terminal: print writes to bottom if scrolled" {
|
||||
var t = try init(testing.allocator, .{ .cols = 5, .rows = 2 });
|
||||
defer t.deinit(testing.allocator);
|
||||
|
||||
Reference in New Issue
Block a user