diff --git a/TODO.md b/TODO.md index 75ac207..67586b6 100644 --- a/TODO.md +++ b/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**