From 74426f60a676cbee3e30a3aef86e409f6d9c5cee Mon Sep 17 00:00:00 2001 From: Ken Simpson Date: Tue, 21 Apr 2026 09:59:57 -0700 Subject: [PATCH] docs: add modal performance plan --- docs/LEARNINGS.md | 1 + docs/UPNEXT.md | 37 ++++++++++++++++++++++++++++++++++--- 2 files changed, 35 insertions(+), 3 deletions(-) diff --git a/docs/LEARNINGS.md b/docs/LEARNINGS.md index 7e5f3a8..7afef98 100644 --- a/docs/LEARNINGS.md +++ b/docs/LEARNINGS.md @@ -169,3 +169,4 @@ - Frontend mapped-progress lanes cannot be keyed only by `laneId` at prompt scope. Different remote components legitimately reuse local worker lane numbers like `0` and `1`, so the UI has to namespace lane ownership by component representative or concurrent components will clear each other's purple bars and appear to flicker between active and ready states. - Streamed remote progress should suppress native proxy `executing` churn, but not the proxy's terminal completion. If the frontend drops every native phase update after streamed progress starts, a single-node remote component can stay stuck purple because it only ever receives streamed `finalizing` and never consumes the completion event that should turn it green. - The in-process "already auto-deployed" cache cannot be treated as authoritative once operators can delete the Modal app out-of-band. A later `app not found` lookup has to invalidate that cache entry and redeploy instead of assuming the earlier successful deploy still exists remotely. +- Combined first-run and second-run performance logs are now clear enough to prioritize work. In this workflow, uploads and volume reload were already cheap, while the recurring non-sampling bottleneck was static-phase model loading (`UNETLoader` + `CLIPLoader` at roughly `24-25s` total per run). The second run also showed that local proxy caching can skip the static proxy dispatch while still leaving a large replay cost inside the mapped phase, which makes two no-brainer optimizations obvious: reuse heavy loader outputs on the same worker, and avoid replaying the whole static producer branch just to reconstruct session-backed bridge values. diff --git a/docs/UPNEXT.md b/docs/UPNEXT.md index a91339c..ed22cf1 100644 --- a/docs/UPNEXT.md +++ b/docs/UPNEXT.md @@ -45,7 +45,38 @@ Partial-completion semantics for one proxy would require ComfyUI scheduler behav - Split-phase caching and batching have been hardened around the real workflow edges we saw online: provenance-based cache keys for non-transportable boundary inputs, correct handling of local `ModalMapInput` markers, broadcast `CONDITIONING`, per-item session-ref batches, primitive over-list inputs, singleton semantic lists, and `INPUT_IS_LIST` targets. - The frontend and observability paths are also in better shape: component-scoped progress lanes no longer flicker, single-node components clear their purple state correctly, and `COMFY_MODAL_STREAM_REMOTE_CONTAINER_LOGS=true` can mirror remote container logs into local stderr during streamed runs. -### Remaining work +### Performance findings -- Keep watching mirrored live logs under broader scale-out for real session misses or excessive replay after container churn. -- If production still shows too many split-proxy session misses, strengthen worker/session routing instead of piling on more replay logic. The likely next step is stronger affinity or an explicit leased-worker/session model rather than best-effort class-instance affinity. +- Upload and sync overhead are already small in the captured runs. Queue-time rewrite and sync took about `1.25s` on the first run and `0.214s` on the second run, and the custom-nodes bundle was already mirrored in both runs. +- Volume reload is not currently a material cost for this workflow. Both runs logged `Skipping modal_volume_reload ... because no new assets were uploaded`. +- The dominant recurring non-sampling cost is remote model loading inside the static phase. On both runs, `UNETLoader` took about `14.4s` and `CLIPLoader` took about `10.0s`, so roughly `24-25s` per run is spent reloading models even when workflow structure is otherwise unchanged. +- The second run proved that local ComfyUI proxy caching is now helping, but it also showed the next bottleneck clearly: the mapped phase still has to replay the static producer work to rebuild session-backed state before sampling starts. +- Persisted Modal `Dict` caching is helping some small nodes, but it is not touching the dominant cost centers in this workflow because the expensive static loader outputs are not being reused from a warm in-process loader cache. +- Modal container startup is still expensive, but keeping workers hot full-time is not the preferred answer for this project. Performance work should prioritize better reuse within a worker lifecycle and less replay when a worker is already available. + +### Optimization plan + +1. Add warm-worker loader reuse for heavy model nodes. + Focus on `UNETLoader` and `CLIPLoader` first, since the logs show they dominate non-sampling runtime. + Reuse loaded outputs across compatible requests on the same Modal worker using a stable loader-input key and clone-or-safe-share semantics. + Acceptance criteria: on a same-worker repeat run, those loader nodes log `Loader cache hit` instead of `Loader cache miss`, and the static phase drops by roughly the combined `~24s` loader cost seen in the current logs. + +2. Avoid full static-phase replay for session-backed mapped runs when bridge refs are still valid locally. + The second run shows that local proxy caching skipped the static proxy dispatch, but the mapped phase still replays the static producer branch to rebuild session-backed state. + Add a fast path where the mapped phase can resolve durable bridge refs directly into the current session without rerunning the whole static producer subgraph when the required bridge values are already reconstructible from cached durable metadata or retained in-process state. + Acceptance criteria: on an unchanged repeat run, the mapped phase no longer re-enters the full static producer prompt before `KSampler` starts, and time-to-sampling shrinks materially. + +3. Add targeted observability for replay cost breakdown. + Emit explicit timers for bridge-record lookup, replayed static-subgraph execution, session-value restoration, and loader-cache hit/miss counts. + Right now the logs make replay visible, but not cheap to quantify automatically. + Acceptance criteria: one log block per mapped phase shows how much time was spent in direct bridge resolution versus replay and how many heavy loaders were reused. + +4. Reduce remote custom-node initialization churn where possible. + `init_external_custom_nodes` is only about `~1s`, so it is not the first optimization target, but it is still worth trimming after loader reuse lands. + Investigate whether the worker can avoid repeated import-error noise and repeated custom-node init work once the bundle digest is unchanged. + Acceptance criteria: repeated runs on the same worker avoid redundant custom-node initialization work and produce cleaner logs. + +5. Keep monitoring session-routing quality under real scale-out. + This is durability work with performance impact, not the first speed target. + If session churn still forces replay too often under scale-out, the next structural change is stronger worker/session routing or an explicit leased-worker/session model. + Acceptance criteria: mirrored logs show low replay frequency for ordinary repeat workflows, and session misses remain rare under moderate parallel use.