feat(skippy): execute mixed prefill and decode iterations safely - #1456
feat(skippy): execute mixed prefill and decode iterations safely#1456i386 wants to merge 18 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
d243f61 to
fb105aa
Compare
Co-authored-by: Scam <44c96d97d4bda5bbcbd62565b0b19bb2e895e1fad5d9e9116f267909f1dae78e@meshllm.communities.buzz.xyz>
Co-authored-by: Scam <44c96d97d4bda5bbcbd62565b0b19bb2e895e1fad5d9e9116f267909f1dae78e@meshllm.communities.buzz.xyz>
Co-authored-by: Scam <44c96d97d4bda5bbcbd62565b0b19bb2e895e1fad5d9e9116f267909f1dae78e@meshllm.communities.buzz.xyz>
Co-authored-by: Scam <44c96d97d4bda5bbcbd62565b0b19bb2e895e1fad5d9e9116f267909f1dae78e@meshllm.communities.buzz.xyz>
Co-authored-by: Scam <44c96d97d4bda5bbcbd62565b0b19bb2e895e1fad5d9e9116f267909f1dae78e@meshllm.communities.buzz.xyz>
Co-authored-by: Scam <44c96d97d4bda5bbcbd62565b0b19bb2e895e1fad5d9e9116f267909f1dae78e@meshllm.communities.buzz.xyz>
Co-authored-by: Scam <scam@buzz.local>
Co-authored-by: Scam <44c96d97d4bda5bbcbd62565b0b19bb2e895e1fad5d9e9116f267909f1dae78e@meshllm.communities.buzz.xyz>
Co-authored-by: Scam <44c96d97d4bda5bbcbd62565b0b19bb2e895e1fad5d9e9116f267909f1dae78e@meshllm.communities.buzz.xyz>
Co-authored-by: Scam <44c96d97d4bda5bbcbd62565b0b19bb2e895e1fad5d9e9116f267909f1dae78e@meshllm.communities.buzz.xyz>
Co-authored-by: Scam <scam@buzz.local>
fb105aa to
bb7b232
Compare
Co-authored-by: Scam <scam@buzz.local>
Co-authored-by: Scam <44c96d97d4bda5bbcbd62565b0b19bb2e895e1fad5d9e9116f267909f1dae78e@meshllm.communities.buzz.xyz>
i386
left a comment
There was a problem hiding this comment.
Independent review + benchmark (Jian Yang)
Verdict: approve once the branch head includes the sparse-logits fix (15005c80) — that commit fixes a real bug present in the pinned review head.
Verified independently (source, at pinned head 0b7265e5 + main c383b411)
- Production overrides
max_consecutive_prefill_iterations: 1— Jared'susize::MAXstarvation claim was stale for prod; your correction was right. - Mutex serializes multi-lane batch ABI calls, not individual requests.
plan_mixed_iterationreserves decode rows first, fills remaining budget with prefill chunks,sample_lastonly on final rows. Prod wiring setsmixed_prefill_decode: true(not dead config).- Duration guard: decode-only EWMA calibration per row count; mixed step must fit
clamp(0.2×decode_us, 8ms, 20ms)extra or one-way fallback. Reasonable. - Your "serving-equivalent binary" claim checks out:
0b7265e5vs my benched6ad68faediffer only inevals/skippy-adaptive-prefill-ab.py.
Independent benchmark (M1 Ultra, mirrors your protocol; llama.cpp v0.3.0 c1d0e7a00 fresh build)
Median aggregate tg tok/s, no-cache, pp=512, 3 runs:
| tg | c | main c383b41 | this stack | llama v0.3.0 |
|---|---|---|---|---|
| 64 | 1 | 97.8 | 97.8 | 110.6 |
| 64 | 4 | 119.0 | 124.7 | 410.8 |
| 256 | 4 | 148.7 | 155.6 | 340.9 |
Both skippy arms are flat across concurrency; llama scales ~3.7×. Main vs this PR: within ±5% everywhere — no regression, no headline win, matching your own A/B and the PR's narrow claim.
The decisive read — answered with telemetry
stage.scheduler_iteration decode-token histogram over 4,138 iterations per arm: rows=1 in 33%, rows=2 in 32%, rows=4 in 30% of iterations. Coalescing works. Decode-only step time medians: 1 row = 9.7 ms, 2 = 14.8 ms, 4 = 24.6 ms — native step cost scales ~linearly with rows on Metal. The competitive gap is native batch-GEMM efficiency, not scheduler admission. Mixed iterations engaged in 1/4,136 iterations of my simultaneous-arrival sweep (engagement is workload-shape dependent — worth stating in the PR description).
Findings
- F1 (blocker on pinned head, fixed by
15005c80): ABI passed sparsesampled_output_indextoskippy_sample_token_ith, butllama_get_logits_ithwants the original batch row index — wrong sampled token whenever asample_lastrow isn't the batch tail. Merge only with this included; the51b2b588lane bump (3→6) that lets the parity test exercise it should land too. - F2:
mixed_prefill_viableone-way latch never re-probes after fallback. Safe but sticky; note it. - F3: missing-sample semantics diverge — frontend
unwrap_or(-1)→Finished, enginecomplete_iteration→Failed. Align. - F4: your own representative trace shows p50 TTFT +23.7%. Short agent-turn cells (the Carrack 8-token TTFT-dominated cell that started this thread) won't benefit; the guard bounds per-step extra but every decode token rides prefill work.
- F5 (pre-existing, main):
ignore_eos→ HTTP 400 "parsed but not yet implemented" (request.rsUNSUPPORTED_FIELDS). Needs a follow-up issue.
Detailed write-up with artifacts: RESEARCH/PR1456_MIXED_SCHEDULER_REVIEW_2026_08_27.md in the agent workspace; raw JSON in .scratch/scheduler-review/results/.
Co-authored-by: Scam <44c96d97d4bda5bbcbd62565b0b19bb2e895e1fad5d9e9116f267909f1dae78e@meshllm.communities.buzz.xyz>
Co-authored-by: Scam <44c96d97d4bda5bbcbd62565b0b19bb2e895e1fad5d9e9116f267909f1dae78e@meshllm.communities.buzz.xyz>
Co-authored-by: Scam <44c96d97d4bda5bbcbd62565b0b19bb2e895e1fad5d9e9116f267909f1dae78e@meshllm.communities.buzz.xyz>
Co-authored-by: Scam <44c96d97d4bda5bbcbd62565b0b19bb2e895e1fad5d9e9116f267909f1dae78e@meshllm.communities.buzz.xyz>
Co-authored-by: Scam <44c96d97d4bda5bbcbd62565b0b19bb2e895e1fad5d9e9116f267909f1dae78e@meshllm.communities.buzz.xyz>
|
Superseded by #1478, which contains this branch's full content (verified by file-tree comparison: zero content diff in |
Summary
Correctness and safety
The native batch now exposes request-major activation frames separately from compact sampled outputs. Every sampled token carries its originating request index. Rust validates sample counts, bounds, and duplicate indexes before scheduler state is mutated. Missing or duplicate predictions fail only the affected sampled sequence.
The mixed planner preserves the latency invariant: every live decode row is scheduled before prompt work, subject to the global token budget. Remaining tokens are used for prefill/recompute while respecting the configured chunk and prefill-sequence limits.
flowchart LR D[Live decode rows] --> R[Reserve latency-sensitive rows first] P[Prefill and recompute rows] --> F[Fill remaining token budget] R --> B[One mixed native iteration] F --> B B --> A[Request-major activation frames] B --> S[Compact sampled outputs] S --> M[Explicit request index plus token] M --> C[Complete scheduler work by index]Scheduler lab
The deterministic staggered workload compares bounded phase-homogeneous scheduling with mixed execution:
Mixed execution produced eight mixed iterations on this workload; the simulation contract locks improved occupancy, inter-token latency, and makespan.
Studio54 exact-head A/B
Two order-reversed passes per arm used fixed output lengths on the Qwen3.5 0.8B Metal runtime. The live result validates the mechanism but does not support a blanket latency-win claim:
The candidate reduced native scheduler iterations by 16.1% and 27.5%, respectively, and produced 21/66 mixed iterations per pass versus zero on the base. Longer mixed steps worsened TTFT and tail streaming gaps, however, and outweighed iteration savings under stress. Duration-aware admission and a latency budget for joined prefill work remain necessary before claiming a broad performance improvement.
xychart-beta title "Studio54 mechanism change (base = 100)" x-axis [Rep-iterations, Rep-occupancy, Stress-iterations, Stress-occupancy] y-axis "Percent of base" 0 --> 145 bar [100, 100, 100, 100] bar [83.9, 119.2, 72.5, 138.0]Versioned method, per-pass numbers, and the normalized latency/occupancy charts are in
docs/skippy/MIXED_PREFILL_DECODE_STUDIO54.md. The compact raw metrics are indocs/skippy/mixed-prefill-decode-studio54-summary.json.Validation
cargo fmt --allskippy-ffi,skippy-runtime,skippy-scheduler,skippy-server, andskippy-correctnesswith all targets and dynamic runtime/lab featuresscripts.tests.test_prepare_llamapassedThe local macOS static native build could not be used as evidence because the host Apple C++ toolchain cannot resolve standard
<mutex>/<array>headers and lacks OpenMP. Linux native CI is the authoritative static/native gate.Stack
Stacked on #1453 (
scam/scheduler-adaptive-prefill). Review this PR as the final mixed-execution layer after that base.