perf: smoother solve cold-start — warming-up UX + live-frame display fix - #23
Conversation
…onds)
Diagnosed the run-orchestrator lag: it's all cold-start — ~80s Julia/Piccolo
load + first-iter JIT, plus a ~35s first plot_pulse (Makie compiles its render
stack once). Steady state is fine (~1.5-3s/iter, 0.2-0.9s plots).
Fix: a PackageCompiler sysimage baking Piccolo + CairoMakie + the solve/plot
code paths (build_sysimage.jl + sysimage_exercise.jl). install.sh builds it
best-effort at <project>/amico-sysimage.{dylib,so}; amico-run auto-detects a
sysimage beside --project and passes --sysimage (no agent/flag change). Falls
back cleanly to no-sysimage (slow first run) if the build is skipped/fails.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…tart) - Inspector shows 'Julia warming up — first solve compiles (~1-2 min)' when a run has started but emitted no frame yet, instead of an idle panel, so the cold start doesn't read as frozen. Buffered like the image/completion so it shows even if the panel opens late; replaced by the first frame. - Template skips the iter-0 plot (just the random init) — defers Makie's first-plot compile off the very first iteration when there's no sysimage. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ll step The PackageCompiler build (CairoMakie native-compile) is a long one-time cost (~25-50 min, machine-dependent) — too slow to sit on the install critical path (β.4's <=60min target). Flip it to opt-in: install.sh builds it only with AMICO_BUILD_SYSIMAGE=1 (else prints the tip), plus a 'pnpm --filter amicode-v2 sysimage' script. amico-run auto-detects it once present; until then solves pay the cold start and the inspector shows 'warming up'. One-time + per-machine (native artifact — not shippable prebuilt). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…st the first) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…watch Live frames/FINISHED were detected only via fs.watch (FSEvents), which coalesces and silently drops events under CPU load → the inspector could sit on 'warming up' while iter_*.png frames pile up on disk unrendered (reported: 'ran but not plotting', frames present + valid on disk). Add a 700ms polling backstop to RunsRootWatcher: re-resolve latest, rescan the active run for the newest frame + FINISHED, and poke the log tailer. fs.watch stays for low latency; the poll guarantees delivery. All sinks are idempotent (frame dedup by iter, finishedSeen guard, log byte-offset) so the two paths never double-deliver. LogTailer.poke() drains only after attach() sets the start offset (never re-reads replayed lines). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ERY 6 - A new run's 'warming up' state now clears the previous run's image + stats, so the old iter-N plot no longer lingers on screen while the new solve compiles (reported: 'displaying iter 60 from the old one as the new iters run'). - Template plots every 6 iters (was 10) for more frequent live frames. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ck review nits The real "plots never display" bug: LiveRunSink shared one counter between image() and iter(). run.log AMICODE_ITER lines stream once per iteration and race the counter to max BEFORE the lagging PNG frames land (solver logs iter=k, then writes iter_k.png), so every image() hit `k <= latest` and was dropped — blank inspector for the whole solve. Split into SinkDedup: frames dedup only against frames; log lines advance a separate high-water mark for the status bar. Extracted SinkDedup to the pure, vscode-free run_dir_reader layer so the live path is finally unit-tested (Jack #9: "no test covering the live status-bar / incremental inspector path" — the exact gap this regression slipped through). Also folds in the remaining non-blocking review nits (kept here rather than restacking the 7-deep chain to place one-liners on their home branches): - #13: #runlabel was styled but never populated → setRunLabel + webview handler. - #12: resolveJuliaProject now expands a leading ~ (parity with resolveRunsRoot). - #9: server_manager ServerOptions.env comment no longer cites the removed AMICODE_EXTENSION_URL/MCP-callback env. - #9/#11: AGENTS.md step-2 author path now matches step-3 run path (/tmp/amicode-work/solve.jl) and the invocation passes --lab default (run provenance — amico-run supports --lab). Plus the cold-start display polish already in flight on this branch: launch stays idle for a prior finished run (no stale plot), and the placeholder toggles via style.display (the [hidden] attr was overridden by .placeholder{display:flex}). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ipped) Jack #11 must-change ("bundle a real run.log") never actually landed: the demo run.log is matched by .gitignore's `*.log`, so it was never committed. .vscodeignore has `!demo/**` so it'd ship IF present on disk — meaning it only "worked" on the capture machine; a clean checkout / CI VSIX shipped the demo with NO run.log → blank stats row, exactly the failure he flagged. Un-ignore the asset (.gitignore negation), track it, and assert it in the packaging manifest test so a future drop is caught. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Recording the #9 ↔ #23 link for the record. On #9 I flagged the absence of a test for the live status-bar / incremental inspector path. That gap was masking a real bug: at #9's head #23 is what makes chat→solve→inspector actually work end-to-end:
Net: #9 and #23 are a unit — #9 on its own ships a blank inspector. So the live-path test gap on #9 reads as "covered downstack in #23," not unaddressed. |
The on-machine PackageCompiler build (CairoMakie native-compile dominated,
~25-50 min, per-machine, non-shippable) wasn't worth it even opt-in. Removed:
- julia/build_sysimage.jl + julia/sysimage_exercise.jl (the build scripts)
- the `sysimage` npm script
- the install.sh build block (AMICO_BUILD_SYSIMAGE gating)
- amico-run's auto-detect of <project>/amico-sysimage.{dylib,so} + its test
Kept the generic `--sysimage <path>` flag + manifest plumbing (already on main
from β.1): it's the passive hook for the intended fast-path — a prebuilt
sysimage distributed like Piccolissimo's (CI build on self-hosted binary-builder
runners → tarball+sha256 → Cloudflare R2 → manifest.json → download), pointed at
via the flag. That's a separate effort; a Piccolissimo image can't be reused
directly since amicode's needs Piccolo + CairoMakie baked in.
Until that exists, every solve pays the cold start and the inspector shows
"warming up" — which is now the whole of this PR's perf story.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Rchari1
changed the title
jack-champagne
left a comment
jack-champagne
left a comment
There was a problem hiding this comment.
This is the keystone and the code is correct — almost ready to go. The frame/log dedup (SinkDedup), the poll backstop, the AGENTS step-2/3 path reconciliation, the bundled run.log, and the #runlabel wiring close the open findings from #9/#11/#13, and the placeholder.hidden→display:none change squashes a real latent bug (the placeholder was never actually hiding — .placeholder{display:flex} out-specifies [hidden]). Two changes before merge:
- Retitle. The final commit removed the sysimage, so "perf: smoother solve cold-start" no longer fits — the cold start itself is unchanged; what's left is a warming-up indicator plus the folded-in review fixes. Something like "inspector: live-frame fix + warming-up state; fold review fixes; drop local sysimage."
- One watcher-integration test (or a tracked follow-up if you'd rather not block the unit).
SinkDedupis now unit-tested — good — butRunsRootWatcher's poll / idle-on-finished baseline / warming-up transition has zero coverage and is the most intricate code in the stack. Inline notes below.
Everything else is minor (inline). This is the PR that makes the whole stack function end-to-end — almost there.
|
|
||
| /** fs.watch backstop: re-resolve `latest`, then rescan the active run for new | ||
| * frames / FINISHED and drain the log — catching anything FSEvents dropped. */ | ||
| private tick(): void { |
There was a problem hiding this comment.
[important] This poll backstop — plus the idle-on-finished baseline in start() and the warming-up transition — is the most intricate new code in the release, and none of it has a test. The new SinkDedup unit test covers the dedup primitive, not this state machine. One test that appends iter_*.png + AMICODE_ITER lines to a temp run dir over time and asserts the sink/inspector calls (incl. a finished run at launch staying idle, and warming → first frame) would close it. Fine as a tracked follow-up if you don't want to block the unit — but it's the riskiest untested code in the stack.
There was a problem hiding this comment.
Done in e83d76a — added test/watcher_statemachine.test.ts covering exactly this. It drives RunsRootWatcher over a temp run dir and asserts the inspector calls: (a) a run already FINISHED at launch stays idle (no stale plot rendered, even after a poll tick); (b) a fresh run shows warming-up, then the poll delivers the newest frame, then completion fires once. Infra to make the vscode-coupled watcher testable: a minimal vscode stub aliased in vitest.config.ts, the inspector mocked to capture calls, and tick() driven directly so the poll path is deterministic (no 700ms-timer race). 49 tests green.
| const m = ITER_PNG_RE.exec(f); | ||
| if (m) { const k = parseInt(m[1], 10); if (k > newest) { newest = k; newestPath = path.join(runDir, f); } } | ||
| } | ||
| if (newestPath) this.sink.image(newestPath, newest); // deduped by lastFrameIter |
There was a problem hiding this comment.
[minor] The poll delivers only the newest frame per tick, so frames produced between two ticks are skipped on this path. Correct for "show the latest pulse" (not an animation) — worth a one-line comment saying so, since otherwise it reads like a dropped-frame bug.
There was a problem hiding this comment.
Good call — added a comment in e83d76a: the poll intentionally delivers only the newest frame per tick (latest pulse, not an animation; the fs.watch path still catches most frames at low latency), so a coalesced frame is no loss. The new state-machine test also asserts the newest-wins behavior explicitly (two frames between ticks → only iter_18 delivered).
| @printf("AMICODE_ITER iter=%d f=%.6e inf_pr=%.3e inf_du=%.3e\n", k, st.obj_value, st.inf_pr, st.inf_du) | ||
| flush(stdout) | ||
| (k % PLOT_EVERY == 0) && save_control_plot(k) | ||
| (k > 0 && k % PLOT_EVERY == 0) && save_control_plot(k) # skip iter-0 (just the random init; defers Makie's first-plot compile off the first iter) |
There was a problem hiding this comment.
[minor] skip-iter-0 + PLOT_EVERY=6 means a solve converging in <6 iters emits no per-iter frame — the inspector shows "warming up" until the end-of-solve guarantee frame. Fine, just noting the edge; the warming-up state covers it.
There was a problem hiding this comment.
Noted in e83d76a — added a comment at PLOT_EVERY flagging the <6-iter edge (no per-iter frame; the end-of-solve guarantee frame + the warming-up state cover it). No behavior change.
[important] Add a test for the live RunsRootWatcher state machine — the poll
backstop + idle-on-finished-at-launch baseline + warming→first-frame transition,
which the SinkDedup unit test (the dedup primitive) did not cover. The test drives
the watcher over a temp run dir and asserts the inspector calls:
- a run already FINISHED at launch stays idle (no stale plot re-rendered),
- a fresh run shows warming-up, then the poll delivers the newest frame
(newest-wins across a tick), then completion fires once.
Infra: a minimal `vscode` stub aliased via vitest.config.ts so node-side modules
that import vscode are unit-testable; the inspector is mocked to capture calls;
tick() is driven directly so the poll path is deterministic (no 700ms race).
[minor] file_watcher.ts: comment that the poll delivers only the newest frame per
tick by design (latest pulse, not an animation) — not a dropped-frame bug.
[minor] solve_template.jl: note the <PLOT_EVERY-iter edge (a solve converging in
<6 iters emits no per-iter frame; the warming-up state covers it).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks @jack-champagne — all three addressed in e83d76a:
49 tests green. Re-requesting review. |
jack-champagne
left a comment
jack-champagne
left a comment
There was a problem hiding this comment.
All three addressed in e83d76a — verified against the diff, not just the replies:
- [important] live state-machine test:
test/watcher_statemachine.test.tsdrives the real RunsRootWatcher over a temp run dir, calling tick() directly for determinism. Asserts the three states I flagged — finished-at-launch stays idle (no stale frame), fresh run → warming-up + run label, poll backstop delivers newest frame (newest-wins on coalesced ticks), then terminal completion. No external-binary dependency, so it actually runs in CI (unlike the #22 merge test). - [minor] newest-frame-per-tick comment added.
- [minor] <6-iter edge comment added.
Keystone verified-correct and validated live. Resolves #27. Good to merge.
2 participants
Makes the live-solve experience smoother. Stacked on #22; base
rchari/qa-chat-fixes. Do not merge.Diagnosis (measured)
The run-orchestrator lag is entirely cold-start, not the solver or the watcher:
plot_pulsecall (CairoMakie compiles its render stack — one-time)amico-run spawns a fresh Julia per solve, so every cold solve pays this — not just the first.
Cold-start UX (the perf story here)
Live-frame display fix (the "plots never show" bug)
LiveRunSinkshared one iter counter between frame display and log-line parsing. Therun.logAMICODE_ITERlines stream once per iteration and race the counter to max before the lagging PNG frames land (the solver logsiter=k, then writesiter_k.png), so everyimage()hitk <= latestand was deduped away → blank inspector for the whole solve.SinkDedup(run_dir_reader.ts): frames dedup only against frames; log lines advance a separate high-water mark for the status bar.Folded-in review follow-ups
Kept here rather than restacking the 7-deep chain for one-liners (flagged on each PR thread):
#runlabelpopulated (setRunLabel+ webview handler, wired from the watcher).resolveJuliaProjectexpands a leading~(parity withresolveRunsRoot) + test.server_managerServerOptions.envcomment no longer cites the removedAMICODE_EXTENSION_URL/MCP env; AGENTS.md invocation passes--lab default./tmp/amicode-work/solve.jl).run.logwas*.log-gitignored and never committed (demo shipped a blank stats row); un-ignored + tracked + asserted in the packaging manifest test.No local sysimage build
An earlier rev added an opt-in PackageCompiler sysimage build to collapse the cold start. Removed — the native compile (CairoMakie-dominated, ~25–50 min, per-machine, non-shippable) wasn't worth it even opt-in. The generic
amico-run --sysimage <path>flag + manifest plumbing (already on main from β.1) stays as the hook for the intended fast-path: a prebuilt sysimage distributed like Piccolissimo's — CI build on self-hosted binary-builder runners → tarball+sha256 → Cloudflare R2 →manifest.json→ download. That's a separate effort (a Piccolissimo image can't be reused directly — amicode's needs Piccolo + CairoMakie baked in).Tests
47 extension + 45 amico-run green (incl.
SinkDeduplive-path tests,~-expansion, demo run.log packaging assertion).Note for later
To actually eliminate the ~80 s cold start, the two levers are (a) the prebuilt-sysimage download above, or (b) a warm Julia daemon (keep one Piccolo-loaded process alive, send it solves) — bigger amico-run change, no build cost.
🤖 Generated with Claude Code