Implement GPUBuffer.mapAsync and update wgpu-core by kunalmohan · Pull Request #27084 · servo/servo

kunalmohan · 2020-06-25T14:19:48Z

This PR covers the following-

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

The mapAsync() methods has the following flow-

A new promise is created in mapAsync() and a message sent to the server-side. This returns a promise.
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.
buffer_map_async() command is issued with a callback responsible to send the data back to the client.
All the devices are polled every time the server receives a new message.
The data is sent back on completion and promise resolved through AsyncWGPUListener (similar to how the requestDevice() and RequestAdapter are processed).
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 · 2020-06-25T14:19:53Z

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 · 2020-06-25T14:19:55Z

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 · 2020-06-25T14:20:09Z

r?@kvark

kvark · 2020-06-25T17:25:53Z

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.

kvark

Very nice! Just a few notes here

components/script/dom/gpubuffer.rs

components/webgpu/lib.rs

kunalmohan · 2020-06-26T17:25:57Z

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

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

kunalmohan · 2020-06-27T13:06:40Z

@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

Looks reasonable, thank you!

kvark · 2020-06-27T15:17:19Z

@bors-servo r+

bors-servo · 2020-06-27T15:17:20Z

📌 Commit 183a43d has been approved by kvark

kunalmohan · 2020-06-27T15:21:21Z

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.

kvark · 2020-06-27T16:08:06Z

@bors-servo r+

bors-servo · 2020-06-27T16:08:07Z

📌 Commit db2d313 has been approved by kvark

bors-servo · 2020-06-27T16:42:37Z

⌛ Testing commit db2d313 with merge 1ea332e...

Implement GPUBuffer.mapAsync and update wgpu-core  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. ---  - [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)  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___

bors-servo · 2020-06-27T17:46:24Z

💔 Test failed - status-taskcluster

jdm · 2020-06-27T17:54:46Z

@bors-servo retry

Intermittent timeouts in linux WPT media tests #25522

bors-servo · 2020-06-27T18:29:12Z

⌛ Testing commit db2d313 with merge af110ac...

bors-servo · 2020-06-27T19:29:11Z

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

highfive assigned asajeffrey Jun 25, 2020

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jun 25, 2020

highfive assigned kvark and unassigned asajeffrey Jun 25, 2020

kvark suggested changes Jun 25, 2020

View reviewed changes

kvark approved these changes Jun 26, 2020

View reviewed changes

kunalmohan added 4 commits June 27, 2020 20:27

Implement GPUBuffer.mapAsync and update wgpu-core

b74cea3

Remove peek-poke and peek-poke-derive from ignored packages

e6f0d12

address review comments

ef3b141

Ensure proper unmap of buffer

b484836

kvark approved these changes Jun 27, 2020

View reviewed changes

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jun 27, 2020

kunalmohan force-pushed the gpu-buffer-mapping branch from 183a43d to 434466f Compare June 27, 2020 15:52

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Jun 27, 2020

Fix ArrayBuffer creation in buffer mapping

db2d313

kunalmohan force-pushed the gpu-buffer-mapping branch from 434466f to db2d313 Compare June 27, 2020 16:02

highfive removed the S-awaiting-review There is new code that needs to be reviewed. label Jun 27, 2020

highfive added the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Jun 27, 2020

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Jun 27, 2020

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-tests-failed The changes caused existing tests to fail. labels Jun 27, 2020

bors-servo merged commit af110ac into servo:master Jun 27, 2020

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Jun 27, 2020

This was referenced Jun 27, 2020

Support for webxr layer management #26927

Merged

Introduce dynamic module #27026

Merged

kunalmohan deleted the gpu-buffer-mapping branch June 27, 2020 19:42

Uh oh!

Conversation

kunalmohan commented Jun 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

highfive commented Jun 25, 2020

Uh oh!

highfive commented Jun 25, 2020

Uh oh!

kunalmohan commented Jun 25, 2020

Uh oh!

kvark commented Jun 25, 2020

Uh oh!

kvark left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kunalmohan commented Jun 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kvark left a comment

Choose a reason for hiding this comment

Uh oh!

kunalmohan commented Jun 27, 2020

Uh oh!

kvark left a comment

Choose a reason for hiding this comment

Uh oh!

kvark commented Jun 27, 2020

Uh oh!

bors-servo commented Jun 27, 2020

Uh oh!

kunalmohan commented Jun 27, 2020

Uh oh!

kvark commented Jun 27, 2020

Uh oh!

bors-servo commented Jun 27, 2020

Uh oh!

bors-servo commented Jun 27, 2020

Uh oh!

bors-servo commented Jun 27, 2020

Uh oh!

jdm commented Jun 27, 2020

Uh oh!

bors-servo commented Jun 27, 2020

Uh oh!

bors-servo commented Jun 27, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

kunalmohan commented Jun 25, 2020 •

edited

Loading

kunalmohan commented Jun 26, 2020 •

edited

Loading