Initial implementation of GPUBuffer for WebGPU #25264

imiklos · 2019-12-12T12:13:33Z

Added WebIDL bindings for GPUBuffer, GPUBufferDescriptor, GPUBufferUsage.
Implemented the createBuffer and createBufferMapped functions of GPUDevice.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes addresses a part of #24706

cc @kvark @jdm @zakorgy

This change is

highfive · 2019-12-12T12:13:40Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/gpuadapter.rs, components/script/dom/gpubufferusage.rs, components/script/dom/navigator.rs, components/script/dom/mod.rs, components/script/dom/identityhub.rs and 6 more
@KiChjang: components/script/dom/gpuadapter.rs, components/script/dom/gpubufferusage.rs, components/script/dom/navigator.rs, components/script/dom/mod.rs, components/script/dom/identityhub.rs and 6 more

highfive · 2019-12-12T12:13:42Z

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!

jdm · 2019-12-12T14:40:54Z

components/script/dom/gpubuffer.rs

+            },
+            _ => {},
+        };
+        if let Some(window) = self.global().downcast::<Window>() {


Since this method is called from the destructor, instead of getting the channel from the global we should hold on to the channel so we can use it directly.

I think "holding on to the channel so we can use it directly" will be generally addressed in #25030, it might be a good idea to coordinate with @zakorgy and perhaps rebase this one off of those changes when they are completed(or do it in a follow-up requiring some refactoring).

I've saved the channel in the GPUBuffer so we can send requests directly.

jdm · 2019-12-12T14:40:54Z

components/webgpu/lib.rs

+                        global.device_create_buffer_mapped(device.0, &descriptor, &mut arr_buff, id));
+                    let buffer = WebGPUBuffer(id);
+
+                    if let Err(e) = sender.send((buffer, arr_buff as usize)) {


This sends a pointer over IPC, so this won't work with multiple processes. How do we deal with this? Do we need to copy the memory into a Vec and send that instead?

Yes, you are right. I've copied the memory into a Vec as you mentioned.

jdm · 2019-12-12T14:40:54Z

components/script/dom/gpudevice.rs

@@ -63,6 +75,74 @@ impl GPUDevice {
    }
 }

+impl GPUDevice {
+    fn resolve_create_buffer_mapped(


This should be marked unsafe, since it accepts an arbitrary integer, converts it to a pointer, and dereferences it by using it to create a slice.

jdm · 2019-12-12T14:40:54Z

components/script/dom/gpudevice.rs

+        );
+        unsafe { buff.to_jsval(*cx, js_gpu_buffer.handle_mut()) };
+        let mut out = Vec::new();
+        out.push(js_gpu_buffer.get());


This can be ObjectValue(buff.reflector().get_jsobject().get()), and then we don't need js_gpu_buffer.

kvark

Reviewed 14 of 14 files at r1.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @imiklos)

components/script/dom/gpubuffer.rs, line 32 at r1 (raw file):

    label: DomRefCell<Option<DOMString>>,
    size: Cell<GPUBufferSize>,
    usage: Cell<u32>,

usage and size shouldn't be mutable: they are set at creation and persist across the life time

components/script/dom/gpubufferusage.rs, line 9 at r1 (raw file):

#[dom_struct]
pub struct GPUBufferUsage {

does this have to be an interface?
There are no methods expected, it's just a namespace, conceptually.
In Gecko, we mark those interfaces as concrete: false and don't need any classes on C++ side for them.

components/script/dom/gpudevice.rs, line 190 at r1 (raw file):

                    .unwrap(),
                None => {
                    return GPUBuffer::new(

if the GPU process is dead and the channel is not available, we should probably not pretend we are doing anything constructive here

components/script/dom/gpudevice.rs, line 246 at r1 (raw file):

        };

        let (buffer, array_ptr_address) = receiver.recv().unwrap();

we can't pass around pointers between processes, as @jdm noted already

imiklos

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @imiklos, @jdm, @kvark, and @zakorgy)

components/script/dom/gpubuffer.rs, line 32 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

usage and size shouldn't be mutable: they are set at creation and persist across the life time

Thank you! I will remove the Cell.

components/script/dom/gpubufferusage.rs, line 9 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

does this have to be an interface?
There are no methods expected, it's just a namespace, conceptually.
In Gecko, we mark those interfaces as concrete: false and don't need any classes on C++ side for them.

I didn't find similar to concrete: false in servo. @jdm Do you have an idea about this?

components/script/dom/gpudevice.rs, line 190 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

if the GPU process is dead and the channel is not available, we should probably not pretend we are doing anything constructive here

You are right! I've just put an unimplemented! macro instead of returning with a GPUBuffer here.

components/script/dom/gpudevice.rs, line 246 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

we can't pass around pointers between processes, as @jdm noted already

Yes, I've copied the memory into a Vec and sent that instead of pointers.

imiklos · 2019-12-16T14:12:05Z

@jdm @kvark @gterzian Thank your for your review! I've addressed what you mentioned.

imiklos · 2019-12-16T14:25:18Z

@kvark I get the following validation layer error on destruction:
(this is an unmapped buffer created with device.createBuffer)

[2019-12-16T13:09:39Z ERROR relevant] Values of this type can't be dropped!
[2019-12-16T13:09:39Z ERROR gfx_backend_vulkan]
VALIDATION [VUID-vkDestroyDevice-device-00378 (0)] : OBJ ERROR : For device 0x7fac702eac70, DeviceMemory object 0x3 has not been destroyed. The Vulkan spec states: All child objects created on device must have been destroyed prior to destroying device (https://www.khronos.org/registry/vulkan/specs/1.1-extensions/html/vkspec.html#VUID-vkDestroyDevice-device-00378)
object info: (type: DEVICE_MEMORY, hndl: 3)

[2019-12-16T13:09:39Z ERROR relevant] Values of this type can't be dropped!

I got the same result with explicitly call the Buffer.destroy() or without.
The Global.delete() called before exit.
Am I missing something?

kvark · 2019-12-16T15:13:25Z

I get the following validation layer error on destruction:

This comes from rendy-memory, which you need to double-check if you are destroying correctly.

bors-servo · 2019-12-17T05:12:50Z

☔ The latest upstream changes (presumably #25299) made this pull request unmergeable. Please resolve the merge conflicts.


        Initial implementation of GPUBuffer for WebGPU

Added WebIDL bindings for GPUBuffer, GPUBufferDescriptor, GPUBufferUsage Implemented the `createBuffer` and `createBufferMapped` functions of GPUDevice

jdm · 2019-12-17T14:08:07Z

@bors-servo r+

bors-servo · 2019-12-17T14:08:08Z

📌 Commit ebfcd0f has been approved by jdm

bors-servo · 2019-12-17T15:38:34Z

⌛ Testing commit ebfcd0f with merge c9a7c0e...


        Auto merge of #25264 - szeged:wgpu_buffer, r=jdm

@kvark

Initial implementation of GPUBuffer for WebGPU Added WebIDL bindings for `GPUBuffer`, `GPUBufferDescriptor`, `GPUBufferUsage`. Implemented the `createBuffer` and `createBufferMapped` functions of `GPUDevice`. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes addresses a part of #24706   cc @kvark @jdm @zakorgy  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/25264)

bors-servo · 2019-12-17T16:22:55Z

💔 Test failed - status-taskcluster

jdm · 2019-12-17T16:38:39Z

@bors-servo retry

#24726

bors-servo · 2019-12-17T17:44:44Z

⌛ Testing commit ebfcd0f with merge f86a027...


        Auto merge of #25264 - szeged:wgpu_buffer, r=jdm

@kvark

Initial implementation of GPUBuffer for WebGPU Added WebIDL bindings for `GPUBuffer`, `GPUBufferDescriptor`, `GPUBufferUsage`. Implemented the `createBuffer` and `createBufferMapped` functions of `GPUDevice`. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes addresses a part of #24706   cc @kvark @jdm @zakorgy  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/25264)

bors-servo · 2019-12-17T18:01:40Z

💔 Test failed - status-taskcluster

jdm · 2019-12-17T18:43:36Z

@bors-servo retry

#24726

bors-servo · 2019-12-17T19:25:08Z

⌛ Testing commit ebfcd0f with merge cea55e3...


        Auto merge of #25264 - szeged:wgpu_buffer, r=jdm

@kvark

Initial implementation of GPUBuffer for WebGPU Added WebIDL bindings for `GPUBuffer`, `GPUBufferDescriptor`, `GPUBufferUsage`. Implemented the `createBuffer` and `createBufferMapped` functions of `GPUDevice`. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes addresses a part of #24706   cc @kvark @jdm @zakorgy  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/25264)

bors-servo · 2019-12-17T19:43:46Z

☀️ Test successful - status-taskcluster
Approved by: jdm
Pushing cea55e3 to master...

highfive assigned jdm Dec 12, 2019

highfive added the S-awaiting-review label Dec 12, 2019

jdm requested changes Dec 12, 2019

View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Dec 12, 2019

kvark reviewed Dec 12, 2019

View changes

imiklos reviewed Dec 16, 2019

View changes

imiklos force-pushed the szeged:wgpu_buffer branch from 706c2ac to 2033790 Dec 16, 2019

highfive added S-awaiting-review and removed S-needs-code-changes labels Dec 16, 2019

jdm approved these changes Dec 16, 2019

View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Dec 16, 2019

highfive added the S-needs-rebase label Dec 17, 2019

Initial implementation of GPUBuffer for WebGPU

Loading status checks…

ebfcd0f

Added WebIDL bindings for GPUBuffer, GPUBufferDescriptor, GPUBufferUsage Implemented the `createBuffer` and `createBufferMapped` functions of GPUDevice

imiklos force-pushed the szeged:wgpu_buffer branch from 2033790 to ebfcd0f Dec 17, 2019

highfive added S-awaiting-review and removed S-needs-code-changes labels Dec 17, 2019

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Dec 17, 2019

highfive added S-tests-failed and removed S-awaiting-merge labels Dec 17, 2019

highfive added S-awaiting-merge and removed S-tests-failed labels Dec 17, 2019

highfive added S-tests-failed and removed S-awaiting-merge labels Dec 17, 2019

highfive added S-awaiting-merge and removed S-tests-failed labels Dec 17, 2019

bors-servo merged commit ebfcd0f into servo:master Dec 17, 2019
2 checks passed

2 checks passed

Community-TC (pull_request) TaskGroup: success
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Dec 17, 2019

kvark deleted the szeged:wgpu_buffer branch Dec 19, 2019

servo / servo

Initial implementation of GPUBuffer for WebGPU #25264

Initial implementation of GPUBuffer for WebGPU #25264

imiklos commented Dec 12, 2019 •

edited

highfive commented Dec 12, 2019

highfive commented Dec 12, 2019

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

kvark left a comment

imiklos left a comment

imiklos commented Dec 16, 2019

imiklos commented Dec 16, 2019

kvark commented Dec 16, 2019

bors-servo commented Dec 17, 2019

jdm commented Dec 17, 2019

bors-servo commented Dec 17, 2019

bors-servo commented Dec 17, 2019

bors-servo commented Dec 17, 2019

jdm commented Dec 17, 2019

bors-servo commented Dec 17, 2019

bors-servo commented Dec 17, 2019

jdm commented Dec 17, 2019

bors-servo commented Dec 17, 2019

bors-servo commented Dec 17, 2019

servo / servo

Join GitHub today

Initial implementation of GPUBuffer for WebGPU #25264

Initial implementation of GPUBuffer for WebGPU #25264

Conversation

imiklos commented Dec 12, 2019 • edited

highfive commented Dec 12, 2019

highfive commented Dec 12, 2019

This comment has been minimized.

jdm Dec 12, 2019

This comment has been minimized.

gterzian Dec 15, 2019

This comment has been minimized.

imiklos Dec 16, 2019

This comment has been minimized.

jdm Dec 12, 2019

This comment has been minimized.

imiklos Dec 16, 2019

This comment has been minimized.

jdm Dec 12, 2019

This comment has been minimized.

jdm Dec 12, 2019

kvark left a comment

imiklos left a comment

imiklos commented Dec 16, 2019

imiklos commented Dec 16, 2019

kvark commented Dec 16, 2019

bors-servo commented Dec 17, 2019

jdm commented Dec 17, 2019

bors-servo commented Dec 17, 2019

bors-servo commented Dec 17, 2019

bors-servo commented Dec 17, 2019

jdm commented Dec 17, 2019

bors-servo commented Dec 17, 2019

bors-servo commented Dec 17, 2019

jdm commented Dec 17, 2019

bors-servo commented Dec 17, 2019

bors-servo commented Dec 17, 2019

imiklos commented Dec 12, 2019 •

edited