Keep the defect list in TODO.md and point PERFORMANCE.md at it
The six defects the perf suites turned up were written out in full in both files. TODO.md is where a defect belongs -- file, line, functional consequence, blast radius, what closing it touches -- and duplicating that in a report guarantees the two drift, with no way to tell which copy is current. PERFORMANCE.md keeps what is actually its argument: that stress testing reaches failures a suite calling each function a handful of times does not, and that two of the six kill the process rather than slowing it down. The detail is one reference away. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -7,8 +7,10 @@ two commands, and the suites that produced it are checked in as
|
||||
`tests/perf.c` and `tests/perf_render.c`.
|
||||
|
||||
Read it in this order if you only want the short version: **the frame budget** is
|
||||
the part that matters, **what the numbers say** is the argument, and **defects
|
||||
these tests found** is the part that cost me a day.
|
||||
the part that matters and **what the numbers say** is the argument. The six
|
||||
defects the suites turned up on the way — two of them process-killing — are in
|
||||
`TODO.md` under **Performance**, along with the targets these numbers are
|
||||
measured against.
|
||||
|
||||
## Reproducing it
|
||||
|
||||
@@ -303,36 +305,20 @@ a terminal and it is far worse. `akgl_actor_initialize` and
|
||||
|
||||
## Defects these tests found
|
||||
|
||||
Stress testing is worth doing because it breaks things that unit tests do not.
|
||||
All six of these came out of writing this suite. Each is filed in `TODO.md` with
|
||||
its file, line, and blast radius.
|
||||
Six, and they are filed where defects live: `TODO.md`, under **Performance ->
|
||||
Defects the perf suites found**, items 28-33, each with its file, line,
|
||||
functional consequence, and what fixing it would touch. They are not repeated
|
||||
here.
|
||||
|
||||
1. **`akgl_path_relative` leaked an error context per call** on the root-fallback
|
||||
branch, because the branch `return`ed from inside its `HANDLE` block and so
|
||||
skipped the `RELEASE_ERROR` in `FINISH`. The 129th such call exhausted
|
||||
`AKERR_ARRAY_ERROR` and **aborted the process**. Every map load resolves
|
||||
several paths this way. *Fixed in this change*, with a regression test in
|
||||
`tests/util.c` that resolves 256 paths and asserts the pool is where it
|
||||
started.
|
||||
2. **`akgl_tilemap_load` leaks five pooled strings per load** that
|
||||
`akgl_tilemap_release` does not give back. The 52nd map load in a process
|
||||
finds the string pool empty. Not fixed — it needs the loader gone over
|
||||
properly, and that deserves its own commit.
|
||||
3. **`akgl_get_json_string_value` turns pool exhaustion into a segfault.** It
|
||||
finishes with `FINISH(errctx, false)`, so a failed `akgl_heap_next_string` is
|
||||
swallowed, and the next line dereferences the pointer it never set
|
||||
(`src/json_helpers.c:91`). This is what defect 2 actually looks like from the
|
||||
outside: not `AKGL_ERR_HEAP`, a crash.
|
||||
4. **`akgl_game_update` segfaults if `akgl_game_init` did not run.**
|
||||
`akgl_game_updateFPS` calls `game.lowfpsfunc` through an unguarded pointer on
|
||||
every frame under 30 fps, which includes the first frame. That is precisely
|
||||
the path `renderer.h` documents for an embedder — `akgl_render_bind2d` over a
|
||||
window the host already owns.
|
||||
5. **`akgl_game_update`'s 16x update multiplier**, above.
|
||||
6. **`akgl_heap_release_character` leaks the character's `state_sprites`
|
||||
property set** and never drops the references it took on its sprites. Already
|
||||
documented in `heap.h`; this suite makes it observable, since a benchmark that
|
||||
loads a character 10,000 times leaks 10,000 property sets.
|
||||
Worth saying in this document, because it is the argument for having written the
|
||||
suites at all: stress testing breaks things unit tests do not reach. Two of the
|
||||
six are process-killing — an error-context leak that aborted on the 129th path
|
||||
resolution (fixed, with a regression test), and a pooled-string leak that turns
|
||||
into a segfault rather than an `AKGL_ERR_HEAP` around the 52nd map load. Neither
|
||||
is reachable by a suite that calls each function a handful of times, and neither
|
||||
had anything to do with speed. They came out of loops that ran the same call ten
|
||||
thousand times in one process, which is what a running game does and what
|
||||
nothing else in this tree did.
|
||||
|
||||
## What this report does not cover
|
||||
|
||||
|
||||
Reference in New Issue
Block a user