From bacf735dd69d0107aa6c51e9dd9185717ff39cdf Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Thu, 28 May 2026 06:38:45 -0700 Subject: [PATCH] render: make cell style snapshots allocation-free RenderCellStyle used pointer fields for foreground and background colors that pointed back into storage owned by the same struct. Reusing a single style value in a per-cell render loop still forced the destination to escape, producing heap allocations for heavily styled output. Store resolved colors as values with explicit presence booleans instead and make StyleInto fill that shape directly. The binding helper now returns its snapshot by value rather than writing through a Go pointer, so cgo does not force a temporary snapshot allocation on every call. --- render_state_cell.go | 125 ++++++++++++++++++-------------------- render_state_cell_test.go | 45 ++++++-------- 2 files changed, 78 insertions(+), 92 deletions(-) diff --git a/render_state_cell.go b/render_state_cell.go index dc2ade6..17c6c0d 100644 --- a/render_state_cell.go +++ b/render_state_cell.go @@ -26,61 +26,74 @@ typedef struct { bool inverse; } render_cell_style_snapshot; -static inline GhosttyResult render_cell_style_snapshot_get( - GhosttyRenderStateRowCells cells, - render_cell_style_snapshot* out -) { - if (out == NULL) return GHOSTTY_INVALID_VALUE; +typedef struct { + GhosttyResult result; + render_cell_style_snapshot style; +} render_cell_style_snapshot_result; - *out = (render_cell_style_snapshot){0}; +static inline render_cell_style_snapshot_result render_cell_style_snapshot_get( + GhosttyRenderStateRowCells cells +) { + render_cell_style_snapshot_result out = { + .result = GHOSTTY_SUCCESS, + .style = {0}, + }; GhosttyResult result = ghostty_render_state_row_cells_get( cells, GHOSTTY_RENDER_STATE_ROW_CELLS_DATA_HAS_STYLING, - &out->has_styling + &out.style.has_styling ); - if (result != GHOSTTY_SUCCESS) return result; + if (result != GHOSTTY_SUCCESS) { + out.result = result; + return out; + } - if (out->has_styling) { + if (out.style.has_styling) { GhosttyStyle style = GHOSTTY_INIT_SIZED(GhosttyStyle); result = ghostty_render_state_row_cells_get( cells, GHOSTTY_RENDER_STATE_ROW_CELLS_DATA_STYLE, &style ); - if (result != GHOSTTY_SUCCESS) return result; - - out->bold = style.bold; - out->faint = style.faint; - out->italic = style.italic; - out->underline = style.underline != GHOSTTY_SGR_UNDERLINE_NONE; - out->strikethrough = style.strikethrough; - out->inverse = style.inverse; + if (result != GHOSTTY_SUCCESS) { + out.result = result; + return out; + } + + out.style.bold = style.bold; + out.style.faint = style.faint; + out.style.italic = style.italic; + out.style.underline = style.underline != GHOSTTY_SGR_UNDERLINE_NONE; + out.style.strikethrough = style.strikethrough; + out.style.inverse = style.inverse; } result = ghostty_render_state_row_cells_get( cells, GHOSTTY_RENDER_STATE_ROW_CELLS_DATA_FG_COLOR, - &out->foreground + &out.style.foreground ); if (result == GHOSTTY_SUCCESS) { - out->has_foreground = true; + out.style.has_foreground = true; } else if (result != GHOSTTY_INVALID_VALUE) { - return result; + out.result = result; + return out; } result = ghostty_render_state_row_cells_get( cells, GHOSTTY_RENDER_STATE_ROW_CELLS_DATA_BG_COLOR, - &out->background + &out.style.background ); if (result == GHOSTTY_SUCCESS) { - out->has_background = true; + out.style.has_background = true; } else if (result != GHOSTTY_INVALID_VALUE) { - return result; + out.result = result; + return out; } - return GHOSTTY_SUCCESS; + return out; } */ import "C" @@ -136,20 +149,20 @@ const ( // render-state cell. It corresponds to the data returned by // ghostty_render_state_row_cells_get for style, foreground color, background // color, and has-styling, flattened into one Go value for hot render paths. -// -// Foreground and Background point at storage owned by this struct when set and -// are nil when the cell has no explicit/resolved color. Because those pointers -// target internal fields, callers should not copy a RenderCellStyle value after -// calling [RenderStateRowCells.ResolvedStyleInto]; pass the same value by -// pointer and reuse it instead. type RenderCellStyle struct { - // Foreground is the resolved foreground color, or nil when the cell has no - // explicit foreground and the renderer should use its default foreground. - Foreground *ColorRGB + // Foreground is the resolved foreground color when HasForeground is true. + Foreground ColorRGB + + // Background is the resolved background color when HasBackground is true. + Background ColorRGB - // Background is the resolved background color, or nil when the cell has no - // explicit background and the renderer should use its default background. - Background *ColorRGB + // HasForeground reports whether Foreground contains a resolved foreground + // color. When false, the renderer should use its default foreground. + HasForeground bool + + // HasBackground reports whether Background contains a resolved background + // color. When false, the renderer should use its default background. + HasBackground bool // HasStyling reports whether the cell has any explicit non-default styling. HasStyling bool @@ -171,9 +184,6 @@ type RenderCellStyle struct { // Inverse reports whether inverse video is set. Inverse bool - - foreground ColorRGB - background ColorRGB } // RenderStateRowCells iterates over cells in a render-state row. @@ -283,27 +293,26 @@ func (rc *RenderStateRowCells) Style() (*Style, error) { } // StyleInto fills dst with the reusable resolved style snapshot for the current -// cell. It is an alias for [RenderStateRowCells.ResolvedStyleInto]. -func (rc *RenderStateRowCells) StyleInto(dst *RenderCellStyle) error { - return rc.ResolvedStyleInto(dst) -} - -// ResolvedStyleInto fills dst with the current cell's resolved colors and text -// style flags. This is the allocation-reusing form of querying Style, FgColor, +// cell. This is the allocation-reusing form of querying Style, FgColor, // BgColor, and HasStyling separately: it performs one cgo transition, stores -// colors in dst-owned memory, and sets Foreground/Background to nil when the -// corresponding color is absent. -func (rc *RenderStateRowCells) ResolvedStyleInto(dst *RenderCellStyle) error { +// colors as values, and sets HasForeground/HasBackground when the corresponding +// color is present. +func (rc *RenderStateRowCells) StyleInto(dst *RenderCellStyle) error { if dst == nil { return errors.New("libghostty: nil RenderCellStyle") } - var snap C.render_cell_style_snapshot - if err := resultError(C.render_cell_style_snapshot_get(rc.ptr, &snap)); err != nil { + result := C.render_cell_style_snapshot_get(rc.ptr) + if err := resultError(result.result); err != nil { return err } + snap := result.style dst.HasStyling = bool(snap.has_styling) + dst.HasForeground = bool(snap.has_foreground) + dst.HasBackground = bool(snap.has_background) + dst.Foreground = ColorRGB{R: uint8(snap.foreground.r), G: uint8(snap.foreground.g), B: uint8(snap.foreground.b)} + dst.Background = ColorRGB{R: uint8(snap.background.r), G: uint8(snap.background.g), B: uint8(snap.background.b)} dst.Bold = bool(snap.bold) dst.Faint = bool(snap.faint) dst.Italic = bool(snap.italic) @@ -311,20 +320,6 @@ func (rc *RenderStateRowCells) ResolvedStyleInto(dst *RenderCellStyle) error { dst.Strikethrough = bool(snap.strikethrough) dst.Inverse = bool(snap.inverse) - if bool(snap.has_foreground) { - dst.foreground = ColorRGB{R: uint8(snap.foreground.r), G: uint8(snap.foreground.g), B: uint8(snap.foreground.b)} - dst.Foreground = &dst.foreground - } else { - dst.Foreground = nil - } - - if bool(snap.has_background) { - dst.background = ColorRGB{R: uint8(snap.background.r), G: uint8(snap.background.g), B: uint8(snap.background.b)} - dst.Background = &dst.background - } else { - dst.Background = nil - } - return nil } diff --git a/render_state_cell_test.go b/render_state_cell_test.go index 96b9b70..b0be6cd 100644 --- a/render_state_cell_test.go +++ b/render_state_cell_test.go @@ -336,7 +336,7 @@ func TestRenderStateRowCellsStyle(t *testing.T) { } } -func TestRenderStateRowCellsResolvedStyleInto(t *testing.T) { +func TestRenderStateRowCellsStyleInto(t *testing.T) { term, err := NewTerminal(WithSize(80, 24)) if err != nil { t.Fatal(err) @@ -387,55 +387,46 @@ func TestRenderStateRowCellsResolvedStyleInto(t *testing.T) { } var style RenderCellStyle - if err := rc.ResolvedStyleInto(&style); err != nil { + if err := rc.StyleInto(&style); err != nil { t.Fatal(err) } if !style.HasStyling { t.Fatal("expected first cell to have styling") } - if style.Foreground == nil || *style.Foreground != (ColorRGB{R: 255, G: 0, B: 0}) { - t.Fatalf("expected red foreground, got %#v", style.Foreground) + if !style.HasForeground || style.Foreground != (ColorRGB{R: 255, G: 0, B: 0}) { + t.Fatalf("expected red foreground, got %#v", style) } - if style.Background == nil || *style.Background != (ColorRGB{R: 0, G: 0, B: 255}) { - t.Fatalf("expected blue background, got %#v", style.Background) + if !style.HasBackground || style.Background != (ColorRGB{R: 0, G: 0, B: 255}) { + t.Fatalf("expected blue background, got %#v", style) } if !style.Bold || !style.Faint || !style.Italic || !style.Underline || !style.Strikethrough || !style.Inverse { t.Fatalf("expected all style flags to be set, got %+v", style) } - fgPtr := style.Foreground - bgPtr := style.Background + + allocs := testing.AllocsPerRun(1000, func() { + if err := rc.StyleInto(&style); err != nil { + t.Fatal(err) + } + }) + if allocs != 0 { + t.Fatalf("expected StyleInto to allocate zero times, got %v", allocs) + } if !rc.Next() { t.Fatal("expected second cell") } - if err := rc.ResolvedStyleInto(&style); err != nil { + if err := rc.StyleInto(&style); err != nil { t.Fatal(err) } if style.HasStyling { t.Fatal("expected second cell to have default styling") } - if style.Foreground != nil { - t.Fatalf("expected nil foreground for default-styled cell, got %+v", *style.Foreground) - } - if style.Background != nil { - t.Fatalf("expected nil background for default-styled cell, got %+v", *style.Background) + if style.HasForeground || style.HasBackground { + t.Fatalf("expected default style to have no colors, got %+v", style) } if style.Bold || style.Faint || style.Italic || style.Underline || style.Strikethrough || style.Inverse { t.Fatalf("expected all style flags to be cleared, got %+v", style) } - - if err := rc.Select(0); err != nil { - t.Fatal(err) - } - if err := rc.StyleInto(&style); err != nil { - t.Fatal(err) - } - if style.Foreground != fgPtr { - t.Fatal("expected foreground pointer storage to be reused") - } - if style.Background != bgPtr { - t.Fatal("expected background pointer storage to be reused") - } } func TestRenderStateRowCellsSelected(t *testing.T) { -- 2.51.2