diff --git a/BUILT.md b/BUILT.md index 75d8366..622629f 100644 --- a/BUILT.md +++ b/BUILT.md @@ -1062,28 +1062,54 @@ in the old code and half in the new, and it would be an easy thing to do by acci the loader lock — and holding OCaml's lock through it stalls every other OCaml thread. DISCUSS.md §14's third cost, in the one place this change creates it. -**What it is worth, measured** (this machine, warm caches, a one-`defn` redefinition over the editor socket, median of -12): +**What it is worth, measured.** This machine, warm caches, a one-`defn` redefinition driven over the editor socket, +median of 12; the four columns taken back to back in one sitting, because the run-to-run drift on this machine is +larger than what is being measured: -| | before | after | -|---|---|---| -| redefinition, end to end | 23.2ms | 22.0ms | -| ...of which the build (`:ms`) | 20.4ms | 19.0ms | -| a `break` round trip (editor socket + one agent ask) | 0.069ms | 0.019ms | +| | merged, before | merged, after | `--two-process`, before | `--two-process`, after | +|---|---|---|---|---| +| redefinition, end to end | 22.1ms | 21.2ms | 20.8ms | 22.1ms | +| ...of which the build (`:ms`) | 19.5ms | 18.8ms | 18.4ms | 19.5ms | +| a `break` round trip | 0.061ms | **0.020ms** | 0.060ms | 0.064ms | -**The transport was about 50µs of a 23ms redefinition, and removing it does not move that number.** That is the finding, -and it is worth more than a speedup would have been: the end-to-end column moved by less than its own run-to-run spread, -and `--two-process` measures 21.6ms — marginally *faster* than the merged build did before this change. **The merge's -prize was never latency.** It is that the compiler and the program now share an address space, which is what makes the -items below deletable at all and what unblocks reading the stopped frame's memory directly. Anyone reaching for an -in-process JIT on the strength of "transport is slow" should read this row first: code generation is 19 of the 22 -milliseconds. +The `break` row is the editor socket *plus* one question to the agent, so the ~41µs it lost is the whole of the +internal socket. Everything else is noise: the redefinition column moves by less than its own spread and moves in both +directions. + +**The transport was about 40µs of a 21ms redefinition, and removing it does not move that number.** That is the +finding, and it is worth more than a speedup would have been. `--two-process` is not slower than the merged build in +any column — it measured *faster* on both sides of this change — so **the merge's prize was never latency.** It is that +the compiler and the program share an address space, which is what makes the items below deletable at all and what +unblocks reading the stopped frame's memory directly. Anyone reaching for an in-process JIT on the strength of +"transport is slow" should read the build row first: code generation is 19 of the 21 milliseconds, and the socket was +0.2% of it. The test that pins it is a deletion, because a reply cannot say which way it came: `test_dev.ml` **unlinks the agent's socket file** once the merged program has bound it, and then runs every evaluation in the file. Unlinking a bound unix socket does not disturb the listener, it makes new connects fail — so if the deliveries still install, nothing connected. The agent still binds it, for `--two-process` and for a person at a raw socket. +#### The 4K result cap is not a transport buffer, and it stays + +Next on the list, and the answer is no — with half of it deleted anyway, which is the useful part. + +`RESULT_MAX` reads like a wire size: 4096 bytes, a cap on a value the compiler reads back after `C-x C-e`. It was +written down **twice**, once in `runtime/flan_dev.c` and once in `vendor/agent/flan_agent.c`, with a run-time check +that the two had not drifted. That second copy *was* transport — a buffer sized to be sent through a socket — and it +is gone: the agent asks `flan_dev_result_cap()` and allocates, so the bound is one file's decision now and the +drift check has nothing left to check. + +**The bound itself cannot go, and the reason is the rule the merge was built around.** `result` is the buffer the +**game thread** writes into, from a render thunk at a frame boundary. A growable one means the frame thread calling +`realloc` — an allocation in the one place this design exists to keep allocation out of. And it would break the +seqlock, which the premise for this work correctly says must stay: a seqlock is a protocol about torn *contents*, and +it assumes the address it `memcpy`s from neither moves nor goes away underneath the reader. Growing on the writer's +side is a use-after-free the counter cannot see. + +So it is a render budget, not a wire size, and it was only ever mistaken for one because the agent had a copy of it. +Removing the bound is a redesign of the *read* — probe the length, allocate, re-read, validate the generation, retry — +and it belongs with moving the read to a frame boundary, which is the seqlock's own decision and its own lane. + ### The Emacs client `emacs/flan-mode.el` derives from `prog-mode` with `lisp-mode`'s syntax table, which is most of the work: Flan is diff --git a/NEXT.md b/NEXT.md index f68de04..c125d38 100644 --- a/NEXT.md +++ b/NEXT.md @@ -535,9 +535,12 @@ What it reopened is now being deleted one piece at a time, each with its own gre call into the agent's verb table. Measured: the transport was ~50µs of a 23ms redefinition, so the end-to-end number did not move. Code generation is 19 of the 22 milliseconds, which is the number any backend argument has to start from. -2. **The 4K `RESULT_MAX` cap** in `runtime/flan_dev.c`. It exists to size a transport buffer. **The seqlock does not - go with it** — the game thread still writes and the compiler thread still reads, so that race is real in one - process too. +2. ~~**The 4K `RESULT_MAX` cap** in `runtime/flan_dev.c`.~~ **Refused, with half of it deleted** — it is not a + transport buffer. The agent's second copy of the number and the drift check between the two files were, and those + are gone. The bound itself is the buffer the *game thread* writes into, so a growable one means the frame thread + calling `realloc`, and it would break the seqlock — which is a protocol about torn contents and assumes the address + it copies from does not move. Removing it is a redesign of the read, and belongs with moving the read to a frame + boundary. 3. **`flan_agent.c`'s snapshot copying and generation stamping.** The compiler can read the stopped frame's memory directly. 4. **The render-thunk-per-inspection design for locals and globals.** A redesign rather than a deletion, and its own diff --git a/runtime/flan_dev.c b/runtime/flan_dev.c index 869a01a..dfdc41e 100644 --- a/runtime/flan_dev.c +++ b/runtime/flan_dev.c @@ -141,6 +141,19 @@ void *flan_dev_global(const char *name, uint64_t size, const void *init) { * while the game thread was free to be a hundred bytes into the next value. * A seqlock cannot validate a read that happens after it returns. */ +/* Fixed, and it stays fixed. This was expected to go with the socket — a 4K + * cap reads like a transport buffer — and it is not one. This is the buffer + * the *game thread* writes into, from a render thunk at a frame boundary, and + * a growable one would mean the frame thread calling realloc: an allocation in + * the one place this whole design exists to keep allocation out of. It would + * also break the seqlock above, which is a protocol about torn *contents* and + * assumes the address it memcpys from does not move or go away underneath it; + * growing on the writer's side is a use-after-free the counter cannot see. + * + * So the cap is a render budget, not a wire size, and what did go is the + * agent's second copy of the number. Removing the bound itself is a redesign + * of the read — probe, allocate, re-read, validate, retry — and belongs with + * moving the read to a frame boundary, which is the seqlock's own decision. */ #define RESULT_MAX 4096 static char result[RESULT_MAX]; static size_t result_len; @@ -265,12 +278,12 @@ void flan_dev_result_end(void) { * and the reader is the listener thread, which has nothing better to do. * * [cap] is the caller's buffer. A value longer than it is truncated, which is - * the only failure this can have and is a clamp rather than an overrun; the - * agent sizes its buffer at RESULT_MAX so it does not arise. */ -/* What a caller's buffer has to be for the copy never to be truncated. The - * bound is declared in one place and asked for rather than written down twice: - * the agent's buffer and this one agreeing is the whole of "never truncated", - * and two literals in two files is how that stops being true. */ + * the only failure this can have and is a clamp rather than an overrun; every + * caller sizes its buffer from [flan_dev_result_cap] so it does not arise. */ +/* What a caller's buffer has to be for the copy never to be truncated. One + * declaration, asked for rather than written down twice — the agent carried + * its own copy of the number and a check that the two had not drifted, back + * when it was sizing something to send through a socket. */ uint64_t flan_dev_result_cap(void) { return RESULT_MAX; } int flan_dev_result_read(char *dst, uint64_t cap, uint64_t *gen, diff --git a/vendor/agent/flan_agent.c b/vendor/agent/flan_agent.c index 60721af..450cce7 100644 --- a/vendor/agent/flan_agent.c +++ b/vendor/agent/flan_agent.c @@ -60,11 +60,17 @@ typedef void (*install_fn)(void); typedef void (*call_fn)(void); /* The value of the last evaluated expression, copied out under the seqlock in - * runtime/flan_dev.c rather than borrowed. The buffer below is RESULT_MAX, so - * the copy is never truncated; a read that loses the race reports the last - * complete generation and no bytes, which leaves the daemon polling rather - * than showing it half a value. */ -#define RESULT_MAX 4096 + * runtime/flan_dev.c rather than borrowed: a reader gets bytes of its own, and + * a read that loses the race reports the last complete generation and no + * bytes, which leaves the compiler polling rather than showing it half a + * value. + * + * How big that copy has to be is the runtime's number, and it is asked for + * rather than written down here as well. This file used to carry its own copy + * of the bound and a check that the two had not drifted — which is what you do + * when one of them is sizing a buffer to send through. Nothing here sends + * anything any more, so the bound belongs to whoever owns the storage, and + * changing it is one file's decision. */ int flan_dev_result_read(char *dst, uint64_t cap, uint64_t *gen, uint64_t *len); uint64_t flan_dev_result_cap(void); @@ -865,16 +871,14 @@ static void handle_line(char *line, sink *o) { } if (strcmp(line, "result") == 0) { uint64_t gen = 0, len = 0; - char v[RESULT_MAX]; - /* The one thing that could make the copy truncate, checked where it - * would happen rather than trusted to two files holding the same - * number. */ - if (flan_dev_result_cap() > sizeof v) { - reply(o, "err the agent's result buffer is smaller than the " - "runtime's\n"); - return; - } - flan_dev_result_read(v, sizeof v, &gen, &len); + uint64_t cap = flan_dev_result_cap(); + char *v = malloc(cap ? (size_t)cap : 1); + if (v == NULL) { reply(o, "err out of memory reading the result\n"); return; } + /* Allocating here is fine and is the distinction that matters: this runs + * on the listener thread, or on the compiler thread in a merged build. + * What the game thread writes into is a fixed static in flan_dev.c + * precisely so that *it* allocates nothing. */ + flan_dev_result_read(v, cap, &gen, &len); char hdr[64]; int k = snprintf(hdr, sizeof hdr, "%llu %llu\n", (unsigned long long)gen, (unsigned long long)len); @@ -882,6 +886,7 @@ static void handle_line(char *line, sink *o) { emit(o, hdr, (size_t)k); if (len > 0) emit(o, v, (size_t)len); } + free(v); return; } /* Before the dlopen, not after it: a module there is no room to queue is @@ -989,8 +994,12 @@ char *flan_agent_request(const char *line, uint64_t *len) { sink o = { -1, NULL, 0, 0 }; size_t n = strlen(line); if (n >= sizeof stack) { - *len = 0; - return NULL; + /* The same answer the socket gives for the same input. The point of one + * verb table is that the two callers cannot be told apart, and a silent + * empty reply here would be the first place they could. */ + reply(&o, "err path too long\n"); + *len = (uint64_t)o.len; + return o.buf; } memcpy(stack, line, n + 1); pthread_mutex_lock(&request_lock);