perf(qwen38): various DFlash2 optimizations concurrency C1-C5 - #659
Open
Graffioh wants to merge 28 commits into
Open
perf(qwen38): various DFlash2 optimizations concurrency C1-C5#659Graffioh wants to merge 28 commits into
Graffioh wants to merge 28 commits into
Conversation
Restore the paged-attention C ABI through a separate tree entry point. Reject unsupported GDN variants, initialize root-only chain trees, and size draft metadata from graph capacity.
Keep GDN journals inside the result buffer, restore the 10-source tensor layout, validate every tree destination before mutation, and allow selector widths that use the CPU top-k fallback.
Validate singleton selector vocabularies and token IDs before codebook gathers. Reject unsupported GDN output layouts on Vulkan, and use overflow-safe ceil division in paged attention.
Preserve paged-KV allocation order across rollback and destroy draft graphs before park frees their weights. Reject invalid GDN replay layouts and malformed SWA metadata. Exercise both grouped and scalar GDN dispatch paths.
This was referenced Aug 26, 2026
Contributor
Author
Graffioh
marked this pull request as ready for review
August 26, 2026 12:57
Contributor
There was a problem hiding this comment.
17 issues found across 66 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="server/deps/llama.cpp/ggml/src/ggml-cuda/gdn-replay-log.cu">
<violation number="1" location="server/deps/llama.cpp/ggml/src/ggml-cuda/gdn-replay-log.cu:341">
P1: When this transaction follows a graph compute, these kernels can read replay logs, accepted metadata, and scratch rows before ggml's nonblocking graph stream finishes writing them. Launch on the graph/backend stream or explicitly synchronize that stream before the commit.</violation>
</file>
<file name="server/src/qwen35/concurrency/qwen35_slot_manager.cpp">
<violation number="1" location="server/src/qwen35/concurrency/qwen35_slot_manager.cpp:291">
P2: Every ordinary decode lane now routes through the batched path, allocating and filling a row vector for one token on every step. Preserve a lightweight single-token path, or suppress batched row materialization for `n_tokens == 1`, to avoid this host-side allocation and copy in the hot decode loop.</violation>
</file>
<file name="server/src/common/dflash2_batch.cpp">
<violation number="1" location="server/src/common/dflash2_batch.cpp:18">
P2: When a transient drafting thread exits after using this file, its thread-local graphs retain their gallocr device buffers and ggml contexts because the structs have no destructors. Add thread-exit cleanup, matching the existing `SelectorGraph` destructor, so each such thread does not leak graph memory until process exit.</violation>
<violation number="2" location="server/src/common/dflash2_batch.cpp:175">
P1: When the selector codebooks are F16, `ggml_get_rows` produces non-F32 rows, but this path reads them into float buffers. Cast both gathered tensors to `GGML_TYPE_F32` before readback, as the existing selector path does; otherwise batched selection can fail or use corrupted scores.</violation>
</file>
<file name=".github/workflows/ci.yml">
<violation number="1" location=".github/workflows/ci.yml:237">
P2: The `-R` regex references `gdn_transition_journal_preflight$`, but no such ctest name is registered in server/CMakeLists.txt (only `gdn_replay_log`, `gdn_replay_log_gallocr`, `gdn_replay_log_preflight`). With `--no-tests=error`, ctest silently matches nothing for this clause and exits 0, so the `test_gdn_replay_log` target added in this same diff is built but never runs — the new GDN replay verification ships without executing. Match the registered test name, e.g. `gdn_replay_log_preflight$` if the intent is the preflight run, or `gdn_replay_log$` for the default replay-log run.</violation>
</file>
<file name="server/src/common/concurrency/chain_spec_shapes.h">
<violation number="1" location="server/src/common/concurrency/chain_spec_shapes.h:18">
P1: When concurrency exceeds 64 lanes, this fallback still returns 64. The callers allocate 64-wide buffers but index every lane, causing out-of-bounds writes; reject unsupported widths or size the buffers for the actual lane count.</violation>
</file>
<file name="server/src/common/step_graph.h">
<violation number="1" location="server/src/common/step_graph.h:23">
P1: When a retained graph is rebuilt for a different backend, `step_graph_free` leaves `sg.alloc` bound to the old backend's buffer type, so the new graph can be allocated in the wrong backend. Recreate the gallocr whenever the backend changes, alongside the graph invalidation.</violation>
</file>
<file name="server/src/draft/draft_graph.cpp">
<violation number="1" location="server/src/draft/draft_graph.cpp:419">
P2: When more than one lane is batched, this view disables the fused dynamic-convolution kernel for every attention and MLP prepare/finish call, replacing each with the much larger unfused graph. Materialize each lane's coefficient slice contiguously, or update the fused kernel to accept the packed stride, so batching does not regress dynamic-convolution performance.</violation>
</file>
<file name="server/src/common/feature_gate.cpp">
<violation number="1" location="server/src/common/feature_gate.cpp:179">
P2: When `--draft-block-size` is set above 16, this predicate admits a concurrent chain that `Qwen35Backend` cannot initialize. Reject explicit widths above 16 in the concurrent compatibility check, or return a dedicated incompatibility error before backend construction.</violation>
</file>
<file name="server/src/qwen35/concurrency/qwen35_seq_engine.cpp">
<violation number="1" location="server/src/qwen35/concurrency/qwen35_seq_engine.cpp:191">
P2: When six decode slots are eligible, this selects all six, bypassing the exact five-lane C5 draft specialization and adding an unnecessary draft lane. Cap selection at five lanes and leave any remaining eligible row on the ordinary AR path.</violation>
</file>
<file name="server/src/qwen35/qwen35_backend.cpp">
<violation number="1" location="server/src/qwen35/qwen35_backend.cpp:485">
P1: When a concurrent Qwen35 request uses a local draft without the selector metadata, with a block size above 16, or without a target `lm_head`, `fixed_chain.enabled` is false and initialization now fails outright. Preserve the existing generic concurrent draft path and use the fixed chain only when its extra prerequisites are met.</violation>
</file>
<file name="server/deps/llama.cpp/ggml/src/ggml-sycl/ggml-sycl.cpp">
<violation number="1" location="server/deps/llama.cpp/ggml/src/ggml-sycl/ggml-sycl.cpp:4962">
P1: When a GDN node requests a replay log, `op_params[3]` is nonzero but this predicate still selects SYCL; the kernel never writes that log, so rollback consumes invalid state data. Reject replay-log nodes here and fall back to a backend that implements capture.</violation>
<violation number="2" location="server/deps/llama.cpp/ggml/src/ggml-sycl/ggml-sycl.cpp:4962">
P1: For a valid GDN with a state width other than 16/32/64/128, this predicate selects SYCL and `launch_gated_delta_net` hits its `GGML_ABORT` default case. Limit support to the widths implemented by the launcher.</violation>
</file>
<file name="server/test/test_seq_engine_contract.cpp">
<violation number="1" location="server/test/test_seq_engine_contract.cpp:330">
P3: This test asserts `!validate_step_result(...).empty()` but never pins which violation it expects. It is meant to exercise the disabled-speculation burst guard, yet it would still pass if that guard regressed and a different (or wrong) error fired. Check the returned string mentions "disabled speculation" (or otherwise contains the specific expected message) so the test fails precisely when the guard it protects changes.</violation>
</file>
<file name="docs/handoffs/pr651-draft-batching-performance.md">
<violation number="1" location="docs/handoffs/pr651-draft-batching-performance.md:25">
P3: This handoff note is stale relative to the code it is committed alongside. It describes item 1 as pending work using a singular per-lane builder `build_draft_kv_append()`, but HEAD already uses the packed `build_draft_kv_appends()` (the very signature the note proposes adding) from `build_draft_kv_batch()`; no singular function exists. Items 1-2 are already implemented by this PR (mechanisms 1-2) and item 3 is rejected in this PR, yet it also instructs reviewers to "not combine all three into one patch" and ship item 3 separately, contradicting the bundled six-mechanism PR that adds the file. Update the note to reflect the shipped state (or mark it as a historical record) so a reader does not treat the work as pending.</violation>
</file>
<file name="server/test/test_draft_topk_cuda.cpp">
<violation number="1" location="server/test/test_draft_topk_cuda.cpp:130">
P3: The `K` loop here asserts `geometric_draft_topk_cuda_supports_k` against `expected`, which is the same constant expression the header's `constexpr` body is written with. So the loop is a tautology: it only fails if this test's literal and the header literal diverge, and it can never catch the failure mode that matters, namely `supports_k` drifting out of sync with the kernel's actually instantiated K cases (1-8/12/16 in geometric_draft_topk_cuda.cu). The behavioral coverage this test cares about is already provided by the invalid-pointer checks and the real-device-memory unsupported-K loop in `draft_topk_cuda_suite`. Pin the documented contract to a fixed set rather than re-deriving the header expression, so a change to the header constant is what breaks the test.</violation>
</file>
<file name="server/src/common/ddtree.cpp">
<violation number="1" location="server/src/common/ddtree.cpp:379">
P3: The new truncate_verified_path is never called from the serving path; the only reference besides its declaration is the unit test test_ddtree_path.cpp. As written, the commit-bounding behavior it provides is not actually applied by qwen35/laguna backends, which still cap accepted with their own local `if (accepted_n > need_commit_budget)` logic. If a #651 caller is expected to consume it, note that it isn't reachable in the current combined head; otherwise this is dead code plus the duplicated bounding logic already in the backends.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Add fixed-width DFlash2 concurrent decoding for Qwen3.8 and batch the shared dense draft projections across active lanes. Keep convolution, cache updates, RoPE, masks, and attention lane-local. Preserve transactional state promotion and fixed-chain validation through C6.
Use width 5 for the draft graph when five lanes are active. Keep target verification on decode bucket 6 to preserve MMQ numerics.
Cache completed paged-tree graphs by owner, backend, and topology. Rebuild on shape changes and model lifecycle events while preserving the uninitialized metadata arena.
Graffioh
force-pushed
the
codex/qwen38-gdn-replay-tile
branch
from
August 26, 2026 15:22
470b477 to
62d051e
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Depends on #651 (
codex/draft-projection-batching). GitHub keeps this PR based onmainbecause #651 comes from a fork; after #651 lands, the visible diff reduces to this follow-up stack.Summary
This draft contains six independently reviewable mechanisms:
fc,wk, andwvmatrix multiplicationsThe first four mechanisms are the measured C1-C4 stack previously described here. The last two are profiling-led follow-ups: one targets the important C5 case, and one removes repeated host graph construction across C1-C5.
Lane-local positions, masks, cache writes, RoPE, attention, dynamic-convolution history, accepted prefixes, active slots, recurrence order, and transaction synchronization remain unchanged. Dummy lanes stay after real lanes and their proposals are discarded.
C1-C4 mechanisms
Qwen3.8 27B IQ4_XS target, DFlash2 Q8 width 8, Q8 KV, HumanEval raw prompts, ten fixed-C waves, and 256 forced output tokens on one R9700:
Each column is the causal comparison for that mechanism against its immediate baseline; the percentages are not additive.
Launch accounting remains mechanism-specific:
On Strix Halo, append packing preserved complete outputs and changed goodput by -0.45%, +0.18%, +0.95%, and +1.08% at C1-C4. Dynamic-convolution packing passed correctness and capture integrity there, but its timing was inconclusive because the unchanged C1 control moved by 8.65%. No Strix performance claim is made for the GDN or new follow-up mechanisms.
Exact C5 draft shape
The existing graph bucketing padded C5 draft work to six lanes. Specializing only the draft graph produced this A/B/A result:
Baseline goodput drift was -0.206%, so the bracket-mean gain clears 2x measured drift. Complete output digests match. The matched kernel capture measured 125.002 fewer speculative dispatches per round. The target graph intentionally remains at width six; specializing target and draft together was not promoted.
Stable target-graph retention
After the C5 launch reduction, profiling showed repeated host-side
target_graph_buildwork rather than a remaining launch-count bottleneck. Retaining a graph while its owner/backend/topology identity remains stable produced:target_graph_buildp50Complete outputs match at every C. Total
target_graph_buildtime fell 89.70-92.12%. A duration-based analyzer inferred reuse on 95.13-95.25% of rounds by classifying phases below 50 microseconds; the capture format has no direct cache-hit marker.This is a small host-side gain, not a kernel or launch-count claim: the C5 speculative dispatch count was identical before and after, and kernel-family device-time changes stayed within 0.59%. The minimum all-C gain clears the maximum measured A/A2 drift at 1x but not 2x. C6+ and memory scaling across multiple backend/owner identities were not measured.
Rejected follow-up
Asynchronous metadata upload reduced
metadata_uploadp50 by 70.23-85.35%, but the wait moved into target compute.target_computep50 regressed by 0.143-0.588%, and C1-C5 goodput remained mixed from -0.069% to +0.460%. No code from that candidate is retained.Correctness and verification
dflash_serverbuild passes on the combined headtest_gdn_replay_logcovers the owned tile, scalar/grouped variants, generic fallback, padded and permuted slots, and transaction preflight#651..HEADancestry andgit diff --checkpassThe retrospective C1-C5 comparisons, metrics, analyzer, decision log, and hashes are in LuceForge PR #73, under
qwen38-dflash2-pr659-c1-c5-observations. They are recorded as observations rather than backfilled accepted trials because no Luceloop draft existed before these edits.