perf(chrome): replay themed dialog frames, the menu bar and menu titles from a pixel cache - #1421
Conversation
|
Per-step CPU time, re-measured with the machine idle (3 in Three headless replay, 200 M guest instructions, 3 pairs each, identical ticks and framebuffer hash on every pair; each step against the previous one in the order the fixes were built):
All four together vs rebased #1220 (5 pairs, both workloads): 3 in Three −60.7% instructions / −53.0% CPU time; SimCity 2000 −39.1% / −30.8%. Note on the first row: removing the per-dispatch clone alone lowers the instruction count but costs CPU time (the borrowed check still walks the whole range list, now without the copy that had just pulled it into cache). The sorted-ranges commit removes that walk, so #1419 is measured as the pair; a run of #1419's two commits on their own is queued and will be posted here. |
benletchford
left a comment
There was a problem hiding this comment.
The rendered-pixel cache has a reproducible invalidation bug. install_standard_depth_clut rewrites the main colour table in place and sets its seed to the depth, so a colour-to-grayscale SetDepth change can keep the screen tuple, table pointer, and seed unchanged while replacing all RGB entries.
I tested this on this PR head: select colour 8 bpp, draw the themed menu bar to warm the cache, select grayscale 8 bpp (SetDepth: whichFlags=1, flags=0), draw again, and compare with a fresh rendering after clearing theme_chrome_cache. The cached and fresh framebuffers differ. This is a supported mode change, not a guest violating the colour-table protocol.
The key also omits the fallback device_clut: when no main-device table is available, theme_pixel_index reads that host palette, but the cache records only ctab: None.
Please cover these paths with reliable invalidation (or decline to cache when the dependency cannot be tracked), and add cached-versus-fresh regressions for both cases. The performance improvement is worthwhile, but the current implementation can keep stale chrome colours, so I am leaving this open with changes requested.
…tSeed Review finding (benletchford#1421): install_standard_depth_clut rewrites the main colour table in place and stamps ctSeed with the depth, so a colour-to- grayscale SetDepth at the same depth kept the cache key while every entry changed, and the key ignored the host-side table that colour resolution falls back to when no device table exists. Key cached chrome on the colour-environment digest (depth, the device table's pointer and every entry, and the host-side table) instead of the seed. Regression tests draw the themed menu bar, switch colour to grayscale at 8 bpp with the seed unchanged, and compare the cached replay against a fresh rendering; a second test does the same through the host-table fallback. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UqCuD9vsGeeij5DdYt3vcK
c2ece6d to
cc70b72
Compare
|
Thanks, both findings were real and your repro was exact. Fixed and rebased:
Rebased onto current Suite on the pushed head: 5,069 pass. One failure, |
cc70b72 to
7104fc9
Compare
|
Follow-up, replacing the digest key after measuring it on SimCity 2000: that game animates palette entries many times a second, so a whole-environment key re-rendered the chrome nearly every frame (13.5% of the run in chrome work, worse than before the fix). Pushed a rebuilt branch on top of #1423's mirror: commit 1 is the chrome cache, commit 2 records, per cached rendering, the colours it used and the indices they resolved to, and replays only while each still resolves to the same index through the mirror. Your two repros pass unchanged, and a new test checks that a palette change touching only unused entries keeps the rendering while one touching a used colour re-renders identically to a fresh draw. Commit 2 also stops Numbers against current Suite on the pushed head: 5,067 pass; the one failure is the |
3cdd254 to
6782f46
Compare
|
Resolved the current-master conflict by retaining both required memory imports. #1423 has now landed, so I rebased this branch to drop that prerequisite and retain only the theme-cache delta. The branch is now one conventional commit; new-head CI is running. The resulting source tree is identical to the locally tested combined palette/theme tree. Historical benchmark percentages remain historical, not a current-master performance rerun. |
|
Fresh combined validation with the current palette mirror, sound-finalization, simulated-time headless, and theme-cache changes: 5,437 library tests passed (3 ignored), 98 macOS desktop tests passed (1 ignored), including the Metal minification test, and wasm32-unknown-unknown library check passed. The complete source diff is whitespace-clean. Waiting for the refreshed PR CI showcase checks before merging; no new performance percentage is claimed. |
Summary
Cache resolved theme artwork for dialog frames, menu-bar backgrounds and
menu-title backgrounds instead of rebuilding their
ThemeBitmaps everychrome redraw. The cache holds up to 24 entries with least-recently-used
replacement. It is used by the non-classic themes.
September 8 refresh: this branch incorporates upstream
b5f4f083(0.39.1)through the refreshed #1423 branch, plus a new retained-text equivalence test.
It remains stacked on #1423; the diff includes that mirror's commits until it
merges. Fresh CPU acceptance on this upstream is still required.
How this fits with the other performance work
The font/retained-text overhaul, Systemless #1524, merged in 0.39.0. The effect sizes below are SC2K process CPU time for the stated replay, not GUI Activity Monitor percentages or a cross-workload ranking.
Draft #1598: retained-pixel hashing is stacked on #1597, not included in any of the five PRs above. It measured about31.2% additional SC2K CPU reduction on0.39.0 and31.6–40.4% on the refreshed0.39.1 stack; both are after #1524. Clock/load variation means that wider range is not evidence that0.39.1 made the patch better. EV is not cleared: its unrestricted current pair used2.369% more CPU; a severely throttled reverse pair is excluded. The PR remains draft for that check. A separate local bulk retained-fill prototype has no measured effect size yet. Sequential reductions apply to remaining work, not by adding percentages; there is no directly measured all-PR total.
What the text-rendering overhaul changed
#1524 added CPU-side retained high-resolution glyph coverage alongside guest framebuffer bytes. Glyph masks are cached: the new hot work is repeatedly erasing/repainting their pixels and updating subpixel, palette and snapshot state, not necessarily rasterizing font outlines again. GPU presentation does not remove this bookkeeping.
That changes what each optimization can save. #1421 avoids rebuilding themed background artwork, but replay still performs observed pixel writes and actual title glyphs are drawn separately. #1423 makes colour resolution cheaper, not retained-pixel bookkeeping. #1597 removes whole redundant paint passes, so it also avoids their new text cost. #1598 and the separate local bulk-fill work target that remaining per-pixel cost directly.
We have not run a matched before/after-#1524 experiment for the older PRs. Their percentage changes therefore cannot yet be quantified; the old numbers must not be ranked against the 41%/31% post-overhaul results as if they shared a baseline. More rendering CPU can also dilute a JIT optimization's percentage without making the JIT optimization itself slower. The mechanism explains why priorities changed, but is not proof of a particular slowdown or speedup caused by #1524 alone.
Theme is another comparison boundary: the 41% and 31% post-overhaul replays use
classic-system7. #1421's non-classic theme-artwork cache is bypassed in that path, so it does not add its historical 20.8% saving to those runs. The old5f21681CLI default wassystemless-default; the current CLI default isclassic-system7. Changes in theme, scheduling and retained rendering must be separated before attributing a changed effect size specifically to #1524.What is cached, and what is not
Keys cover artwork kind, rectangles/state, theme, screen geometry and depth.
Each cached rendering also remembers the colours it used and their resolved
device indices. Replay is allowed only if those colours still resolve to the
same indices through #1423's current device-table mirror.
This is not
ctSeed-only validation. Same-seed table rewrites, depth changesand the host CLUT fallback are checked. A palette change unrelated to the used
colours need not force a new bitmap rendering.
Title glyphs are not part of this cache.
draw_theme_menu_title_chromedraws the title's themed background; actual font text is painted separately.
Both fresh artwork and cached pixels go through
fb_set_pixel_index, preservingthe bus's write observers and retained-text erases, including same-value writes.
Caching device indices does not authorize bypassing those observers.
fill_theme_rectalso resolves its colour through the mirror and requestsrow fills. With current retained presentation enabled, those requests still
fall back to observed per-byte writes; this PR alone does not remove that
observer cost. A separate bulk-observer prototype is being evaluated locally.
Rejected approaches and motivation
The original seed-keyed validation missed same-seed palette changes. A later
whole-environment digest cache key was correct but caused frequent re-rendering
under palette animation. The current used-colour validation avoids that
unnecessary invalidation while checking every colour the artwork depends on.
Historical profiling during the #1362 re-baselining found dialog-frame bitmap
blits at 7.5% self time and menu-bar/title-background blits at another 4.1% in
a 3 in Three replay. These are old workload-specific samples, not current
profile shares or predictions of the remaining opportunity.
Historical measurements — not current-head acceptance
Five interleaved pairs against
5f21681(then master, already carrying #1220,#1419 and #1420), including #1423, using the older instruction-quota
headless runner. These are not an isolated measurement of this cache or the
corrected fixed-time runner in #1596, and not GUI CPU/FPS measurements.
The old comparison against the preceding digest-keyed version reported SC2K
−18.7% instructions / −17.1% CPU and a flat result on 3 in Three, consistent
with palette animation affecting invalidation. It does not establish the
size of that effect after upstream's later presentation/scheduling changes.
Equal ticks and logical framebuffer hashes were reported for those pairs;
checkpoint equality is not proof of all transient or retained high-resolution
output. The original logs and rejected versions remain retained locally.
Tests and review status
Existing tests compare cached and fresh screen bytes, unused same-seed table
rewrites, active/inactive cache entries, colour-to-grayscale changes with an
unchanged seed, and the host-table fallback with
MainDevicenil. An unusedentry rewrite should retain the rendering, not force a fresh one.
The new regression paints retained outline text across dialog-frame, menu-bar
and menu-title artwork, then compares cached replay with a forced fresh render:
guest bytes and saved subpixel detail, resolved RGB and subsequent title
painting must match. It covers the distinction between cached backgrounds and
separately painted glyphs. CI on
3a8fcc16passed Linux build/tests, classic/PowerPC showcase assertions, headless/package,
licenses and the nonblocking formatting/clippy job. The test-only follow-up
3cdd2547strengthens the fixture precondition: its saved snapshot must containactual retained coverage, not merely grey logical pixels. CI on that final
head also
passes all four jobs, including the strengthened regression and classic/PowerPC
showcase assertions.
Earlier review requested changes; the fixes have not yet received approval.
No new performance percentage is
claimed until the final implementation is measured on a current, equal-time
baseline.
Original implementation/review history: Claude session.