Add fixed-count indirect multi-draw#253
Conversation
644b785 to
3bed88f
Compare
|
Closeout corrections are pushed at 46096ae. The exact head passes the complete native suite, prepared-indexed race checks, Linux/Windows/Rust/Wasm compile coverage, Linux-target vet, and the Apple M1 public parity plus lifecycle probes. The fork CI run is currently action_required with no jobs started; please approve the workflow when convenient. CODEOWNER review remains requested from @kolkov. |
kolkov
left a comment
There was a problem hiding this comment.
Thank you for this substantial contribution — the engineering quality is evident: thorough testing (parity oracle with pixel-for-pixel readback comparison), Metal ICB implementation, DX12 ABI fix discovery (D3D12_INDIRECT_ARGUMENT_DESC 12→16 bytes), and fail-closed token design. This is clearly deep GPU engineering work.
Before we proceed, we'd like to discuss the architectural approach. We have questions, not conclusions — you may have considered tradeoffs we haven't.
Architectural question: Prepare/Token/Execute vs draw_count > 1
Rust wgpu implements multi-draw indirect as a simple draw_count parameter on the existing HAL method (wgpu-hal/src/lib.rs:1489-1494):
unsafe fn draw_indexed_indirect(
&mut self,
buffer: &Buffer,
offset: BufferAddress,
draw_count: u32, // count > 1 = multi-draw
);Vulkan backend (command.rs:1147-1156): if multi_draw_indirect capability → single vkCmdDrawIndexedIndirect with count. Otherwise → loop of N individual calls.
Metal backend (command.rs:1290-1309): loop of N draw_indexed_primitives_indirect calls. No ICB, no prepare/execute split. MoltenVK also uses a CPU loop for vkCmdDrawIndexedIndirect count > 1 (MoltenVK #1796).
Our current DrawIndexedIndirect(buffer, offset) doesn't have a drawCount parameter — that's the gap. The Rust approach would be to add drawCount to the existing method.
Your approach uses ICB on Metal, which is genuinely faster — Tellusim benchmarked 39% improvement on M1 with ICB vs loop. But it surfaces Metal's multi-phase ICB lifecycle into the public API. We see the tension: the simpler draw_count API can't unlock ICB performance because Metal requires upfront pipeline and buffer configuration that isn't available at draw time.
The WebGPU spec itself doesn't define multi-draw yet (gpuweb #4349 closed, #5175 open with drawCount proposal). So we're in extension territory regardless of approach.
Our question: Given that we're a WebGPU-compatible library (not a Metal-first engine), would a phased approach work?
- Phase 1: Add
drawCountto existingDrawIndexedIndirect(Rust wgpu parity, loop on Metal) - Phase 2: Metal ICB optimization as internal backend detail when performance data demands it
Or do you see the ICB path as essential from day one? We'd like to understand the use case driving this — is Hearth hitting the loop bottleneck in practice?
Pipeline opt-in (SupportIndirectCommandBuffers)
We researched this and understand that MTLRenderPipelineDescriptor.supportIndirectCommandBuffers must be set at pipeline creation time — it cannot be enabled retroactively on a compiled pipeline state. So your pipeline opt-in field is technically necessary for ICB, not arbitrary complexity. We acknowledge that.
The remaining question: could the Metal backend set this flag automatically for all pipelines when the device supports Apple7+ (rather than requiring explicit user opt-in)? Or does enabling it impose constraints on shaders that could break non-ICB use cases?
Apple family restriction
apple7 && !apple8 && !apple9 && !apple10 means only M1 is supported. Apple GPU families are cumulative — Apple8 supports everything Apple7 does. What is the concern with newer families? We understand "needs visible proof" but this limits the feature to a single generation.
DX12 adjacent fixes
Commits 30ccaa9 (render-target readback transition), 3bed88f (D3D12_INDIRECT_ARGUMENT_DESC ABI size fix), and 5bd5ad8 (Vulkan pipeline nil-layout guard) are valuable independent correctness fixes. Would you be open to submitting those as a separate PR? They could be merged immediately regardless of the MDI discussion.
What we appreciate
- The parity oracle test methodology — rendering with both paths and comparing pixel-for-pixel is excellent
- Fail-closed design with typed errors
- Coverage across all backends (including explicit "unsupported" for Browser/GLES/Software/Noop)
- The DX12 ABI bug discovery — real find that benefits everyone
- Deep understanding of Metal ICB constraints that even Rust wgpu hasn't tackled
We're not rejecting this — we want to find the right architecture together. Looking forward to your thoughts.
|
Thank you for the detailed review. The phased direction works for Hearth. Separating the portable count API from Metal execution strategy makes the public surface much easier to review and maintain. I will split the three correctness fixes and reduce Phase 1 to the proposed count-bearing DrawIndexedIndirect path through public, core, and the required HAL interface, with native Vulkan/DX12 lowering and honest backend loops. ICB is not required in the Phase 1 public interface. Hearth long-term goal is to return to stock github.com/gogpu/wgpu, so I would like Phase 2 to make Metal acceleration an internal backend implementation behind that same count method. I will keep the existing prepare/token implementation only as a performance and parity oracle while prototyping internal pass scheduling; it will not be proposed as permanent public surface. Before rebuilding the branch, may I confirm the Phase 1 details: count zero is a uniform early no-op, fixed-count semantics may use an honest backend loop, the required HAL method gains drawCount, and Vulkan initially uses one native call within its private limit and an exact loop otherwise? If you prefer an additive MultiDrawIndexedIndirect sibling to avoid the source break, I can use that shape with the same internal path. Once Phase 1 is settled, would a short backend-internal Metal design note comparing deferred recording with a targeted pass split be useful before implementation? |
|
The independent correctness/runtime extractions are now split for review: #257 (Vulkan nil layouts), #258 (DX12 indirect descriptor ABI), and #260 (Metal autorelease thread affinity). I have kept #259 (DX12 texture transitions) in draft after final review exposed broader command-order/subresource state requirements; it should not be merged in its current form. The count-only replacement remains local until the zero-count/source-shape questions above are confirmed, and it contains none of the prepared/ICB/material-page surface. |
|
One additional fork-exit dependency is now isolated as #261: the Metal texture descriptor/copy fix that separates array layers from true 3D depth. It is independent of MDI and includes physical 1D-array, 2D-array, 3D, mixed-copy, and Queue.WriteTexture range coverage. |
|
Thank you for the thoughtful response and for splitting the fixes — #257, #258, #260, #261 are under review. Here are our answers to your Phase 1 questions, based on deep research of the Rust wgpu reference, Vulkan spec, DX12 docs, and WebGPU proposals. Confirmed Phase 1 details1. count=0 is a valid no-op. Vulkan spec explicitly allows 2. Honest backend loops are correct. Metal and GLES loop, DX12 passes count natively to 3. HAL method gains DrawIndexedIndirect(buffer Buffer, offset uint64, drawCount uint32)
DrawIndirect(buffer Buffer, offset uint64, drawCount uint32)This matches the Rust wgpu HAL trait exactly ( 4. Vulkan: single native call when 5. Modify existing, not additive sibling. Breaking the internal HAL interface is acceptable — external consumers use the public wgpu API, which can expose separate 6. Metal design note for Phase 2+ is welcome but not blocking for Phase 1. Phase 1 uses the simple loop on Metal (matching current Rust wgpu). ICB optimization is a future internal backend concern. Additional design pointsStride: Not a user parameter. Fixed Feature gating: Phase 1 MDI does not require a feature flag at the public API level. Loop emulation is transparent. Your extracted fixes (#257, #258, #260, #261) are under review — we'll post feedback shortly. Looking forward to the MDI Phase 1 redesign when you're ready. |
|
The earlier DX12 transition draft #259 has now been closed and superseded by #262. The replacement reconciles resource state in submission order (instead of trusting recording-time state), includes array/3D copy and packed depth/stencil correctness coverage, and is intentionally draft pending native Windows debug-layer validation. |
46096ae to
1953452
Compare
|
Update after the Phase 1 rewrite: head The Metal ICB follow-up is now implemented and published as draft #263. It is stacked on the current #253 head and should be reviewed after #258 and #253. The optimization remains entirely private to #263 uses a deliberately narrow, fail-closed path: on supported Apple GPUs, an eligible first large triangle-list indexed draw on a buffer-only pipeline is translated with one compute dispatch and executed as one bounded ICB range. Small counts, later draws, incompatible pipelines, unsupported devices, writable storage bindings, and setup failures retain #253's exact loop. Pipeline creation automatically attempts the private ICB flag for eligible pipelines and retries ordinary creation if Metal rejects it; no public opt-in or capability surface is added. Native parity covers both index widths, nonzero argument/index offsets, This keeps Phase 1 independently portable and reviewable while making the measured Metal acceleration available as the separate backend-only follow-up requested in the review. |
kolkov
left a comment
There was a problem hiding this comment.
The redesign matches what we agreed on — Prepare/Token/ICB completely removed, replaced with drawCount on existing HAL methods. Exact Rust wgpu parity.
Backend lowering looks correct:
- Vulkan: native call when
multiDrawIndirectsupported, loop fallback with correct offset advance (fixes the bug we reported in gfx-rs/wgpu#9870) - DX12: single
ExecuteIndirectwith count - Metal: loop with offset advance
- GLES/Software/Noop: honest unsupported / no-op
This depends on #258 (DX12 ABI fix). No one in our ecosystem currently uses multi-draw indirect, so this isn't urgent — but the implementation is clean and we'd accept it once #258 lands and the branch is rebased.
One note: indirectRangeFits() is duplicated across 4 packages — consider extracting to a shared internal helper.
@lkmavi — this changes the HAL interface (DrawIndirect/DrawIndexedIndirect gain drawCount parameter). Would appreciate your review from the Metal backend perspective.
Summary
Add fixed-count indirect multi-draw while keeping the existing single-draw
public API:
MultiDrawIndirectandMultiDrawIndexedIndirect;drawCount uint32to the existing HAL indirect methods;count=1for existing single-draw calls.No prepare/token lifecycle, Metal ICB surface, material-page API, or count-buffer
operation is added. This replaces the earlier prepared/ICB design with the
focused count contract requested in maintainer review.
Semantics
count=0is a valid no-op before state or buffer validation;FeatureMultiDrawIndirectreports a native performance path;FeatureMultiDrawIndirectCountremains reserved for GPU count buffers.Backend lowering
multiDrawIndirectis enabled and the count iswithin
maxDrawIndirectCount; otherwise exact offset-advancing calls.ExecuteIndirectwithMaxCommandCount=drawCount.correct indexed support needs a separate translation design because GL cannot
apply a WebGPU index-buffer base offset to an indirect record. Rust wgpu's
current GLES loop advances the 20-byte record offset, but appears to drop a
nonzero index-buffer slice offset; copying that loop would preserve the same
correctness gap rather than provide honest support.
Depends on #258 for the corrected DX12 indirect descriptor ABI. The branch is
stacked on that commit and should be rebased onto
mainafter #258 lands.Verification
CGO_ENABLED=0 go test ./...GOOS=linux GOARCH=amd64 CGO_ENABLED=0 go test -exec=true ./...GOOS=windows GOARCH=amd64 CGO_ENABLED=0 go test -exec=true ./...GOOS=linux GOARCH=amd64 CGO_ENABLED=0 go test -exec=true -tags rust ./...GOOS=js GOARCH=wasm CGO_ENABLED=0 go test -exec=true . ./internal/browsergo test -tags=integration . -run '^TestMultiDrawIndexedIndirectRendersDistinctRecords$' -count=5on Apple M1 Metal, rendering and reading back two distinct indexed records
from a nonzero indirect-buffer offset
0 allocs/op across seven runs
fallback planning, and wrapper loop tests