Plan the performance work the baseline points at
New "The plan" subsection under Performance in TODO.md: nine entries in blast-radius order. Six are next (string-pool free list, text texture ring cache, non-raising sprite lookup, hot-path log demotion, the tileset scan fix with its two latent draw bugs, the draw_world layer loop); three are on record but unscheduled (uniform-grid broad phase behind target 12, the realistic map fixture for target 13, the footprint refactor for 14 and 15). Co-Authored-By: Claude Code (Claude Fable 5, claude-fable-5) <noreply@anthropic.com>
This commit is contained in:
115
TODO.md
115
TODO.md
@@ -1238,6 +1238,121 @@ Notes on the ones worth arguing about:
|
||||
layers, several tilesets — would make target 13 measurable and would probably
|
||||
find something.
|
||||
|
||||
### The plan
|
||||
|
||||
The order below is the order the work should happen in, set by blast radius
|
||||
per frame and per byte, not by how interesting the change is. Items 1 through
|
||||
6 are the next changes; 7 through 9 are on record so the decision that
|
||||
unblocks each is made with the numbers in front of it. The engine comparison
|
||||
that informs several of these is in `PERFORMANCE.md` under **How other
|
||||
engines spend the same frame**.
|
||||
|
||||
1. **String pool free-list index — targets 6 and 7, one change.** A next-free
|
||||
hint per pool in `src/heap.c` (the five scans are `src/heap.c:52-116`)
|
||||
takes the last-slot claim from 250.9 ns to constant time, and it lands
|
||||
before anyone raises `AKGL_MAX_HEAP_*`, because today's cost grows with the
|
||||
ceiling rather than the usage. Fold in item 8 under **Known and still
|
||||
open** — `akgl_heap_next_string` is the only acquire that takes a
|
||||
reference, and a free-list change touches every acquire anyway. Make the
|
||||
release wipe (`src/heap.c:211`) proportional to bytes used: either `memset`
|
||||
through `strnlen(data, AKGL_MAX_STRING_LENGTH) + 1` or a tracked length
|
||||
field — and the length-field option must respect the layout history in
|
||||
item 6, which put two real bugs on the bytes after `data`. Budgets to
|
||||
move: `tests/perf.c:317`, `:333`, `:347`; re-record the three rows in
|
||||
`PERFORMANCE.md` in the same commit.
|
||||
|
||||
2. **Text texture ring cache — targets 8 and 9, one cache.** A static
|
||||
four-to-eight entry ring in `src/text.c` keyed on (`TTF_Font *`, text
|
||||
bytes, `SDL_Color`, wraplength) — `x`/`y` stay out of the key, position
|
||||
only affects `dest`. Invalidate in `akgl_text_unloadfont`
|
||||
(`src/text.c:40-55`) and `akgl_text_unloadallfonts` (`:76-87`): a cached
|
||||
texture keyed on a closed font whose address was reused is a stale hit, so
|
||||
unload must sweep the ring. Verification is a counting test, not a
|
||||
stopwatch — target 9 is *zero* texture create/destroy per steady-state
|
||||
frame, so count them through a stub; plus a new perf row ("rendertextat,
|
||||
cached") with the raw-SDL control row the perf rules require. Budget to
|
||||
move: `tests/perf_render.c:391`.
|
||||
|
||||
3. **Non-raising sprite lookup — target 10, a design change.** Give
|
||||
`akgl_character_sprite_get` (`src/character.c:84-94`) the companion that
|
||||
returns `NULL` without raising, then convert the three sites that raise
|
||||
and handle `AKERR_KEY` for a routine condition: `akgl_actor_update`
|
||||
(`src/actor.c:165`, handled `:172`), `actor_visible` (`:209`, handled
|
||||
`:212-216`) and `akgl_actor_render` (`:241`, handled `:245-249`). While
|
||||
there, stop `akgl_actor_render` looking the sprite up twice on the success
|
||||
path (`:241` and again inside `actor_visible` at `:242`). Takes the
|
||||
sprite-less update from 616.5 ns toward the twice-success ceiling. Budget
|
||||
to move: `tests/perf.c:588`.
|
||||
|
||||
4. **Demote the hot-path log lines.** `akgl_actor_initialize` logs every
|
||||
spawn (`src/actor.c:51`) and `akgl_character_sprite_add` every binding
|
||||
(`src/character.c:80`); 20% of a spawn is formatting a line nobody reads,
|
||||
and that is with the output thrown away. Demote them (and
|
||||
`src/character.c:285`, `src/renderer.c:34`, `src/tilemap.c:660`) from
|
||||
`SDL_Log` to `SDL_LogDebug` — SDL checks priority before formatting, so
|
||||
the cost disappears at default priority and the messages survive for
|
||||
anyone who turns debug on. The spawn benchmark pair
|
||||
(`tests/perf.c:410`, `:429`) already measures exactly this gap.
|
||||
|
||||
5. **Fix the tileset scan, and the two bugs hiding in it.** The per-tile scan
|
||||
in `akgl_tilemap_draw` (`src/tilemap.c:756-798`) is the known
|
||||
O(tiles x tilesets) FIXME, worth 0.8% of the frame at eight tilesets — but
|
||||
it is also wrong twice. The range test uses `>=` on the upper bound
|
||||
(`firstgid + tilecount >= tilenum`, `src/tilemap.c:760-761`), so the tile
|
||||
one past the end of set A also matches set B's `firstgid`; and there is no
|
||||
`break` after a match, so a tile matching two ranges is blitted twice.
|
||||
Correct the test to `tilenum < firstgid + tilecount`, `break` on match,
|
||||
and skip `tilenum == 0` before scanning at all. Verify with a counting
|
||||
test — a stub `draw_texture` backend asserting exactly one blit per
|
||||
non-empty visible cell, same pattern as the `tests/game.c` update counter —
|
||||
because a boundary double-blit is invisible to a stopwatch and mostly
|
||||
invisible on screen. File alongside, same entry: `start_x`/`start_y` are
|
||||
never clamped to zero (`src/tilemap.c:719-728`), and the first-column
|
||||
`src.x +=` accumulates across rows (`:766`). Mutation testing is
|
||||
mandatory here; this is control flow. Do not reorganize the loader for
|
||||
this — 0.8% does not justify it.
|
||||
|
||||
6. **Bound the draw_world layer loop.** `akgl_render_2d_draw_world`
|
||||
(`src/renderer.c:150-167`) walks all `AKGL_TILEMAP_MAX_LAYERS` and rescans
|
||||
all 64 actor slots per layer — 1024 refcount checks a frame for a
|
||||
one-layer map. Bound the walk by `akgl_gamemap->numlayers` and build the
|
||||
per-layer actor lists in one pool pass into static index arrays.
|
||||
Invisible at 60 fps under the software renderer; measurable on a 2 ms GPU
|
||||
frame. Counting test, same as item 5.
|
||||
|
||||
7. **Target 12 stays deferred; the design goes on record.** At the 64-actor
|
||||
ceiling the all-pairs sweep is 0.7% of a frame and a spatial index is
|
||||
solving a problem we do not have. When `AKGL_MAX_HEAP_ACTOR` rises, the
|
||||
structure is the incremental uniform grid, not the tree: cells keyed on
|
||||
tile size, static arrays, insert/remove as actors move, wired into the
|
||||
`collide` slot of `akgl_PhysicsBackend` that the simulation loop does not
|
||||
call yet (`include/akgl/physics.h:55`). Construct measured this design at
|
||||
a 96% check reduction over brute force and rejected quadtrees as costlier
|
||||
to maintain; Phaser's rebuild-the-RTree-every-frame approach carries a
|
||||
documented ~5,000-body ceiling and is the counterexample. Citations in
|
||||
`PERFORMANCE.md`.
|
||||
|
||||
8. **Target 13 needs the fixture before it needs anything else.** A
|
||||
128x128, four-layer, multi-tileset JSON map fixture, so
|
||||
`akgl_tilemap_load` is measured on a map rather than on a PNG decode and a
|
||||
`memset`. Today the loader benchmark uses the 2x2 fixture while
|
||||
`perf_render` synthesizes its 128x128 map in-process
|
||||
(`tests/perf_render.c:141-175`) — the draw path is measured at realistic
|
||||
size and the load path is not. It would probably find something.
|
||||
|
||||
9. **Targets 14 and 15: the footprint refactor, sketched for when it is
|
||||
scheduled.** One shared cell arena sized
|
||||
`AKGL_TILEMAP_MAX_WIDTH * AKGL_TILEMAP_MAX_HEIGHT` once instead of
|
||||
per-layer, and tileset offset tables sized by `tilecount` instead of by
|
||||
`AKGL_TILEMAP_MAX_TILES_PER_IMAGE`. One correction to the record: the
|
||||
FIXME at `include/akgl/tilemap.h:113-128` worries about wasted *leading*
|
||||
entries per tileset, but `akgl_tilemap_compute_tileset_offsets`
|
||||
(`src/tilemap.c:179-231`) indexes by local id from zero — the table is
|
||||
merely oversized, not sparse. The refactor kills most of the 1.37 ms
|
||||
`memset` in `akgl_tilemap_load` (`src/tilemap.c:615`) and takes BSS from
|
||||
28 MB toward the 4 MB target. Real ABI break; belongs to a minor release,
|
||||
as the note above already says.
|
||||
|
||||
## Memory checking
|
||||
|
||||
`cmake --build build --target memcheck` runs **the suites that already exist**
|
||||
|
||||
Reference in New Issue
Block a user