The 4K cap was a render budget, and only its second copy was transport
RESULT_MAX was written down twice, once in flan_dev.c and once in the agent, with a run-time check that the two had not drifted. That second copy was a buffer sized to be sent through a socket, and it is gone: the agent asks flan_dev_result_cap() and allocates, which it can do because it is never the game thread. The bound itself stays, and the reason is the rule everything else here is built around. result is what the game thread writes into, from a render thunk at a frame boundary; a growable one is the frame thread calling realloc, which is an allocation in the one place this design exists to keep allocation out of. It would also break the seqlock, which is a protocol about torn contents and assumes the address it copies 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 and not a wire size, and it only looked like one because the agent had a copy of it. Removing the bound is a redesign of the read -- probe, allocate, re-read, validate, retry -- and belongs with moving the read to a frame boundary. Also: flan_agent_request answers "err path too long" where the socket does, instead of nothing. One verb table is only worth having if the two callers cannot be told apart. The measurement table in BUILT.md is retaken back to back across all four shapes, because this machine drifts by more than the thing being measured. The internal socket was ~41us of a 21ms redefinition; --two-process is unchanged and is not slower than the merged build in any column.
This commit is contained in:
parent
65a62bbb97
commit
f63974142f
54
BUILT.md
54
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
|
||||
|
||||
9
NEXT.md
9
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
|
||||
|
||||
@ -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,
|
||||
|
||||
43
vendor/agent/flan_agent.c
vendored
43
vendor/agent/flan_agent.c
vendored
@ -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);
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user