diff --git a/BUILT.md b/BUILT.md index e3b35c2..696d81e 100644 --- a/BUILT.md +++ b/BUILT.md @@ -4815,3 +4815,75 @@ now read out before the check, and the loop advances to a saved `match-end`. `emacs/*.el` is already a dependency of that dune stanza, so a new `.el` needs no build change. Nothing in it needs a daemon — ghost text is a function from a table of rows and the text in a buffer to overlays, and both halves are fixtures. + +## A hot loop keeps five numbers, and the window is the editor's + +The scalar watch above keeps one value per name. From a hot inner loop that is nearly useless: you see whichever of +the 91,200 cells happened to run last, and `watch.clj`'s own docstring says so — "from a hot loop you only ever see +whichever cell happened to run last — use `spy-long` there instead." This is that other half, and it is the piece +`PORTING.md` item 5 calls least obvious and most valuable. + +`(watch-num-i64 "cell" (at grid i))` reaches `flan_dev_watch_num_i64` through the same plain `declare-c` the four +scalars use. No arm in the checker, no special form, nothing the compiler learns — the same deliberate non-ownership +of `check.ml` that the scalars were built under. + +**What a slot keeps: count, min, max, last, mean.** The argument for those five is that each answers a question you +can ask without building a query. `n` is how many times the expression ran, which is the first thing that is wrong +when a loop is wrong — a count that tracks the frame counter rather than the cells is a loop that is not running. +`min` and `max` are the range, which is the thing a single sample can never show you and the thing you are looking +for when you suspect an index or a velocity is leaving the region it should stay in. `last` is the one sample, kept +because it is what the scalar watch would have given you and losing it would be a regression. `mean` is carried as a +running `sum` and divided at read time, because a mean accumulated as a mean drifts and a sum does not. + +**What it deliberately does not keep: a ring, or a history.** A small ring of the last N samples was the other +candidate and it loses on the only ground that matters: N samples out of 91,200 is a sample of the *tail* of the +loop, not of the loop, so it answers "what did the last few cells do" when the question is "what did the cells do". +Beyond five numbers every richer answer is a UI for building a query, and a query builder is the one thing this +design exists not to be — the same reason the ghost text section gives for showing the last value in a loop rather +than offering to pick one. + +**The write path does no formatting, and that is the whole feature.** An `snprintf` per sample at thousands of +samples a frame is a HUD that costs more than the game. A sample is a relaxed load, five compares and stores, and the +slot's seqlock; the *reader* — the agent's listener thread, once per editor tick — turns the five numbers into text. +`watch.clj` reaches the same place with a `double-array` per label and a `render` that Emacs calls, and the reasoning +is the author's rather than ours. It is also why the slot grew a `num` flag instead of a second table: the read path +already copies under a seqlock, so it copies five doubles instead of 192 bytes and renders them out of locals after +the counter check. + +**The window is since the last reset, and this is a deliberate divergence from `watch.clj`.** There the stats are +cumulative until `reset-spies!` is called by hand. Cumulative is the wrong default for a frame loop: a `min` and a +`max` over a whole session reach the session's extremes within a few seconds of play and then never move again, so +the two most useful of the five go dead exactly when you start interacting with the thing you are debugging. This +tool exists to show you a number while you drag the mouse. So `flan-watch--tick` sends `:reset t` beside its read and +the displayed range is "since you last looked" — a fifth of a second, a dozen frames. A caller that wants the +cumulative numbers gets them by not resetting; the setting lives in the editor, not in the runtime. + +**Reset is its own message, and it is not a side effect of reading.** A destructive read was the tempting shape and +is wrong: it makes *looking* change what is there, so anything that polls — a test's `await`, a second editor, a +person reading twice — silently shortens the window and gets a count that is noise. This was caught before the test +was written rather than after, which is the only reason the test has assertions about `n` at all. `watch reset` is a +line in `flan_agent.c` beside `watch on`/`watch off`, `:reset t` is a field on the read op, and `dev.ml` sends it +*after* the read so the tick reports the window it just closed. + +**The reader never writes the table.** Reset bumps one global epoch counter and touches no slot; a slot clears itself +on its next sample, when it notices the epoch has moved, and does that *inside* its own odd-generation window so a +reader can never catch a half-cleared slot. The game thread stays the only writer of the table, which is the +invariant the whole of `flan_dev.c`'s watch section rests on. The cost is that a new window begins when the program +next runs rather than at the instant of the reset — and for a frame loop that is the only moment it could sensibly +begin. The test waits for it rather than reading once, and says why. + +**An `i64` accumulates as a double**, so a magnitude past 2^53 loses precision in the sum and in the ends of the +range. Recorded rather than designed around: a count, a coordinate and a tile index are what this is pointed at, and +a second integer accumulator for a case nobody has would be two code paths for one tool. `watch-num-i64` and +`watch-num-f64` are two entry points only so a program need not cast at the call site, which is noise in the one +place this is meant to be droppable into. + +**A whole number prints as one.** `watch.clj`'s `fmt-num` does this and the reason survives the port: a watch on an +array index that reads `66.0000` sends you looking for a rounding bug that is not there. + +**Ghost text needed one character.** `flan-watch-ghost-call-regexp` was `watch\(?:-[[:alnum:]]+\)?` — one optional +hyphenated segment, which matches `watch-i64` and backtracks to failure on `watch-num-i64`, so a numeric watch got +no inline value at all while appearing normally in the buffer. A `*` for the `?` is the whole fix. It is worth +recording as the predictable cost of the decision that section defends: anchoring on the *name* rather than on the +head of the call is what makes the head a `defcustom`, and a `defcustom` with an enumerated default is a default that +needs widening when the set of heads grows. That is a cheaper failure than the alternative and it is not a free one. diff --git a/HANDOFF-f3.md b/HANDOFF-f3.md new file mode 100644 index 0000000..9e35de7 --- /dev/null +++ b/HANDOFF-f3.md @@ -0,0 +1,234 @@ +# Handoff — a watch for a running program (`PORTING.md` Tier 1, item 5) + +Written because the session was wound down for budget. **The work is finished and `dune test` is green**, run twice. +This file exists so the reasoning is not re-derived, and so the two things I would have done next are named. + +--- + +## The finding that reframes the task + +**Most of item 5 was already built before this session.** The brief reads as though a watch had to be built from +nothing; it did not. What existed at `344e571`: + +- `runtime/flan_dev.c` — the watch table: 64 slots, a name and rendered text per slot, a per-slot seqlock, an + `on`/`off` flag so a watch call costs a load and a not-taken branch when nobody is looking, and four scalar entry + points (`flan_dev_watch_i64`/`_u64`/`_f64`/`_str`). +- `vendor/agent/flan_agent.c` — `watch`, `watch on`, `watch off` on the agent socket. +- `lib/dev.ml` — `watch_read`, `watch_enable`, and the `:op "watch"` / `:op "watch-enable"` arms. +- `emacs/flan-watch.el` — the watch buffer *and* the inline ghost text, both fed from one reply. +- `test/test_dev.ml` — a block driving a real daemon and a real running program. + +That is `watch.clj`'s `spy`, end to end. **What was missing was `spy-num`** — the numeric accumulator for hot loops, +which is precisely the part `PORTING.md` calls "least obvious and most valuable". That is what this session built. + +--- + +## The accumulator decision + +This is the expensive part to re-derive, so it is stated in full. The long-form version is in `BUILT.md`, "A hot loop +keeps five numbers, and the window is the editor's"; `runtime/flan_dev.c` carries it at the code. + +### What a slot keeps + +**Count, min, max, last, mean.** Five numbers per label. + +The scalar watch keeps one value per name. Sampled from a hot inner loop that is nearly useless — you see whichever +of the 91,200 cells happened to run last. `watch.clj`'s own docstring says so and is why `spy-num` exists there. + +Each of the five answers a question you can ask *without building a query*: + +- **`n`** — how many times the expression actually ran. This is the first thing that is wrong when a loop is wrong. A + count that tracks the frame counter rather than the cell count is a loop that is not running. +- **`min` / `max`** — the range. This is the thing a single sample can never show you, and it is what you are looking + for when you suspect an index or a velocity is leaving the region it should stay in. +- **`last`** — the one sample. Kept because it is what the scalar watch would have given you, and losing it on the + way to something richer would be a regression. +- **`mean`** — carried as a running `sum` and divided at read time. A mean accumulated as a mean drifts; a sum does + not. + +### What it deliberately does not keep + +**A ring, or a history.** A small ring of the last N samples was the serious alternative. It loses on the only ground +that matters here: N samples out of 91,200 is a sample of the *tail* of the loop, not of the loop. It answers "what +did the last few cells do" when the question is "what did the cells do". Past five numbers, every richer answer is a +UI for building a query — and a query builder is the one thing this whole design exists not to be. The ghost text +section of `BUILT.md` already settled the same question the same way for the scalar case ("a watch inside a loop +shows the last value written... every better answer is a UI for building a query. Settled, not open."). + +### The write path does no formatting + +This is the feature, not an optimisation. An `snprintf` per sample at thousands of samples a frame is a HUD that +costs more than the game. A sample is: one relaxed load of the epoch, five compares and stores, and the slot's +seqlock. The **reader** — the agent's listener thread, once per editor tick — turns the five numbers into text. +`watch.clj` reaches the same place with a `double-array` per label and a `render` function Emacs calls. + +This is also why the slot grew a `num` flag rather than getting a second table: the read path already copies under a +seqlock, so it copies five doubles instead of 192 bytes and renders them out of locals *after* the counter check. + +### The window is since the last reset — a deliberate divergence from `watch.clj` + +`watch.clj` is cumulative until `reset-spies!` is called by hand. **This is not**, and the disagreement is on purpose. + +Cumulative is the wrong default for a frame loop. A `min` and a `max` over a whole session reach the session's +extremes within a few seconds of play and then never move again — so the two most useful of the five numbers go dead +*exactly* when you start interacting with the thing you are debugging. This tool exists to show you a number while +you drag the mouse. So `flan-watch--tick` sends `:reset t` beside its read, and what you see is "since you last +looked": a fifth of a second, about a dozen frames, which keeps the range tracking the present. + +A caller who wants cumulative numbers gets them by not resetting. The setting lives in the editor, not the runtime. + +### Reset is its own message, never a side effect of reading + +A destructive read was the tempting shape and is wrong: it makes *looking* change what is there. Anything that polls +— a test's `await`, a second editor, a person reading twice — would silently shorten the window and come back with a +count that is noise. So `watch reset` is its own agent command, `:reset t` is a field on the read op, and `dev.ml` +sends the reset *after* the read so a tick reports the window it just closed. + +This was settled before the test was written rather than after, which is the only reason the test can assert anything +about `n` at all. + +### The reader never writes the table + +Reset bumps one global epoch counter and touches no slot. A slot clears itself on its **next sample**, when it +notices the epoch moved, and does so *inside* its own odd-generation window so a reader can never catch a +half-cleared slot. The game thread stays the only writer of the table — the invariant the whole watch section of +`flan_dev.c` rests on. + +The cost: a new window begins when the program next runs, not at the instant of the reset. For a frame loop that is +the only moment it could sensibly begin. **This bit me in the test** (see "What did not work" below) and the test now +waits for it and says why. + +### `i64` accumulates as a double + +A magnitude past 2^53 loses precision in the sum and in the ends of the range. Recorded rather than designed around: a +count, a coordinate and a tile index are what this is pointed at, and a second integer accumulator for a case nobody +has would be two code paths for one tool. `watch-num-i64` and `watch-num-f64` are two entry points only so a program +need not cast at the call site. + +### A whole number prints as one + +`watch.clj`'s `fmt-num` does this and the reason survives the port: a watch on an array index that reads `66.0000` +sends you looking for a rounding bug that is not there. + +--- + +## What was built, file by file + +All of it is **working** — built, run against a real daemon and a real running program, and covered by the test. +Nothing is stubbed and nothing is half-written. + +| File | Change | State | +|---|---|---| +| `runtime/flan_dev.c` | `watch_slot` grew `num`, `epoch` and five doubles; `watch_begin` clears `num`; new section "A number sampled thousands of times a frame" with `watch_record`, `flan_dev_watch_reset`, `flan_dev_watch_num_i64`, `flan_dev_watch_num_f64`, `watch_num_str`, `watch_render_num`; `flan_dev_watch_read` renders a num slot instead of copying its (unused) text | working | +| `vendor/agent/flan_agent.c` | `flan_dev_watch_reset` declaration; a `watch reset` command beside `watch on`/`watch off` | working | +| `lib/dev.ml` | `watch_read` takes `~reset` and sends `watch reset` *after* the read; the `:op "watch"` arm reads a `:reset` field the same way `watch-enable` reads `:on` | working | +| `emacs/flan-watch.el` | `flan-watch-ghost-call-regexp` `?` → `*`; docstring updated; `flan-watch--tick` sends `(:op "watch" :reset t)` | working | +| `test/programs/dev-watch.flan` | `declare-c watch-num-i64`; a `loop-cells` hot loop sampling `"cell"` eight times a step at 0,3,…,21 | working | +| `test/test_dev.ml` | assertions inside the existing watch block: the row exists, `n > 8` (per-sample not per-step), `min < max` (a range, not just `last`), a plain read does not reset, `:reset t` reopens the window | working | +| `BUILT.md` | new section "A hot loop keeps five numbers, and the window is the editor's", after the ghost text one | working | +| `PORTING.md` | item 5 struck in house style with the accumulator decision | working | +| `NEXT.md` | "What is left on `PORTING.md`'s list" corrected: item 5 done, item 6 is next | working | + +### Verified end to end, not just compiled + +Against a real `flan dev` daemon and the real running program, the `"cell"` row read back: + +``` +n=184 min=0 max=21 last=21 mean=10.5000 (first read) +n=368 min=0 max=21 last=21 mean=10.5000 (second read — a plain read does not reset) +n=16 min=0 max=21 last=21 mean=10.5000 (after :reset t — the window reopened) +``` + +The loop is 0,3,…,21, so `min`, `max`, `mean` and the per-step count of 8 are each pinned by a different part of it. + +### Test result + +`dune test` is **green**, run twice in full. `test_dev.ml`'s first block — the separately-known-flaky socket bind race +— failed on one earlier run with "the daemon never listened" and passed on both final runs. That is exactly the +documented flake and another agent is on it; it is not related to anything here. + +--- + +## The ghost text path — the brief's assumption, checked + +The brief said to reuse the ghost text path rather than invent a second one. **It fits, and it needed one character.** + +The important property is in `BUILT.md`: ghost text anchors on the **string literal in the source**, not on a source +location carried in the table. `flan-watch--ghost-sites` scans buffers shown in a window for +`( ""` and matches the name against the rows. So a new *kind* of watch needs nothing new in the daemon, +nothing new in the wire, and no new display path — the accumulator's rendered value is a string like +`n=368 min=0 max=21 last=21 mean=10.5` and flows through the existing `:watch` rows untouched. + +**Is it pull-based?** No, and the brief's phrasing is worth correcting for whoever reads this next. The watch is +**push-based** and that is load-bearing: `watch.clj` is pull (Emacs polls a render function), and `flan_dev.c`'s +comment explains at length why that does not port — an evaluation here compiles a module and `dlopen`s it, so a 5Hz +poll would be hundreds of `.so` files a minute. So the program pushes into a table and the editor reads *memory*. +Ghost text is not a poller at all: it is painted from `flan-watch--absorb`, off the same single reply that paints the +buffer, which is what keeps the two from disagreeing and preserves the one-request-in-flight invariant. The +accumulator sits inside that unchanged. + +**The one thing that did not fit.** `flan-watch-ghost-call-regexp` was `watch\(?:-[[:alnum:]]+\)?` — *one* optional +hyphenated segment. `[[:alnum:]]` does not match a hyphen, so on `(watch-num-i64 "cell" ...` the regexp matched +`watch-num`, then required whitespace, found `-i64`, and backtracked to failure. A numeric watch therefore appeared +normally in the watch buffer and got **no inline value at all** — a silent half-failure. `*` for `?` is the whole +fix, verified in `emacs --batch` against all four head shapes. + +That is worth recording as the predictable cost of the decision the ghost text section defends. Anchoring on the name +rather than on the head is what makes the head a `defcustom`, and a `defcustom` with an enumerated default is a +default that needs widening whenever the set of heads grows. Cheaper than the alternative, but not free. + +--- + +## What remains, in the order a fresh session should do them + +Item 5 is complete. These are the two gaps I would close next, both small. + +1. **`emacs/test-flan-watch.el` — add a `watch-num-i64` fixture site.** The regexp fix is covered by *nothing* + automated; I verified it by hand in `emacs --batch`. Add a fixture whose buffer text contains + `(watch-num-i64 "cell" x)` and assert `flan-watch--ghost-sites` returns `("cell" . …)`, alongside the existing + `watch-i64` fixtures. No daemon needed — the file is already in the suite via `test-flan-cider.el` and, per + `BUILT.md`, ghost text is a pure function from rows plus buffer text to overlays. **This is the highest-value + remaining item**: it is the one change in this commit with no regression test behind it. + +2. **`test/test_dev.ml` — assert the `n=0` render.** `watch_render_num` in `runtime/flan_dev.c` has a branch for a + window with no samples that emits `n=0 last=` (the range and the mean have nothing behind them, but `last` is + still the last value the name ever had). No test reaches it: the program in `dev-watch.flan` samples every step, + so a window is never empty in practice. Reaching it needs a reset followed by a read inside one step, or a + program with a watch call that runs only sometimes. + +Neither blocks anything. Item 6 (frame rollback as a worked example) is the next real piece of work and `NEXT.md` now +points at it. + +--- + +## What I tried that did not work + +- **Two Flan type errors in `dev-watch.flan`.** `(* i 3)` over a `let`-bound `0` is `i32`, and `watch-num-i64` wants + `i64` — fixed with an explicit `(i64 …)`. Then `loop-cells` was declared `i64` and returned the `i32` counter; + declared `i32`. Both are ordinary, but they cost two daemon round trips to find because a build error goes to the + daemon's stdout, not to the client — worth knowing if you are driving `flan dev` by hand. + +- **My first probe script spoke the wrong protocol.** `Wire.send` is **length-prefixed** (`"%d\n%s"`), not + newline-delimited. A newline-delimited client gets the connection closed with no error text. If you write a + throwaway client, read `lib/wire.ml:32` first. + +- **Two attempts at the reset assertion failed before the third stuck**, and both failures were informative rather + than noise: + 1. *"the window did not reopen: 8 after 8"* — one step writes eight samples, so two reads back to back leave no + room *under* the count for a reset to show in. The test now runs the accumulator up past 32 first (which is + itself the other half of the claim: every one of those polls is a plain read, and a plain read must not reset, + or the count could never climb). + 2. *"the window did not reopen: 40 after 40"* — reading immediately after `:reset t` still sees the old count, + because the reset bumps an epoch and the **slot clears on its next sample**. That is the design and not a bug; + the test now `await`s the drop and the comment explains why the laziness is what keeps the reader out of the + table. + +- **Not attempted, deliberately:** the `(watch "hp" hp)` arm in `check.ml`. Both `PORTING.md` and `flan_dev.c` say + it is only wanted for *composites* (a struct, a slice, a union), which need `Render.render` pointed at the + `flan_dev_watch_emit_*` emitters. A scalar and an accumulator both reach the runtime by plain `declare-c` and need + no compiler change, which is the property that kept this change out of `check.ml` entirely. + +- **One worktree note.** This worktree was checked out at `2c232dd`, an ancient commit, rather than at `dev-loop`'s + tip — none of `PORTING.md`, `NEXT.md`, `lib/dev.ml` or `runtime/` existed in it. `git reset --hard dev-loop` on a + clean tree fixed it. Also, `dune` finds the parent repo's root from inside a worktree, so every build here is + `dune build --root .` / `dune test --root .`. diff --git a/NEXT.md b/NEXT.md index 540ec54..6eed88f 100644 --- a/NEXT.md +++ b/NEXT.md @@ -110,10 +110,12 @@ would not read freed memory. The map path was left alone rather than converted h ### What is left on `PORTING.md`'s list -Tier 0 is finished. Of Tier 1, items 5 and 6 are the next two and they are both dev-loop work rather than language -work: **a watch for a *running* program** (the stopped-stack inspector is a different tool for a different moment — -`watch.clj` + `spy-num` is the shape, and the numeric accumulator for hot loops is the part that is least obvious), -and **frame rollback as a worked example** — `snapshot`/`restore` callbacks beside the `continue` restart, which is +Tier 0 is finished, and so is item 5. **The watch for a running program is done** — the `spy` half was already +built (a pushed table, the watch buffer, inline ghost text) and the `spy-num` half landed 2026-09-13: a hot-loop slot +keeps count/min/max/last/mean, the write path does no formatting, and the window is since the editor's last tick +rather than cumulative, which is a deliberate divergence from `watch.clj` argued in `BUILT.md`, "A hot loop keeps +five numbers, and the window is the editor's". So of Tier 1 **item 6 is the next one**: **frame rollback as a worked +example** — `snapshot`/`restore` callbacks beside the `continue` restart, which is now genuinely reachable from a bad index and so is worth more than it was yesterday. `bounds-condition.flan` shows an abandoned frame leaving half-written state behind; rollback is what finishes that thought. diff --git a/PORTING.md b/PORTING.md index 9289b55..1bfcd48 100644 --- a/PORTING.md +++ b/PORTING.md @@ -513,11 +513,35 @@ not compete for the same slot. `continue` instead of ending the process, the missing half is the rollback, because an abandoned frame leaves the grid half-written. That is the next thing on this list. -5. **A watch for a running program.** `watch.clj` + `watch.el` + `spy` + `spy-num` is a - tool the author built deliberately and uses constantly, and the stopped-stack inspector - is a different tool for a different moment. Given that the dev loop is the priority, - this outranks every language feature below it. The numeric accumulator for hot loops is - the part that is least obvious and most valuable. +5. ~~**A watch for a running program.**~~ **Done, 2026-09-13.** The `spy` half — a + pushed table of labelled values, read as memory while the program runs, and still + readable while it is stopped — was already built, along with the watch buffer and the + inline ghost text. What landed today is the `spy-num` half, which is the part this item + called least obvious and most valuable, and it is the part that was missing. + + **A slot keeps five numbers: count, min, max, last, mean.** Each answers a question you + can ask without building a query — `n` is the first thing wrong when a loop is wrong, + the range is what one sample can never show you, `last` is what the scalar watch would + have given you, and the mean is carried as a sum and divided at read time because a + mean accumulated as a mean drifts. A small ring of the last N samples was the other + candidate and loses: N out of 91,200 is a sample of the *tail* of the loop rather than + of the loop. **The write path does no formatting** — a sample is a load, five compares + and the slot's seqlock, and the listener thread renders once per editor tick, which is + the whole reason `spy-num` exists rather than a second `spy`. + + **One deliberate divergence from `watch.clj`**, recorded because it is a real + disagreement and not an oversight: there the stats are cumulative until `reset-spies!`. + Here the window is since the editor's last tick. Cumulative min and max reach the + session's extremes within a few seconds of play and then never move again, so the two + most useful of the five go dead exactly when you start interacting with the thing you + are debugging — and this tool exists to show you a number while you drag the mouse. + Reset is its own message rather than a side effect of reading, so anything that polls + cannot shorten the window under the editor that owns it. + + Full reasoning in `BUILT.md`, "A hot loop keeps five numbers, and the window is the + editor's". One thing this did *not* need and is worth saying: no arm in `check.ml`. The + accumulator is reached by the same plain `declare-c` the scalars use, so the `(watch + "hp" hp)` form is still unbuilt and still only wanted for composites. 6. **Frame rollback in the engine pattern.** Not a language feature — the pieces are all there (`restart-case`, struct assignment, fixed arrays as values). What is missing is diff --git a/emacs/flan-watch.el b/emacs/flan-watch.el index bcca93a..029d7db 100644 --- a/emacs/flan-watch.el +++ b/emacs/flan-watch.el @@ -204,12 +204,14 @@ was the only consumer and wrong the moment it was not.") ;; does not need to: you opened it deliberately and `flan:stopped(...)' is ;; already in view. -(defcustom flan-watch-ghost-call-regexp "watch\\(?:-[[:alnum:]]+\\)?" +(defcustom flan-watch-ghost-call-regexp "watch\\(?:-[[:alnum:]]+\\)*" "Regexp matching the head of a call that writes the watch table. Matched against the symbol after an open paren, with the watched name as a -string literal after it. The default covers `watch', `watch-i64', `watch-f64' -and `watch-str'. It is a setting because the Flan name is the program author's +string literal after it. The default covers `watch', `watch-i64', `watch-f64', +`watch-str' and the two-segment accumulator names `watch-num-i64' and +`watch-num-f64' — hence the `*\=' rather than a `?\=', which stopped at one +hyphenated segment and so found no site for a numeric watch at all. It is a setting because the Flan name is the program author's own `declare-c' binding — only the C symbol behind it is fixed, and the editor never sees that." :type 'regexp) @@ -385,7 +387,15 @@ sends the next question, leaving at most one request in flight — the invariant (flan-watch--absorb reply))) (unless flan-watch--pending (condition-case nil - (progn (flan-dev--send flan-dev--connection '(:op "watch")) + ;; `:reset t' opens a new accumulation window for the numeric + ;; slots, so their count, range and mean are "since the last tick" + ;; rather than since the program started. That is the whole of the + ;; accumulator decision as the editor sees it: a min and a max over + ;; a session go dead within seconds of play, and this tool exists to + ;; show a number while you drag the mouse. The reset happens after + ;; the read, on the daemon's side, so this tick's numbers are the + ;; last tick's window and nothing is lost between the two. + (progn (flan-dev--send flan-dev--connection '(:op "watch" :reset t)) (setq flan-watch--pending t)) (error (flan-watch-stop))))))) diff --git a/lib/dev.ml b/lib/dev.ml index b7c2a88..94dffcd 100644 --- a/lib/dev.ml +++ b/lib/dev.ml @@ -1626,11 +1626,22 @@ let watch_enable t ~on = said nothing about it would be lying by omission. It is a flag and not a count on purpose: the only number available is of write *attempts* that missed, which at frame rate says "3847 names" about one name. *) -let watch_read t = +(* [~reset] opens a new accumulation window for the numeric slots, *after* the + read rather than instead of it. A read that reset as a side effect would + make looking change what is there, so anything that polls would silently + shorten the window and come back with a count that means nothing. Two + messages down a unix socket, one of which is four bytes of reply. *) +let watch_read t ~reset = match request t "watch" with | text -> + let finish result = + if reset then + (try ignore (request t "watch reset") with Unix.Unix_error _ -> ()); + result + in let lines = String.split_on_char '\n' text in - (match lines with + finish + @@ (match lines with | [] -> error "the program gave an empty watch reply" | hdr :: rows when not (String.length hdr >= 3 && String.sub hdr 0 3 = "err") -> @@ -1750,7 +1761,15 @@ let handle t req = ~on:(match Wire.field req "on" with | Some { Form.v = Form.Sym "nil"; _ } | None -> false | Some _ -> true) - | Some "watch" -> watch_read t + | Some "watch" -> + (* Same shape as [:on] above: absent or [nil] is false, anything else is + true. Absent is the important half — a reader that does not ask to reset + must not, so a second editor or a test polling the table cannot cut the + window short under the one that does. *) + watch_read t + ~reset:(match Wire.field req "reset" with + | Some { Form.v = Form.Sym "nil"; _ } | None -> false + | Some _ -> true) | Some "disassemble" -> (match Wire.string_field req "name" with | Some name -> diff --git a/runtime/flan_dev.c b/runtime/flan_dev.c index c2011fd..3c439ea 100644 --- a/runtime/flan_dev.c +++ b/runtime/flan_dev.c @@ -382,6 +382,12 @@ typedef struct { char val[WATCH_VAL]; uint32_t len; int full; /* the value did not fit */ + /* An accumulator slot. [val]/[len] are unused; the five doubles below are + * the value, and the *reader* turns them into text. See "A number sampled + * thousands of times a frame" below. */ + int num; + uint64_t epoch; /* the window [n]/[lo]/[hi]/[sum] belong to */ + double n, lo, hi, last, sum; uint64_t gen; /* this slot's own seqlock */ } watch_slot; @@ -389,6 +395,12 @@ static watch_slot watch_table[WATCH_MAX]; static uint32_t watch_used; /* claimed slots; only ever grows */ static int watch_overflowed; /* some name found no slot; see above */ +/* The current accumulation window. Bumped by [flan_dev_watch_reset], which is + * the editor saying "start again from here"; a slot notices on its next + * sample. The counter is the reader's to move and the slots are the writer's + * to clear, so no thread ever writes the other's memory. */ +static uint64_t watch_epoch; + /* One counter per slot rather than one for the table. * * A table-wide counter would make a read all-or-nothing: the reader would have @@ -480,6 +492,11 @@ int flan_dev_watch_begin(const char *name) { __atomic_thread_fence(__ATOMIC_RELEASE); s->len = 0; s->full = 0; + /* A name written as text is a text slot from now on. The table's rule + * everywhere else is that the last writer wins and this is the same rule; + * a program that watches one name both ways gets whichever ran last, and + * that is not worth a guard on the frame thread. */ + s->num = 0; return 1; } @@ -620,6 +637,149 @@ int32_t flan_dev_watch_str(const char *name, const char *s) { return 1; } +/* ── A number sampled thousands of times a frame ─────────────────────── */ + +/* The scalar watch above keeps one value per name, and from a hot inner loop + * that is nearly useless: you see whichever of the 91,200 cells happened to + * run last. This is the other half — the port of [spy-num] from the author's + * watch.clj, which exists for exactly that reason and is the piece PORTING.md + * calls the least obvious and the most valuable. + * + * **What a slot keeps: count, min, max, last, mean.** Five numbers, and the + * argument for them is that they are the ones you can ask for without building + * a query. [n] says how many times the expression ran, which is the first + * thing that is wrong when a loop is wrong. [min] and [max] are the range, + * which is what you are looking for when you suspect an index or a velocity is + * leaving the region it should stay in — and a range is the thing a single + * sample can never show you. [last] is the one sample, kept because it is what + * the scalar watch would have given you and losing it would be a regression. + * [mean] is carried as a running [sum] and divided at read time, because a + * mean accumulated as a mean drifts and a sum does not. + * + * **What it deliberately does not keep: a ring, or a history.** A small ring + * of the last N samples was the other candidate. It loses on the only ground + * that matters here: N samples out of 91,200 is a sample of the *tail* of the + * loop, not of the loop, so it answers "what did the last few cells do" when + * the question is "what did the cells do". Every richer answer than five + * numbers is a UI for building a query, and a query builder is the one thing + * this design exists not to be. + * + * **The write path does no formatting.** That is the entire point and is where + * the naive version dies: an [snprintf] per sample at thousands of samples per + * frame is a HUD that costs more than the game. A sample here is a load, five + * compares and stores, and the slot's seqlock. The *reader* — the agent's + * listener thread, once per editor tick — turns the five numbers into text. + * watch.clj reaches the same place with a double-array per label and a render + * function Emacs calls; the reasoning is the author's, not ours. + * + * **The window is since the last reset, not since the program started.** This + * is the one place this deliberately *diverges* from watch.clj, whose stats + * are cumulative until [reset-spies!] is called by hand. Cumulative is the + * wrong default for a frame loop: a min and a max over a whole session reach + * the session's extremes within a few seconds of play and then never move + * again, so the two most useful of the five go dead exactly when you start + * interacting with the thing you are debugging. This tool exists to show you a + * number while you drag the mouse. So the editor bumps + * [flan_dev_watch_reset] on its tick and each slot clears itself on its next + * sample, which makes the displayed range "since you last looked" — a fifth of + * a second, a dozen frames — and keeps it tracking the present. A caller that + * wants cumulative numbers gets them by not resetting; that is the setting, + * and it lives in the editor rather than here. + * + * **i64 accumulates as a double**, so a magnitude past 2^53 loses precision in + * the sum and in the ends of the range. Said rather than designed around: a + * count, a coordinate and a tile index are what this is pointed at, and a + * second integer accumulator to cover the case nobody has would be two code + * paths for one tool. */ +static void watch_record(watch_slot *s, double v) { + uint64_t e = __atomic_load_n(&watch_epoch, __ATOMIC_RELAXED); + /* Odd first, then the change, then even — [flan_dev_watch_begin]'s protocol + * exactly, and the epoch check is *inside* the odd window so a reader can + * never catch a half-cleared slot. */ + __atomic_store_n(&s->gen, s->gen | 1, __ATOMIC_RELAXED); + __atomic_thread_fence(__ATOMIC_RELEASE); + s->num = 1; + if (s->epoch != e) { s->epoch = e; s->n = 0; s->sum = 0; } + if (s->n == 0) { s->lo = v; s->hi = v; } + else { if (v < s->lo) s->lo = v; if (v > s->hi) s->hi = v; } + s->n += 1; + s->sum += v; + s->last = v; + __atomic_store_n(&s->gen, (s->gen | 1) + 1, __ATOMIC_RELEASE); +} + +/* Start a new window. Called by the editor beside its table read, never by the + * program. It moves one counter and touches no slot, which is what keeps the + * game thread the only writer of the table. */ +void flan_dev_watch_reset(void) { + __atomic_add_fetch(&watch_epoch, 1, __ATOMIC_RELEASE); +} + +/* Reachable from a program by [declare-c], the same as the four scalars and + * for the same reason — no arm in the checker, nothing the compiler learns: + * + * (declare-c watch-num-i64 [name string x i64] i32 "flan_dev_watch_num_i64") + * ... + * (watch-num-i64 "cell" (at grid i)) + * + * Two entry points rather than one so a program need not cast an i64 to an f64 + * at the call site, which is noise in the one place this is meant to be + * droppable into. */ +int32_t flan_dev_watch_num_i64(const char *name, int64_t x) { + if (!__atomic_load_n(&watch_on, __ATOMIC_RELAXED)) return 0; + watch_slot *s = watch_find(name); + if (s == NULL) return 0; + watch_record(s, (double)x); + return 1; +} + +int32_t flan_dev_watch_num_f64(const char *name, double x) { + if (!__atomic_load_n(&watch_on, __ATOMIC_RELAXED)) return 0; + watch_slot *s = watch_find(name); + if (s == NULL) return 0; + watch_record(s, x); + return 1; +} + +/* A whole number prints as one. watch.clj's [fmt-num] does this and the reason + * is worth keeping: a spy on an array index that reads 66.0000 makes you look + * for a rounding bug that is not there. */ +static void watch_num_str(char *out, size_t cap, double d) { + if (d >= -9.0e15 && d <= 9.0e15 && d == (double)(long long)d) + snprintf(out, cap, "%lld", (long long)d); + else + snprintf(out, cap, "%.4f", d); +} + +/* Rendered on the listener thread, from numbers already copied out of the + * slot, so nothing here races and nothing here runs per sample. Comfortably + * inside [WATCH_VAL]: five numbers at %.4f and their labels is well under 192 + * bytes, and [snprintf] truncates rather than overruns if a caller's buffer is + * smaller than [flan_dev_watch_val_cap]. */ +static uint64_t watch_render_num(double n, double lo, double hi, double last, + double sum, char *vd, uint64_t vcap) { + if (vcap == 0) return 0; + char b[4][40]; + int k; + if (n == 0) { + /* No sample since the window opened. The range and the mean have nothing + * behind them and showing the previous window's would be a lie, but [last] + * is still the last value this name ever had, so it is kept. */ + watch_num_str(b[0], sizeof b[0], last); + k = snprintf(vd, (size_t)vcap, "n=0 last=%s", b[0]); + } else { + watch_num_str(b[0], sizeof b[0], lo); + watch_num_str(b[1], sizeof b[1], hi); + watch_num_str(b[2], sizeof b[2], last); + watch_num_str(b[3], sizeof b[3], sum / n); + k = snprintf(vd, (size_t)vcap, "n=%lld min=%s max=%s last=%s mean=%s", + (long long)n, b[0], b[1], b[2], b[3]); + } + if (k < 0) return 0; + if ((uint64_t)k > vcap - 1) return vcap - 1; + return (uint64_t)k; +} + /* ── Reading the table back ─────────────────────────────────────────── */ /* How many slots have ever been claimed. Only grows, so a reader walking @@ -664,15 +824,22 @@ int flan_dev_watch_read(uint32_t i, char *nd, uint64_t ncap, for (int attempt = 0; attempt < 64; attempt++) { uint64_t g1 = __atomic_load_n(&s->gen, __ATOMIC_ACQUIRE); if (g1 & 1) continue; /* a write is in progress */ + /* An accumulator slot carries five doubles rather than rendered text, so + * what is copied under the seqlock is the numbers; the formatting happens + * *after* the counter check, out of locals, which is why it is safe to do + * it here at all. See "A number sampled thousands of times a frame". */ + int isnum = __atomic_load_n(&s->num, __ATOMIC_RELAXED); + double an = s->n, alo = s->lo, ahi = s->hi, alast = s->last, asum = s->sum; size_t n = __atomic_load_n(&s->len, __ATOMIC_RELAXED); if (n > WATCH_VAL) n = WATCH_VAL; /* a torn read cannot overrun */ if ((uint64_t)n > vcap) n = (size_t)vcap; - memcpy(vd, s->val, n); + if (!isnum) memcpy(vd, s->val, n); /* Ordered before the second read of the counter, or the check is of a copy * the compiler was free to make afterwards. */ __atomic_thread_fence(__ATOMIC_ACQUIRE); if (__atomic_load_n(&s->gen, __ATOMIC_ACQUIRE) == g1) { - *vlen = (uint64_t)n; + *vlen = isnum ? watch_render_num(an, alo, ahi, alast, asum, vd, vcap) + : (uint64_t)n; return 1; } } diff --git a/test/programs/dev-watch.flan b/test/programs/dev-watch.flan index 69f45c1..5283a5d 100644 --- a/test/programs/dev-watch.flan +++ b/test/programs/dev-watch.flan @@ -15,9 +15,20 @@ (declare-c watch-i64 [name string x i64] i32 "flan_dev_watch_i64") (declare-c watch-f64 [name string x f64] i32 "flan_dev_watch_f64") (declare-c watch-str [name string s string] i32 "flan_dev_watch_str") +;; The accumulator, for a value sampled from inside a hot loop. A scalar watch +;; there shows whichever iteration happened to run last, which is the case this +;; exists for; see flan_dev.c, "A number sampled thousands of times a frame". +(declare-c watch-num-i64 [name string x i64] i32 "flan_dev_watch_num_i64") (defvar ticks i64) +(defn loop-cells [] i32 + (let [i 0] + (while (< i 8) + (watch-num-i64 "cell" (i64 (* i 3))) + (set i (+ i 1))) + i)) + (defn step [] i64 (set ticks (+ ticks 1)) ;; Three types, because the table stores *rendered text* and the rendering @@ -26,6 +37,11 @@ (watch-i64 "ticks" ticks) (watch-f64 "half" (/ (f64 ticks) 2.0)) (watch-str "label" "sand") + ;; A hot inner loop, and the value *varies* across it — which is what makes + ;; the row a test of the accumulator rather than of the plumbing. A slot that + ;; only kept [last] would report 21 and no range; n, min and max are each + ;; pinned by a different part of this loop. + (loop-cells) ticks) (defn main [] i32 diff --git a/test/test_dev.ml b/test/test_dev.ml index a4d71d9..6eab670 100644 --- a/test/test_dev.ml +++ b/test/test_dev.ml @@ -1956,6 +1956,72 @@ let () = | Some "\"sand\"" -> () | Some v -> fail "watch rendered a string as %s, unquoted" v | None -> fail "watch lost the string"); + (* The accumulator, which is the other half of the watch and the + half a scalar row cannot stand in for. [loop-cells] samples "cell" + eight times per step at 0, 3, ... 21, so the row has to show a + *range* and a count far above the number of steps. A slot that kept + only the last sample would say 21 and nothing else, and a slot that + counted steps rather than samples would say a number near "ticks". + See flan_dev.c, "A number sampled thousands of times a frame". *) + let stat row key = + let parts = String.split_on_char ' ' row in + List.find_map + (fun p -> + let k = key ^ "=" in + let n = String.length k in + if String.length p > n && String.sub p 0 n = k then + float_of_string_opt (String.sub p n (String.length p - n)) + else None) + parts + in + (match List.assoc_opt "cell" t with + | None -> fail "the accumulator never reached the watch table" + | Some row -> + (match stat row "n", stat row "min", stat row "max" with + | Some n, Some lo, Some hi -> + (* More samples than there were steps: the loop is being counted + per iteration, which is the whole reason this is not a scalar + watch. *) + if n < 8.0 then fail "the accumulator counted %g samples" n; + (* And a range, which is what one sample can never show. *) + if not (lo < hi) then + fail "the accumulator kept no range: min=%g max=%g" lo hi + | _ -> fail "the accumulator rendered as %s" row)); + (* And the window is the editor's to close. [:reset t] starts a new + one, so the count drops to what the program has done since — + which is what makes min and max track the present instead of + reaching the session's extremes and going dead. A read *without* + it must not reset, or anything that polls would cut the window + short under the editor that owns it. *) + let cell_n () = + match List.assoc_opt "cell" (table ()) with + | Some row -> stat row "n" + | None -> None + in + (* Let it run up first. One step writes eight samples, so two reads + back to back leave no room under the count for a reset to show in — + the assertion needs a window with something in it. Polling to get + there is itself the other half of the claim: every one of these + reads is a plain [watch], and a plain [watch] must not reset, or + the count could never climb at all. *) + if not (await (fun () -> + match cell_n () with Some n -> n > 32.0 | None -> false)) + then fail "the accumulator never ran up; a plain read must not reset" + else begin + let high = match cell_n () with Some n -> n | None -> 0.0 in + ignore (ask "(:op \"watch\" :reset t)"); + (* Waited for, not read once. A reset moves one counter and clears no + slot — the slot notices on its *next sample*, which is the game + thread's next time round the loop. That is the design and not a + delay to work around: the reader never writes the table, so the + game thread stays its only writer. The cost is that the new window + begins when the program next runs, which for a frame loop is the + only moment it could sensibly begin anyway. *) + if not (await (fun () -> + match cell_n () with Some n -> n < high | None -> false)) + then fail "the window never reopened after a reset; it stayed at %g" + high + end; (* It keeps moving. A table that filled once and froze would pass everything above and be useless — the counter has to be the program's, not a snapshot the daemon took when it armed. *) diff --git a/vendor/agent/flan_agent.c b/vendor/agent/flan_agent.c index 523b960..82e5604 100644 --- a/vendor/agent/flan_agent.c +++ b/vendor/agent/flan_agent.c @@ -82,6 +82,10 @@ uint64_t flan_dev_result_cap(void); * opening and closing a watch buffer is a message that arrives here. */ void flan_dev_watch_enable(int on); int flan_dev_watch_enabled(void); +/* And [reset], which opens a new accumulation window for the numeric slots. + * It moves one counter and touches no slot, so the game thread stays the only + * writer of the table. */ +void flan_dev_watch_reset(void); uint32_t flan_dev_watch_count(void); int flan_dev_watch_overflowed(void); int flan_dev_watch_read(uint32_t i, char *nd, uint64_t ncap, @@ -894,6 +898,21 @@ static void handle_line(char *line, sink *o) { reply(o, "ok\n"); return; } + /* "watch reset" opens a new window for the numeric accumulators: their + * count, range and mean start again from the next sample, while a text slot + * is untouched. + * + * A separate command rather than a side effect of the read, which was the + * tempting shape and is the wrong one. A destructive read makes *looking* + * change what is there, so anything that polls — a test's [await], a second + * editor, a person pressing the read twice — silently shortens the window + * and gets a count that is noise. Reading is free and resetting is a + * decision; the editor makes it once per tick, after the read. */ + if (strcmp(line, "watch reset") == 0) { + flan_dev_watch_reset(); + reply(o, "ok\n"); + return; + } if (strcmp(line, "watch") == 0) { /* One line per slot: NAMEVALUE. The name cannot contain a tab — it is * a C identifier-ish string the program passed — and the value cannot