Skip to content

[Webgpu] Avoid mappedAtCreation staging upload - #29846

Merged
tianleiwu merged 2 commits into
mainfrom
tlwu/20260723/update_webgpu_to_fix_ci
Jul 26, 2026
Merged

[Webgpu] Avoid mappedAtCreation staging upload#29846
tianleiwu merged 2 commits into
mainfrom
tlwu/20260723/update_webgpu_to_fix_ci

Conversation

@tianleiwu

Copy link
Copy Markdown
Contributor

Fix CI failures like https://github.com/microsoft/onnxruntime/actions/runs/29993467798/job/89171381036

Root cause

  • The failing WebGPU Where tests were tripping GpuDataManager.upload().
  • That path created a fresh mappedAtCreation: true staging buffer for each CPU→GPU upload.
  • On the Windows Chrome/WebGPU runner, even a 16-byte upload hit createBuffer failed ... mappedAtCreation == true, which caused the first test to hang and the rest to cascade with Session already started.

Change

  • Replaced the staging-buffer upload path in /home/runner/work/onnxruntime/onnxruntime/js/web/lib/wasm/jsep/webgpu/gpu-data-manager.ts
  • Now uploads use device.queue.writeBuffer() directly, preserving the existing zero-padded aligned write behavior.

Validation

  • npm ci in js, js/common, and js/web
  • npx eslint web/lib/wasm/jsep/webgpu/gpu-data-manager.ts
  • npm run prebuild in js/web
  • CodeQL: 0 JavaScript alerts

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the WebGPU GpuDataManager.upload() implementation to avoid using per-upload mappedAtCreation: true staging buffers (which were failing/hanging on Windows Chrome/WebGPU CI), and instead performs CPU→GPU uploads via device.queue.writeBuffer() while preserving the existing 16-byte zero-padded alignment behavior.

Changes:

  • Replaced the staging-buffer + copyBufferToBuffer upload path with queue.writeBuffer() into the destination GPU buffer.
  • Preserved the previous “round up to 16 bytes and zero-pad” upload semantics by writing an aligned Uint8Array(size).

Comment thread js/web/lib/wasm/jsep/webgpu/gpu-data-manager.ts Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

@tianleiwu
tianleiwu requested a review from hariharans29 July 24, 2026 17:18
@hariharans29

Copy link
Copy Markdown
Member

Review: PR #29846 — [Webgpu] Avoid mappedAtCreation staging upload (head df70499)

Author: @tianleiwu. 2 commits (e55f7dadf70499, the latter being a Copilot-suggested fix accepted via the web UI). Copilot reviewed twice; 1 comment marked Outdated / resolved after df70499, second review passed with 0 comments. CI on head: 86 / 86 checks OK. @hariharans29 requested as reviewer.

Verdict: approve. Small, tightly-scoped fix for a real CI failure. The change swaps the legacy mappedAtCreation staging-buffer upload path for GPUQueue.writeBuffer() — the modern WebGPU-canonical way to do CPU→GPU uploads — and adds a tidy alignment fast-path on top.


What it does

Single-file change in js/web/lib/wasm/jsep/webgpu/gpu-data-manager.ts:

Before — per-upload staging buffer:

const gpuBufferForUploading = this.backend.device.createBuffer(
  { mappedAtCreation: true, size, usage: GPUBufferUsage.MAP_WRITE | GPUBufferUsage.COPY_SRC },
);
const arrayBuffer = gpuBufferForUploading.getMappedRange();
new Uint8Array(arrayBuffer).set(new Uint8Array(srcArrayBuffer, srcOffset, srcLength));
gpuBufferForUploading.unmap();
const commandEncoder = this.backend.device.createCommandEncoder();
commandEncoder.copyBufferToBuffer(gpuBufferForUploading, 0, gpuDataCache.gpuData.buffer, 0, size);
this.backend.device.queue.submit([commandEncoder.finish()]);
gpuBufferForUploading.destroy();

After — direct writeBuffer with an alignment fast-path:

if (size === srcLength && srcOffset % 4 === 0) {
  // Fast path: already aligned; avoid allocating/copying a padded buffer.
  this.backend.device.queue.writeBuffer(gpuDataCache.gpuData.buffer, 0, srcArrayBuffer, srcOffset, srcLength);
} else {
  const uploadData = new Uint8Array(size);
  uploadData.set(data);
  this.backend.device.queue.writeBuffer(gpuDataCache.gpuData.buffer, 0, uploadData, 0, size);
}

Why this fixes the CI failure

The failing Windows Chrome/WebGPU runner was hitting createBuffer failed ... mappedAtCreation == true even on a 16-byte upload — which then hung the first test and cascaded Session already started errors across the rest. The old code did this on every CPU→GPU upload: create a fresh staging buffer with mappedAtCreation: true → map → memcpy → unmap → encode a copyBufferToBuffer → submit → destroy. That's a lot of driver-visible surface per upload, and the mapped-at-creation path was demonstrably fragile on that runner (likely a Dawn/driver-version quirk).

GPUQueue.writeBuffer is spec-defined to do the same job internally — the browser owns the staging pool, coalesces uploads, and doesn't require the user code to touch the mapping API. It's also the pattern the WebGPU spec explicitly recommends for CPU→GPU uploads of moderate-size data. So this isn't just a workaround; it's a move to the correct primitive.


Correctness

writeBuffer alignment requirements (per the WebGPU spec):

  • Destination bufferOffset % 4 === 0 — always 0 here. ✓
  • Byte size written must be a multiple of 4.

Fast-path branch size === srcLength && srcOffset % 4 === 0:

  • size === srcLength means no padding needed. Since size in this codepath is the pre-computed padded (round-up-to-16) size, size === srcLength implies srcLength was already at the alignment boundary, i.e. a multiple of 16 → also a multiple of 4. ✓
  • srcOffset % 4 === 0 guards the source-side alignment.
  • The writeBuffer(dst, 0, srcArrayBuffer, srcOffset, srcLength) call therefore satisfies both bufferOffset%4 and size%4 constraints.

Padded branch (else):

  • new Uint8Array(size) is zero-initialized by JS engine spec, so the trailing size - srcLength bytes are zeros.
  • uploadData.set(data) copies srcLength bytes into offset 0. Remaining tail stays zero.
  • writeBuffer(dst, 0, uploadData, 0, size) writes size bytes. size is the padded (multiple-of-16) size → multiple of 4. ✓

Small semantic improvement: The old code's tail bytes (past srcLength in the padded staging buffer) were whatever getMappedRange() returned initially — the spec doesn't guarantee zeros there, though Chrome/Dawn happen to zero-init in practice. The new code always explicitly zero-fills the tail. Marginal but strictly better.


Copilot review disposition

  • Copilot Set up CI with Azure Pipelines #1 on the initial commit e55f7da: comment on js/web/lib/wasm/jsep/webgpu/gpu-data-manager.ts marked "Outdated" — addressed by df70499 (the "Potential fix for pull request finding" auto-commit). Given the diff shape (introducing the fast-path guard and the explicit zero-fill split), Copilot's original suggestion was likely about the alignment / offset math on the raw-srcArrayBuffer case that the fast-path now specifically gates for. Correctly resolved.
  • Copilot Remove vsts test runner in cmake file #2 re-review on df70499: 0 new comments.

All Copilot feedback handled.


Minor observations (none blocking)

  1. data in uploadData.set(data) (padded branch) is assumed to refer to a same-scope Uint8Array view of the source (data.length === srcLength). Not visible from the diff alone but consistent with the surrounding code and validated by the green CI.
  2. Destination gpuDataCache.gpuData.buffer needs GPUBufferUsage.COPY_DST for both the old (copyBufferToBuffer) and new (writeBuffer) paths — same requirement, so no wiring change needed elsewhere.
  3. The old staging-buffer allocation used MAP_WRITE | COPY_SRC usage flags. Those are no longer needed since the ephemeral staging buffer is gone; there's no orphan config to clean up because it was a per-call local resource anyway.
  4. Perf: writeBuffer internally may still stage, but the browser manages a pool and can coalesce/reuse. For the small (16-byte) uploads that were failing, this is dramatically fewer driver calls per upload than the old create/map/copy/submit/destroy sequence. No perf regression expected; likely a small improvement, especially for many small uploads.

Bottom line

Correct, focused fix for a real CI-blocking failure. Uses the WebGPU-canonical upload primitive and adds a sensible alignment fast-path. Both Copilot review passes are clean and CI is fully green on the head commit. Ready to merge once @hariharans29 (or another Microsoft-side reviewer) signs off.

@tianleiwu
tianleiwu merged commit f872781 into main Jul 26, 2026
87 checks passed
@tianleiwu
tianleiwu deleted the tlwu/20260723/update_webgpu_to_fix_ci branch July 26, 2026 06:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants