Skip to content

fix(webgpu): own mapped range backing stores#36257

Open
nathanwhit wants to merge 1 commit into
denoland:mainfrom
nathanwhit:fix/webgpu-mapped-range-storage
Open

fix(webgpu): own mapped range backing stores#36257
nathanwhit wants to merge 1 commit into
denoland:mainfrom
nathanwhit:fix/webgpu-mapped-range-storage

Conversation

@nathanwhit

Copy link
Copy Markdown
Member

Summary

  • back mapped WebGPU ranges with V8-owned ArrayBuffer storage
  • copy writable ranges back before unmapping and detach every returned range when unmapping or destroying the buffer
  • cover multi-range writeback and mapped-range detachment

Background

Writable mapped ranges previously used backend memory directly, while readable ranges were copied without being retained for detachment. This made returned ArrayBuffer lifetimes depend on the native mapping and left readable ranges attached after unmap().

Keeping JavaScript ranges in owned storage makes their lifetime independent of the backend mapping. Writable data is copied back before the mapping closes, including when callers request multiple non-overlapping ranges.

Tests

  • cargo test -p deno_webgpu
  • cargo build -p deno -p test_server
  • cargo test -p unit_tests --test unit webgpu_test
  • cargo clippy -p deno_webgpu --all-targets -- -D warnings
  • ./tools/format.js ext/webgpu/buffer.rs tests/unit/webgpu_test.ts
  • ./tools/lint.js tests/unit/webgpu_test.ts

@bartlomieju bartlomieju left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. This correctly closes the dangling-pointer hazard by moving every returned range into V8-owned storage and writing writable ranges back before the mapping closes. I verified the writeback against wgpu-core get_mapped_range (it recomputes the pointer from the live mapping each call and keeps no per-range bookkeeping, so re-fetching during writeback is idempotent), the copy_nonoverlapping bounds, and mappedAtCreation (map_mode is Write, so it flushes via the Init staging arm). Nice multi-range and detach test coverage. Two minor, non-blocking notes inline.

Comment thread ext/webgpu/buffer.rs
let ab = ab.open(scope);
ab.detach(None);
}
self.writeback_and_detach_mapped_js_buffers(scope)?;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Minor: this early-returns before buffer_unmap, so if writeback ever errors the buffer stays mapped with map_state == "mapped". In practice this is unreachable (the range was already validated at getMappedRange time and the mapping is still active, and the old buffer_unmap()? would have errored in the same invalidated cases), so there's no observable regression. A one-line comment noting the intentional ordering would help future readers.

Comment thread ext/webgpu/buffer.rs
let mut first_error = None;
for mapped in self.mapped_js_buffers.replace(vec![]) {
let ab = mapped.buffer.open(scope);
if mapped.copy_on_unmap && mapped.size != 0 && ab.byte_length() != 0 {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Edge case worth a comment: if a caller does ab.transfer() on a writable range and writes into the transferred copy before unmap(), the ab.byte_length() != 0 guard sees the original as detached and silently skips writeback, dropping that write. It's safe (no UB) and arguably outside what the WebGPU mapping model contemplates, but a brief note here would document the intent.

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.

2 participants