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

Implement GPUBuffer.mapAsync and update wgpu-core #27084

Merged
merged 5 commits into from Jun 27, 2020

Conversation

@kunalmohan
Copy link
Collaborator

kunalmohan commented Jun 25, 2020

This PR covers the following-

  1. update wgpu-core to use serializable RenderPass and ComputePass. Implement DispatchIndirect.
  2. Implement GPUBuffer.mapAsync().

The mapAsync() methods has the following flow-

  1. A new promise is created in mapAsync() and a message sent to the server-side. This returns a promise.
  2. On the server-side, a struct BufferMapInfo object stores the relevant info required to send data to the client-side on completion of mapping. This object is stored in a buffer_maps: HashMap<id::BufferId, BufferMapInfo<'a>>, in WGPU. buffers_maps basically stores the data for buffers with pending mapping.
  3. buffer_map_async() command is issued with a callback responsible to send the data back to the client.
  4. All the devices are polled every time the server receives a new message.
  5. The data is sent back on completion and promise resolved through AsyncWGPUListener (similar to how the requestDevice() and RequestAdapter are processed).
  6. On receiving the data, the client sends back a BufferMapComplete message to the server to remove the entry from buffer_maps hashmap.

I intended to implement getMappedRange(), too, in this PR but it's quite a task in itself (implementing intersecting ArrayBuffers). This already being a 500+ line PR, I thought it best to get this model approved first (also it will probably be easier to review this and getMappedRange separately). getMappedRange would come up in the next one.


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #___ (GitHub issue number if applicable)
  • There are tests for these changes OR
  • These changes do not require tests because ___
@highfive
Copy link

highfive commented Jun 25, 2020

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/webidls/GPUComputePassEncoder.webidl, components/script/dom/gpucomputepassencoder.rs, components/script/dom/gpumapmode.rs, components/script/dom/gpubuffer.rs, components/script/dom/webidls/GPUBuffer.webidl and 10 more
  • @KiChjang: components/script/dom/webidls/GPUComputePassEncoder.webidl, components/script/dom/gpucomputepassencoder.rs, components/script/dom/gpumapmode.rs, components/script/dom/gpubuffer.rs, components/script/dom/webidls/GPUBuffer.webidl and 10 more
@highfive
Copy link

highfive commented Jun 25, 2020

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
  • These commits modify script code, but no tests are modified. Please consider adding a test!
@kunalmohan
Copy link
Collaborator Author

kunalmohan commented Jun 25, 2020

r?@kvark

@highfive highfive assigned kvark and unassigned asajeffrey Jun 25, 2020
@kunalmohan kunalmohan added this to In progress in WebGPU MVP via automation Jun 25, 2020
@kvark
Copy link
Member

kvark commented Jun 25, 2020

All the devices are polled every time the server receives a new message.

That's technically fine, but a bit scary nonetheless. In Gecko, we are polling the devices every 100ms, which is supposed to be configurable in the future, see code. Might be worth doing something like this in Servo.

Copy link
Member

kvark left a comment

Very nice! Just a few notes here

components/script/dom/gpubuffer.rs Outdated Show resolved Hide resolved
components/script/dom/gpubuffer.rs Outdated Show resolved Hide resolved
components/script/dom/gpubuffer.rs Outdated Show resolved Hide resolved
components/script/dom/gpubuffer.rs Outdated Show resolved Hide resolved
components/script/dom/gpubuffer.rs Outdated Show resolved Hide resolved
components/webgpu/lib.rs Outdated Show resolved Hide resolved
components/webgpu/lib.rs Outdated Show resolved Hide resolved
components/webgpu/lib.rs Outdated Show resolved Hide resolved
components/webgpu/lib.rs Outdated Show resolved Hide resolved
components/webgpu/lib.rs Outdated Show resolved Hide resolved
@kunalmohan
Copy link
Collaborator Author

kunalmohan commented Jun 26, 2020

I have addressed most of the review comments. I am still unsure about the last one- about how that would be done. I'll try to dig into it but maybe it will be better to solve that along with get_mapped_range() depending how the data will be exposed to the user. I can convert this to a draft and continue implementing get_mapped_range() here itself if that's a better idea.

(This line count of PR increased just because of new TAB space for the full block of code inside run() in webgpu/lib.rs)

@kvark
kvark approved these changes Jun 26, 2020
Copy link
Member

kvark left a comment

We had a chat about this on "#wgpu:matrix.org", and it looks like the PR is on the right track:)

@kunalmohan
Copy link
Collaborator Author

kunalmohan commented Jun 27, 2020

@kvark I have changed the Unmapping process to use get_mapped_range() to write back the data to buffer while unmapping in case of mapAsync as well as mapped_at_creation.

@kvark
kvark approved these changes Jun 27, 2020
Copy link
Member

kvark left a comment

Looks reasonable, thank you!

@kvark
Copy link
Member

kvark commented Jun 27, 2020

@bors-servo
Copy link
Contributor

bors-servo commented Jun 27, 2020

📌 Commit 183a43d has been approved by kvark

@kunalmohan
Copy link
Collaborator Author

kunalmohan commented Jun 27, 2020

error[E0599]: no method named `handle_mut` found for struct `dom::bindings::trace::RootedTraceableBox<mozjs_sys::jsgc::Heap<*mut js::jsapi::JSObject>>` in the current scope
    --> components/script/dom/gpubuffer.rs:284:62
     |
284  |                         MutableHandle::from_raw(self.mapping.handle_mut()),
     |                                                              ^^^^^^^^^^ method not found in `dom::bindings::trace::RootedTraceableBox<mozjs_sys::jsgc::Heap<*mut js::jsapi::JSObject>>`
     | 
    ::: components/script/dom/bindings/trace.rs:1036:1
     |
1036 | pub struct RootedTraceableBox<T: 'static + JSTraceable> {
     | ------------------------------------------------------- method `handle_mut` not found for this
error[E0599]: no method named `handle_mut` found for struct `dom::bindings::trace::RootedTraceableBox<mozjs_sys::jsgc::Heap<_>>` in the current scope
    --> components/script/dom/gpudevice.rs:189:53
     |
189  |                     MutableHandle::from_raw(mapping.handle_mut()),
     |                                                     ^^^^^^^^^^ method not found in `dom::bindings::trace::RootedTraceableBox<mozjs_sys::jsgc::Heap<_>>`
     | 
    ::: components/script/dom/bindings/trace.rs:1036:1
     |
1036 | pub struct RootedTraceableBox<T: 'static + JSTraceable> {
     | ------------------------------------------------------- method `handle_mut` not found for this

checks failed. I didn't get this on my system earlier, but on a rebase against master I can see it failing now. I'll rectify this and push the changes.

@kunalmohan kunalmohan force-pushed the kunalmohan:gpu-buffer-mapping branch from 183a43d to 434466f Jun 27, 2020
@kunalmohan kunalmohan force-pushed the kunalmohan:gpu-buffer-mapping branch from 434466f to db2d313 Jun 27, 2020
@kvark
Copy link
Member

kvark commented Jun 27, 2020

@bors-servo
Copy link
Contributor

bors-servo commented Jun 27, 2020

📌 Commit db2d313 has been approved by kvark

@bors-servo
Copy link
Contributor

bors-servo commented Jun 27, 2020

Testing commit db2d313 with merge 1ea332e...

bors-servo added a commit that referenced this pull request Jun 27, 2020
Implement GPUBuffer.mapAsync and update wgpu-core

<!-- Please describe your changes on the following line: -->
This PR covers the following-
1. update wgpu-core to use serializable `RenderPass` and `ComputePass`. Implement `DispatchIndirect`.
2. Implement `GPUBuffer.mapAsync()`.

The `mapAsync()` methods has the following flow-
1. A new promise is created in `mapAsync()` and a message sent to the server-side. This returns a promise.
2. On the server-side, a `struct BufferMapInfo` object stores the relevant info required to send data to the client-side on completion of mapping. This object is stored in a `buffer_maps: HashMap<id::BufferId, BufferMapInfo<'a>>,` in `WGPU`. `buffers_maps` basically stores the data for buffers with pending mapping.
3. `buffer_map_async()` command is issued with a callback responsible to send the data back to the client.
4. All the devices are polled every time the server receives a new message.
5. The data is sent back on completion and promise resolved through `AsyncWGPUListener` (similar to how the `requestDevice()` and `RequestAdapter` are processed).
6. On receiving the data, the client sends back a `BufferMapComplete` message to the server to remove the entry from `buffer_maps` hashmap.

I intended to implement `getMappedRange()`, too, in this PR but it's quite a task in itself (implementing intersecting ArrayBuffers). This already being a 500+ line PR, I thought it best to get this model approved first (also it will probably be easier to review this and `getMappedRange` separately). `getMappedRange` would come up in the next one.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix #___ (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@bors-servo
Copy link
Contributor

bors-servo commented Jun 27, 2020

💔 Test failed - status-taskcluster

@jdm
Copy link
Member

jdm commented Jun 27, 2020

@bors-servo
Copy link
Contributor

bors-servo commented Jun 27, 2020

Testing commit db2d313 with merge af110ac...

@bors-servo
Copy link
Contributor

bors-servo commented Jun 27, 2020

☀️ Test successful - status-taskcluster
Approved by: kvark
Pushing af110ac to master...

@bors-servo bors-servo merged commit af110ac into servo:master Jun 27, 2020
2 checks passed
2 checks passed
Community-TC (pull_request) TaskGroup: success
Details
homu Test successful
Details
WebGPU MVP automation moved this from In progress to Done Jun 27, 2020
@kunalmohan kunalmohan deleted the kunalmohan:gpu-buffer-mapping branch Jun 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
WebGPU MVP
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.