perf: cut font size/DPI change cost and give each window its own font grid - #55
Merged
Conversation
`FontGrid` carries `px_size` as an object-scoped field with no size in any
cache key, so a size or scale change is only expressible as whole-object
reconstruction via `FontGrid::new` — five call sites in `noa-app`, two
app-wide grids, all on the winit main thread.
That cost had never been measured here: `docs/performance-measurements.md`
has no font or DPI workload, `performance-resource-optimization-matrix.md`
states its own figures are "estimates ... not measured values", and
`docs/specs/theme-settings-ui.md` debounces the font-size scrub to avoid a
cost it never quantified. This is the repository's first font benchmark.
Phases, each a median over N reps in one binary:
A load_font_stack system font discovery
A1 load_primary_font first stage
A2 load_font_stack_with_... second stage
B with_stack metrics + empty atlases, measured DIRECTLY
C FontGrid::new A + B
D prewarm shape_run + raster_shaped over ASCII x 4
styles, through the real render path,
split by stage and by `thicken` share
Baseline on an M-series Mac: `load_font_stack` 19.2 ms, `with_stack` 12 us
— 99.9% of `FontGrid::new` is font discovery. Sub-profiling puts ~16 ms of
that inside one `all_families()` call. Prewarm is 25.3 us/glyph and
essentially ppem-independent (25.4 us at 13px, 26.2 us at 28px); of it,
`raster_shaped` is 89.7% and the `thicken` dilation noa owns is 5.7%. One
size step therefore costs ~57 ms of main-thread CPU, and the raster cannot
be made meaningfully cheaper — ~94% of it is swash's scale+hint+render.
Replaying a warm grid is 0.05 ms, ~200x less, so the lever is not doing the
work rather than doing it faster.
Two measurement rules this had to learn, both of which flattered the result
until they were fixed:
- `B` is measured directly, never as `C - A`. At ~20 ms with ms-scale
jitter that subtraction is pure noise and can go negative.
- Absolute baselines swing with machine load, so only interleaved
same-binary medians compare.
Does not cover the GPU half: texture upload, bind-group rebuild, relayout.
The CPU-half benchmark explicitly does not cover what happens after a
`FontGrid` rebuild: `noa-app` calls `Renderer::sync_atlas` for every window,
which recreates the glyph atlas texture, re-uploads it in full, and rebuilds
every stale pane bind group. This measures that.
Needs a real adapter and exits with a message without one — the sandbox has
no Metal device, which is also why the `pipeline` suite's tests silently take
their skip branch there and must be run unsandboxed to mean anything.
Results at 14 ppem, 512x512 R8 mask atlas (256 KiB), 15 reps, two passes:
S0 empty submit + poll 1.28 ms fixed
S1 sync after rebuild 0.20 ms CPU-side 1.52 ms drained
S2 steady-state sync 0.000 ms -
S3 write_texture alone 0.06 ms CPU-side 1.33 ms drained
upload work above the fixed round trip: S1 0.24 ms, S3 0.05 ms
The GPU half is negligible: ~0.2 ms per window against the CPU half's tens
of milliseconds. Steady-state sync is free, as it must be. In the running app
the upload rides along with the frame's own submit, so it does not even pay
the 1.28 ms round trip separately — which is why S0 is reported at all.
Two measurement traps this had to avoid, both of which flattered the result
before they were fixed:
- `write_texture` only stages. The first version polled without submitting,
so the "+drain" column for S1 measured an upload that had not been handed
to the driver, and read 0.16 ms instead of 1.52 ms.
- Every drained figure carries a fixed ~1.28 ms submit+poll round trip that
has nothing to do with the atlas. S0 exists to subtract it, otherwise a
256 KiB upload appears to cost 1.33 ms when it costs 0.05 ms.
Sub-profiling `load_font_stack` found the cost is not spread across
discovery — it is one call:
src=0.000 styles=1.1 emoji=0.2 names=16.1(n=14) probe=2.8 ms
`source.all_families()` inside `nerd_font_fallback_family_names` walks every
installed family through CoreText and costs ~16 ms on its own: 78% of a
whole `load_font_stack`. On a machine with no Nerd Font family installed it
returns nothing usable — `n` stays at the 14 hardcoded candidates.
The answer depends only on which fonts are installed. It depends on neither
`FontConfig` nor the pixel size, yet it was recomputed on every font-size and
DPI change, twice per step (terminal + sidebar grid), on the winit main
thread — the window-owning thread that also presents.
Resolve it once per process behind a `OnceLock`. `FontStack` is still built
fresh on every call, so `FaceId` identity, `push_dynamic_fallback` mutation
and every grid invariant are untouched; sharing one `Arc<FontStack>` between
grids would have broken all three and was rejected for that reason.
Measured by interleaved `git stash` on/off, same tree, same path, 2 passes,
15 reps, medians:
before after
load_font_stack 20.7 / 20.7 3.7 / 4.1 ms
FontGrid::new 21.1 / 20.8 2.8 / 3.0 ms -86%
one size step 63.3 / 62.5 26.5 / 26.9 ms -58%, -36 ms of stall
At 60 Hz that is 3.8 frames of main-thread stall reduced to 1.6.
Declared trade-off: a Nerd Font installed while noa is running is not picked
up until restart. Ghostty and kitty place the same requirement on newly
installed fonts, and it is bounded here because Symbols Nerd Font Mono ships
inside the binary — PUA icon coverage never depended on a system install.
Two regression tests. `nerd_font_family_names_are_memoized_once_per_process`
asserts the memo is one allocation, not merely equal content, and is
mutation-verified: returning an empty slice after the first call fails it.
`repeated_stack_loads_resolve_the_same_faces` covers the end-to-end
size-change invariant, and its doc comment records that this same mutation
does NOT fail it on a machine with no Nerd Font installed — verified rather
than assumed.
Prerequisite for per-window font grids: every window's `Renderer` shared one
set of glyph atlas textures per format, which is correct only because the
terminal font was app-wide. Giving windows their own pixel size would have
two of them writing one texture set while both renderers' row caches still
hold concrete coordinates into it — corruption, not just repeated uploads.
Four changes, each load-bearing:
- `GlyphAtlasCache` is keyed by `(format, ppem)` rather than format alone,
quantized to 1/64 px to agree with `noa-app`'s font map. Bounded at 8
live sets; evicting one a renderer still holds is wasteful, not unsound,
because the `Arc` keeps its textures alive.
- `SharedGlyphAtlases` carries a process-unique id, and pane bind-group
staleness is now `(atlas_set_id, texture_generation)` rather than
generation alone. This is the subtle half: every set starts its
generation at zero, so switching a renderer between two sets compared
0 == 0 and skipped the rebuild, leaving panes sampling the previous
set's textures.
- `Renderer::rebind_glyph_atlases` points a live renderer at a different
set, dropping the per-row instance caches (their atlas coordinates mean
something else now) and refreshing bind groups. Window renderers are
built once at `lifecycle.rs` and never rebuilt, so without this there was
no way to follow a window's scale change at all. Idempotent, so callers
may call it per frame — which is what the chrome renderers do, rather
than folding the size into a stale key that a later field can outgrow.
- A set now knows the pixel size it belongs to, and `sync` asserts the grid
it is handed matches. Keying the *cache* safely still left the *call*
unguarded, and an unguarded cross-size sync is silent corruption; it is a
loud panic in any debug or test build now. That assertion immediately
caught a test of this suite's own that encoded the pre-per-window
contract, which is rewritten to exercise the backwards-identity property
within one size — what it was really guarding.
`FontGrid::px_size` is exposed here because the atlas key is derived from it;
it lands with this commit rather than the next so this one builds alone.
Four headless-GPU regressions, all mutation-verified against the code they
guard and all failing only where they should. Under format-only keying,
`glyph_atlas_cache_is_keyed_by_pixel_size` and
`a_second_font_size_does_not_corrupt_the_first_renderers_atlas` fail while
the four pre-existing shared-atlas tests keep passing — which is the evidence
that the suite did not cover this hazard. The corruption test asserts on
read-back pixels and redraws the first renderer with a bare `draw` — no
rebuild, no re-sync — because a rebuild would re-upload the first atlas and
hide the window being tested.
No behavior change yet: the app still uses one app-wide font, so every lookup
resolves to a single ppem and a single set.
The terminal `FontGrid` was app-wide and rebuilt at whichever window last
reported a scale change, so on a mixed-DPI setup every other window kept
rasterizing at a size that was not its own — the `#TODO(agent)` in
`on_scale_factor_changed`, removed here. It was also discarded outright on
every size change, re-rasterizing every glyph: ~13.5 ms of main-thread CPU
per grid, 90% of it swash's scale+hint+render, which noa does not own and
cannot make cheaper. Replaying an already-populated grid is 0.05 ms.
`GpuState` holds a map of live grids keyed by pixel size. Windows carry
`font_px` and look theirs up by it, so windows sharing a scale share a grid
and the common case still allocates exactly one; a size already visited comes
back instead of being rebuilt, so 14 -> 15 -> 14, or dragging a window between
a 1x and a 2x display, re-rasterizes nothing. A scale change ensures a grid
for that ONE window, rebinds its renderer to the atlas set for the new size,
and leaves every other window untouched. Bounded at 6 grids per role,
evicting the least-recently-used and never the primary.
Every metrics read that feeds layout or hit-testing goes through the window's
own grid — cursor position, Quick Look, wheel scrolling, IME placement, split
sizing, tab layout, and the scratch terminal's window measurement (which
takes its anchor's size, since it opens on the anchor's display). Converting
those mechanically to "any grid" would have quietly reintroduced the defect
class this change exists to remove.
The same holds for chrome. Modal cards, the sidebar band and overview labels
rasterize through the window they are drawn into, not through the primary,
and each shared renderer is rebound to that size's atlas set on every call.
The one remaining `primary()` read is the metrics probe taken before any
window exists.
Four invariants are enforced by construction rather than by inspection,
because each was violated at least once while this was written:
- `get` and `get_mut` resolve identically, including on a miss. `get_mut`
used to build, which `get` cannot — so a caller picked its atlas set from
the primary and rasterized a freshly built grid of the requested size,
syncing one pixel size into another size's textures. Residency is
`ensure`'s job, off the draw path, and every live window's size is
ensured before the next frame.
- Promotion is not separable from adoption: `ensure_primary` both builds
and promotes, so the app-wide readers cannot be left on a stale size.
- A config change is transactional. `prepare_config` builds and touches
nothing; `install_config` commits. Installing into one map and failing on
the next left the app's configuration and its actual fonts disagreeing,
since the caller reverts `AppConfig` on failure.
- A resident size is reused rather than rebuilt. `Ctrl +/-` called
`FontGrid::new` unconditionally and evicted the warm grid under the same
key, so an interactive scrub paid the full re-rasterization every step —
the exact cost this map exists to avoid, defeated by its own caller.
**Verification note.** All eight modal-card call sites are
`#[cfg(not(target_os = "macos"))]`, and so is their re-export, so `cargo
check` on macOS does not compile them at all: a signature change here is
green on macOS while the non-macOS build fails. They were type-checked by
temporarily rewriting both `cfg` attributes to `cfg(all())`, compiling, and
reverting. A macOS-only green build is not evidence for that code.
Not covered by any automated check: the mixed-DPI behaviour itself. The
payoff needs two displays at different scale factors and has only been
reasoned about, never seen.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02b34f58ef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+176
to
+178
| let mut next = fresh_font(); | ||
| let t = Instant::now(); | ||
| renderer.sync_atlas(&device, &queue, &mut next); |
There was a problem hiding this comment.
fresh_font() always constructs another 14-ppem grid, so this loop changes only the atlas identity, not the font size or DPI. SharedAtlasTexture::sync recreates textures only when their dimensions change, while the new production path switches to a ppem-keyed atlas set through rebind_glyph_atlases; consequently S1 measures a same-size upload and does not include the texture/bind-group transition it claims to benchmark. Alternate between distinct ppem values and perform the corresponding atlas-cache lookup/rebind before reporting these timings as the GPU cost of a size change.
Useful? React with 👍 / 👎.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A font-size or DPI change cost ~63 ms of main-thread CPU — the window-owning
thread that also presents — and that cost had never been measured in this repo.
The terminal
FontGridwas also app-wide and rebuilt at whichever window lastreported a scale change, so on a mixed-DPI setup every other window kept
rasterizing at a size that was not its own (the
#TODO(agent)inon_scale_factor_changed, removed here). This measures the cost, cuts it, andgives each window the grid for its own scale factor.
Changes
docs/performance-resource-optimization-matrix.mdpreviously stated its ownfigures were "estimates … not measured values".
all_families()walks every installed family through CoreText and costs ~16 ms — 78% of a
whole
load_font_stack— for an answer that depends only on which fonts areinstalled, not on config or pixel size.
FontGrid::new20.9 → 2.9 ms(−86%); one size step 62.9 → 26.7 ms. Trade-off: a Nerd Font installed
while noa is running needs a restart, as it does in Ghostty and kitty, and
bounded because Symbols Nerd Font Mono ships in the binary.
Renderer::rebind_glyph_atlases.Prerequisite for the next item: two windows at different scales writing one
texture set, under coordinates both renderers' row caches still hold, is
corruption rather than churn.
font_pxand look their grid up byit; windows sharing a scale share a grid, so the common case still allocates
exactly one. A size already visited comes back instead of being rebuilt —
replaying a warm grid is 0.05 ms against 10.2 ms, so
14 → 15 → 14, ordragging a window between displays, re-rasterizes nothing.
The raster itself cannot be made cheaper: ~94% of it is swash's
scale+hint+render, and the
thickendilation noa owns is 5.7%. The lever isnot doing the work, not doing it faster.
Test plan
cargo test --workspacegreen, including 39 headless-GPU pipeline tests rununsandboxed on a real Metal adapter. Without an adapter those tests take a
skip branch, so a sandboxed pass proves nothing about them.
cargo fmt --all -- --checkclean.code it guards is reverted, and to be the only failure.
#[cfg(not(target_os = "macos"))], as istheir re-export, so
cargo checkon macOS does not compile them at all.Type-checked by temporarily rewriting both
cfgattributes tocfg(all()).Risk
Medium-High. The renderer's shared-atlas ownership contract changed, and the
app-side change touches 93 call sites across 15 files.
Two gaps no automated check here closes:
needs two displays at different scale factors and has only been reasoned
about, never seen.
Reviewing the per-window commit: read it by invariant, not by file — its 18
files are one change, and it cannot be split without leaving an intermediate
that is broken under mixed DPI. Its commit body names four invariants now
enforced by construction, each because it was violated at least once while this
was written: read/write lookup symmetry, promotion inseparable from adoption,
transactional config install, and reusing a resident size rather than
rebuilding it.
Rollback is a plain revert of the merge; nothing here migrates data or changes
a config format.