Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add get_mapped_range_as_array_buffer #4042

Merged
merged 1 commit into from
Aug 13, 2023

Conversation

ryankaplan
Copy link
Contributor

@ryankaplan ryankaplan commented Aug 12, 2023

Checklist

  • Run cargo clippy.
  • Run cargo clippy --target wasm32-unknown-unknown if applicable.
  • Add change to CHANGELOG.md. See simple instructions inside file.

Connections

This change came out of this discussion: https://matrix.to/#/!XFRnMvAfptAHthwBCx:matrix.org/$DUm42m4q3w5SsU6oDki7aXSyLnA2RkC-zQh7Be2rhKE?via=matrix.org&via=fachschaften.org&via=jepcraft.ddns.net%3A443

There isn't a GitHub issue for this but I'm happy to create one if that would be useful.

Description

I'm using wgpu in a wasm build and occasionally want to read texture data in Javascript. Right now when I do that, the data goes from WebGPU -> Uint8Array -> Rust heap -> Uint8Array. That's a lot of copies 😅

This PR adds Buffer::buffer_get_mapped_range_as_array_buffer which bypasses the copy into the Rust heap. It passes the Uint8Array that rust gets from JS directly to the caller, who can then pass it back to their own JS code.

Testing

This is my first change to wgpu -- please feel free to tell me to check extra things to be sure I didn't break anything.

I didn't see a way to add automated tests for this, but I've been using this new API with code that is very similar to this example and it works great: https://github.com/gfx-rs/wgpu/blob/trunk/examples/capture/src/main.rs#L158

@ryankaplan ryankaplan force-pushed the get_mapped_range_as_array_buffer branch from 11ad349 to 5e37c8d Compare August 12, 2023 00:45
@ryankaplan
Copy link
Contributor Author

Side note: I'm impressed with the CI setup here. So cool!!

Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

that's super useful, thanks for contributing!!
Looks all good to me 🚢

@Wumpf Wumpf merged commit 014ae3f into gfx-rs:trunk Aug 13, 2023
@ryankaplan ryankaplan deleted the get_mapped_range_as_array_buffer branch August 31, 2023 05:37
ryankaplan added a commit to ryankaplan/wgpu that referenced this pull request Aug 31, 2023
…avoids copying mapped data into wasm heap (gfx-rs#4042)

Co-authored-by: Ryan Kaplan <[email protected]>
ryankaplan added a commit to ryankaplan/wgpu that referenced this pull request Sep 1, 2023
…avoids copying mapped data into wasm heap (gfx-rs#4042)

Co-authored-by: Ryan Kaplan <[email protected]>
ryankaplan added a commit to ryankaplan/wgpu that referenced this pull request Sep 1, 2023
…avoids copying mapped data into wasm heap (gfx-rs#4042)

Co-authored-by: Ryan Kaplan <[email protected]>
cwfitzgerald pushed a commit that referenced this pull request Sep 5, 2023
…avoids copying mapped data into wasm heap (#4042) (#4103)

Co-authored-by: Ryan Kaplan <[email protected]>
@zeroexcuses
Copy link

  1. Apologies if this is the wrong place to ask this.

  2. I have the following problem. I have data in a js_sys::ArrayBuffer. I want to upload this data into a wgpu/vbo. I prefer to avoid js_sys::ArrayBuffer -> wasm_heap/&[u8] -> wgpu call -> [newly copied] js_sys::ArrayBuffer.

  3. I would prefer to just upload my js::ArrayBuffer directly to the vbo, without the two extra js -> wasm_heap, wasm_heap -> js copies.

  4. I was pointed to this patch as something that may be helpful.

  5. Here is what confuses me: the functions in this patch appear to (1) return js_sys::ArrayBuffer as an return type and (2) does NOT take js_sys::ArrayBuffer as an input argument.

  6. Can the functions in this patch help me solve my problem of uploading directly from js_sys::ArrayBuffer -> wgpu/vbo, without the intermediate js -> wasm_heap, wasm_heap -> js two extra copies ?

If so, could I trouble you for some sample code on how to do this ? I suspect it's < 5 lines, but it is not obvious to me at all (given the type signatures of the functions added in this patch.)

Thank you!

@zeroexcuses
Copy link

EDIT: above is directed at @ryankaplan

@ryankaplan
Copy link
Contributor Author

Hi @zeroexcuses! This patch is a similar flavor of problem to yours, but it's different. I don't think it'll help you directly 😢

What this PR does: previously when reading data out of a wgpu::Buffer on web, wgpu would ask WebGPU for the data and WebGPU would hand it an ArrayBuffer. wgpu would then copy the data into the wasm heap. If you wanted the data in JS, you'd have to then copy that into an ArrayBuffer! This PR makes it so that you can skip the step in the middle when it ends up in the wasm heap.

My fix is very specific to the API I was using, so I don't think it'll be helpful to you.

I do think there's a general way to solve this problem that would work for both of us, but it's a lot of effort... We could make a struct called something like CpuBuffer in Rust that is backed by heap memory in native builds and by an (non-wasm) ArrayBuffer in web builds. Anywhere wgpu moves CPU data around - either by giving it to underlying graphics API's, or getting it from underlying graphic's API's - we'd use CpuBuffer instead of Vec<u8>.

In addition to fixing our two cases, this would have the pretty significant upside that CPU-side geometry data and texture data wouldn't count against the 4GB wasm heap size limit that exists in many browsers. But it'd be a huge effort and I'm not sure how the community decides whether to do stuff like this. The approach is inspired by IndirectBuffer which I've used before.

Hope that's helpful! Let me know if you have any other questions.

@zeroexcuses
Copy link

@ryankaplan : Thank you for your detailed response. To make sure I understand what you have stated, is the following restatement correct ?

Your problem (that you solved):
want: wgpu::buffer -> js/ArrayBuffer
old method: [ wgpu:buffer -> js/ArrayBuffer -> wasm_heap] -> js/ArrayBuffer
where [ ... ] is via wgpu api, and the final -> is via web_sys for: &[u8] -> js/ArrayBuffer
your patch: buffer -> js/ArrayBuffer

My problem:
want: js/ArrayBuffer -> wgpu::buffer
old method: js/ArayBuffer -> [ wasm_heap -> jsArrayBuffer -> wgpu::buffer ]
where the [ ... ] is via wgpu api, and the first -> is via web_sys

Put another way, your problem was "copy data FROM wgpu buffer TO js/ArrayBuffer"; my problem is "copy data FROM js/ArrayBuffer TO wgpu buffer"

Does this sound correct ?

@ryankaplan
Copy link
Contributor Author

ryankaplan commented Sep 13, 2023

Yes, that sounds right to me!

@Wumpf
Copy link
Member

Wumpf commented Sep 13, 2023

I do think this new api can still be an improvement for the upload case: The arraybuffer returned is owned by the WebGPU implementation and copies to it will have visible side effects after unmapping.
Meaning that if one has an ArrayBuffer for uploading ready this reduces one of the copies: Right now it goes ArrayBuffer -> wasm_heap -> mapped ArrayBuffer -> webgpu impl internals. using buffer_get_mapped_range_as_array_buffer this is reduced to ArrayBuffer -> mapped ArrayBuffer -> webgpu impl internals

@zeroexcuses
Copy link

Sorry, I have a dumb question. Can you post me a link to the documentation for "mapped ArrayBuffer" ? On google, the first result links to SharedArrayBuffer, which I think is something different.

What is "mapped ArrayBuffer" ? Is it wasm_heap or js side ?

@kpreid
Copy link
Contributor

kpreid commented Sep 18, 2023

What is "mapped ArrayBuffer" ? Is it wasm_heap or js side ?

It's a JavaScript object of type ArrayBuffer, which points to the mapped memory (not any additional copy of it).

So, if you want to have minimal-copies data transfer, the answer is to write your original data into the mapped ArrayBuffer. If you can't do that directly, you'll have to perform at least one copy (from the ArrayBuffer you created previously to the mapped ArrayBuffer)

@zeroexcuses
Copy link

Sorry, what is even "mapped memory" here? Is it mmap, or some type of hybrid-cpu-gpu-shared-memory? And if the latter, when does writing to it get sync-ed to the gpu? On every byte write? On every flush? What is the flush call? Every time the JS thread finishes execution / blocks / does an .await ?

[Not trolling, genuinely ignorant.]

@kpreid
Copy link
Contributor

kpreid commented Sep 19, 2023

Sorry, what is even "mapped memory" here? Is it mmap, or some type of hybrid-cpu-gpu-shared-memory?

You can treat it like a mmap() of GPU memory instead of a file. Exactly how it works depends on the GPU and computer architecture.

when does writing to it get sync-ed to the gpu? On every byte write? On every flush? What is the flush call?

WebGPU/wgpu guarantee to provide enough synchronization that you don't need to care about that; in particular, if I understand correctly, a buffer can never be simultaneously mapped for CPU access and available for GPU operations. So there's always an explicit map or unmap, and whether that's merely a synchronization barrier or actually a copy from CPU memory to/from GPU memory, you can't tell the difference.

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