Only start WebGPU thread if an adapter is requested #25030

zakorgy · 2019-12-03T12:03:33Z

This addresses 1 and 2 from #24706 (comment)
We send a message to constellation instead of creating the WebGPU thread on the start. We send back the result to script and set the Window's web_gpu component there.
cc @jdm @imiklos

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #___ (GitHub issue number if applicable)

This change is

highfive · 2019-12-03T12:03:39Z

Heads up! This PR modifies the following files:

@asajeffrey: components/constellation/constellation.rs, components/script/dom/bindings/trace.rs, components/script/dom/window.rs, components/script/dom/gpu.rs
@cbrewster: components/constellation/constellation.rs
@KiChjang: components/script_traits/script_msg.rs, components/script/dom/bindings/trace.rs, components/script/dom/window.rs, components/script/dom/gpu.rs

highfive · 2019-12-03T12:03:41Z

Warning

These commits modify script code, but no tests are modified. Please consider adding a test!

gterzian · 2019-12-03T13:55:41Z

components/constellation/constellation.rs

@@ -1945,6 +1945,12 @@ where
                    EmbedderMsg::MediaSessionEvent(event),
                ));
            },
+            FromScriptMsg::StartWebGPU(sender) => {
+                self.webgpu = WebGPU::new();


Even though you are tracking the webgpu_channel_requested flag inside the DOM GPU, multiple script-threads(EventLoop from the perspective of the constellation) could send this message, in which case you would be overwriting previously set self.webgpu(and running multiple gpu threads?).

Also, even within one script-thread, multiple windows could be running on it(see documents in ScriptThread, pipelines in the constellation), in which case I think you could also have multiple DOM GPU instances, each tracking their own webgpu_channel_requested flag.

So I think you need to decide:

Do you want to allow multiple DOM GPU to share a WebGPU at the same time? Is so, how would they be shared(one webgpu shared across the browser, one shared per origin, per agent-cluster?). Then the StartWebGPU would need to include information so that you can either clone an existing one, or start a new one.

Do you want to allow multiple parallel WebGPU at all? In that case, you would need a map with a key representing how an instance is shared, so that you could get it out here, clone the sender, or start a new one.

It's not clear to me from the spec how sharing should happen, although @kvark has previously said multiple parallel applications are wanted(although it's not clear if that means sharing a single webgpu, or running multiple ones, or a combination of that).

The easiest for now might be to simply share a single webgpu instance across the whole browser, in which case all you have to do here is to avoid overwriting an existing one.

Yes, the choice of boundaries for separating WebGPU instances is still ours.

Here is how it works in Gecko, for the reference:

each content process manages multiple tabs

a single WebGPU remote client is established per content process

it spawns a separate WebGPU server on the GPU process

the IPC is a child of CompositorBridge and thus the server message loop is happening on a thread that this parent protocol manages (I assume it's a separate thread)

Does Servo have multiple content processes? I think we can start here by having the same approach - one WebGPU client-server pair per content process.

Yes so a content-process would correspond to a EventLoop in Servo(which is an interface for the constellation to communicate with the underlying ScriptThread that is running in another process). I'd say it maps to the spec concept of agent-cluster.

From the constellation's perspective a document corresponds to a Pipeline, of which multiple can run on one event-loop. Associated dedicated workers also run in the same "event-loop", although on their own thread, not the script-thread(they do have a pipeline id, which I think is the id of the document that started them, to be looked into).

When a pipeline is spawned, an event-loop is either created or re-used, see

servo/components/constellation/pipeline.rs

Line 219 in 6e3c131

pub fn spawn<Message, LTF, STF>(state: InitialPipelineState) -> Result<NewPipeline, Error>

The "re-using" of event-loops is done when a pipeline is part of the same BrowsingContextGroup, and based on the Host of the url of that pipeline, see get_event_loop.

So, this would actually be the first time something is shared like that on an "per-event-loop" basis, and you're going to have to provide some infra for this, because currently the constellation receives message from a given pipeline(see how each ScriptMsg comes with a PipelineID, representing the pipeline from which the message originated).

I think you could do something like:

As suggested in the other comment, make requesting an adapater a ScriptMsg.

When you receive it:

get the corresponding pipeline

get the top-level browsing context for that pipeline.

get the host for that pipeline.

The above info should be enough to then grab the right webgpu instance, or create it.

I would suggest you store them on a HashMap<Host, WebGPU> inside a BrowsingContextGroup, similarly to what is done with EventLoop.

gterzian · 2019-12-03T14:15:06Z

components/script/dom/gpu.rs

+            let (sender, receiver) = ipc::channel().unwrap();
+            let script_to_constellation_chan = global.script_to_constellation_chan();
+            if script_to_constellation_chan
+                .send(ScriptMsg::StartWebGPU(sender))


So I would say you could make the WebGPURequest::RequestAdapter into a ScriptMsg, and in RequestAdapter below just use the script_to_constellation_chan to send it to the constellation.

Then the constellation can

Start a webgpu if needed,

forward the RequestAdapter call.

And webgpu can then reply on a channel included in the RequestAdapter message with a webgpu_channel which can be stored in the adapter(so that it can communicate directly with webgpu going forward).

The main benefit is not blocking on the constellation reply inside the wgpu_channel call, instead making it non-blocking as the actual RequestAdapter call, where the reply eventually resolves via a promise.

You cold store the webgpu_channel somewhere besides the adapter, however it doesn't seem to have much benefit at this point(since the sole RequestAdapter method would always have to go through the constellation first). So it would only make sense to store it internally on the adapter, so that methods of it could send a message directly to webgpu without having to go through the constellation.

zakorgy · 2019-12-05T08:53:43Z

@gterzian @kvark thanks for the well detailed descriptions, I will update my PR according to what you described.

bors-servo · 2019-12-05T19:48:58Z

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

zakorgy · 2020-01-09T12:47:36Z

@gterzian @kvark it took me some time but finally updated the PR to use the solution described in #25030 (comment).

jdm · 2020-01-09T13:45:48Z

components/constellation/constellation.rs

+            Some(browsing_context_group) => {
+                match browsing_context_group.webgpus.entry(host) {
+                    Entry::Vacant(v) => {
+                        let webgpu = v.insert(WebGPU::new().unwrap());


Note: using unwrap in the constellation isn't allowed (and our CI catches it), so please turn this into an error! if it fails.

gterzian

Ok, I think it looks very good, great work!

Before I review everything in details, there are two small changes in the overall structure I would like to suggest:

Do we need the Option<WebGPU> in WebGPUResponse::RequestAdapter? Could we make sending back the channel non-optional(It doesn't seem to be about conveying failure to create an adapter)?
I would like to try to get rid of window.wegbu_channel and window.set_webgpu_channel. Instead, you could store the channel on the GPUAdapter directly, when you handle WebGPUResponse::RequestAdapter. Then, when the adapter creates a device, it could simply clone it's channel and store it directly on the device when handling the WebGPUResponse::RequestDevice(and the device could clone it's own sender when creating a buffer, and so on, without having to go through the window everytime)

zakorgy · 2020-01-09T14:18:27Z

@gterzian those changes sounds doable for me. I will start adding them.

zakorgy · 2020-01-09T16:18:51Z

@gterzian I've addressed your requested changes, could you please take a look now?

gterzian

Ok it looks good to me. I have one small comment, and I'll take another read after it has been addressed.

gterzian · 2020-01-10T07:18:03Z

components/webgpu/lib.rs

@@ -16,7 +16,7 @@ use smallvec::SmallVec;

 #[derive(Debug, Deserialize, Serialize)]
 pub enum WebGPUResponse {
-    RequestAdapter(String, WebGPUAdapter),
+    RequestAdapter(String, WebGPUAdapter, WebGPU),


Could it make sense for the WGPU to keep a sender to itself, so that the constellation doesn't have to send it in the request?

I think yes. Updated the code according to that.

gterzian

Ok looks good, just a few last suggestions.

gterzian · 2020-01-11T14:05:54Z

components/constellation/constellation.rs

+            .flatten();
+
+        for receiver in receivers {
+            if let Some(rec) = receiver {


I think you can use filter_map on webgpus.values() above to filter out the None.

gterzian · 2020-01-11T14:05:54Z

components/script/dom/gpuadapter.rs

@@ -28,6 +29,8 @@ use webgpu::{wgpu, WebGPUAdapter, WebGPURequest, WebGPUResponse};
 #[dom_struct]
 pub struct GPUAdapter {
    reflector_: Reflector,
+    #[ignore_malloc_size_of = "channels are hard"]
+    channel: IpcSender<WebGPURequest>,


Perhaps use WebGPU instead?

gterzian · 2020-01-11T14:05:54Z

components/script/dom/gpuadapter.rs

+                .send(WebGPURequest::RequestDevice(sender, self.adapter, desc, id))
+                .is_err()
+            {
+                promise.reject_error(Error::Type(


They spec says to use Error::Operation https://gpuweb.github.io/gpuweb/#requestdevice

gterzian · 2020-01-11T14:05:54Z

components/script/dom/gpuadapter.rs

+            {
+                promise.reject_error(Error::Type(
+                    "Failed to send RequestDevice message...".to_owned(),
+                ));
            }
        } else {
            promise.reject_error(Error::Type("No WebGPU thread...".to_owned()))


Error::Operation

gterzian · 2020-01-11T14:05:54Z

components/script/dom/gpudevice.rs

+                    id,
+                    wgpu_descriptor,
+                ))
+                .unwrap();


expect("Failed to create WebGPU buffer")

gterzian · 2020-01-11T14:05:54Z

components/script/dom/gpudevice.rs

+                    id,
+                    wgpu_descriptor.clone(),
+                ))
+                .unwrap()



        Only start WebGPU thread if an adapter is requested


        Fix returned error types in WebGPU

zakorgy · 2020-01-13T09:45:21Z

@gterzian updated the PR with your proposed changes, also added a separate commit where I've addressed #25492

gterzian · 2020-01-13T14:32:49Z

FIX #25492

Great job!

@bors-servo r+

bors-servo · 2020-01-13T14:32:51Z

📌 Commit 430248e has been approved by gterzian

zakorgy · 2020-01-13T14:35:32Z

@gterzian thanks for the guidance and the valuable information about Servo infrastructure.

bors-servo · 2020-01-13T14:47:13Z

⌛ Testing commit 430248e with merge 81e4f17...


        Auto merge of #25030 - szeged:webgpu-constellation, r=gterzian

@jdm

Only start WebGPU thread if an adapter is requested  This addresses 1 and 2 from #24706 (comment) We send a message to constellation instead of creating the `WebGPU` thread on the start. We send back the result to script and set the `Window`'s `web_gpu` component there. cc @jdm @imiklos ---  - [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)    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/25030)

bors-servo · 2020-01-13T15:21:56Z

💔 Test failed - status-taskcluster

jdm · 2020-01-13T15:41:34Z

@bors-servo retry

#24726

bors-servo · 2020-01-13T16:03:33Z

⌛ Testing commit 430248e with merge c1e1dea...


        Auto merge of #25030 - szeged:webgpu-constellation, r=gterzian

@jdm

Only start WebGPU thread if an adapter is requested  This addresses 1 and 2 from #24706 (comment) We send a message to constellation instead of creating the `WebGPU` thread on the start. We send back the result to script and set the `Window`'s `web_gpu` component there. cc @jdm @imiklos ---  - [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)    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/25030)

bors-servo · 2020-01-13T16:20:43Z

💔 Test failed - status-taskcluster

jdm · 2020-01-13T16:33:21Z

@bors-servo retry

#24726


        Auto merge of #25030 - szeged:webgpu-constellation, r=gterzian

@jdm

Only start WebGPU thread if an adapter is requested  This addresses 1 and 2 from #24706 (comment) We send a message to constellation instead of creating the `WebGPU` thread on the start. We send back the result to script and set the `Window`'s `web_gpu` component there. cc @jdm @imiklos ---  - [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)    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/25030)

bors-servo · 2020-01-13T16:46:59Z

⌛ Testing commit 430248e with merge 0131952...

bors-servo · 2020-01-13T17:05:16Z

☀️ Test successful - status-taskcluster
Approved by: gterzian
Pushing 0131952 to master...

highfive assigned SimonSapin Dec 3, 2019

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

gterzian reviewed Dec 3, 2019

View changes

jdm assigned gterzian and unassigned SimonSapin Dec 4, 2019

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

gterzian mentioned this pull request Dec 15, 2019

Initial implementation of GPUBuffer for WebGPU #25264

Merged

3 of 3 tasks complete

zakorgy force-pushed the szeged:webgpu-constellation branch 2 times, most recently from ea1f917 to c023e0c Jan 9, 2020

zakorgy force-pushed the szeged:webgpu-constellation branch from c023e0c to 4ae3c8b Jan 9, 2020

jdm reviewed Jan 9, 2020

View changes

jdm added S-fails-tidy and removed S-needs-rebase labels Jan 9, 2020

gterzian suggested changes Jan 9, 2020

View changes

zakorgy requested a review from gterzian Jan 9, 2020

zakorgy force-pushed the szeged:webgpu-constellation branch from 4ae3c8b to 4a72a2c Jan 9, 2020

zakorgy force-pushed the szeged:webgpu-constellation branch from 4a72a2c to 71d5f8b Jan 9, 2020

gterzian suggested changes Jan 10, 2020

View changes

gterzian removed the S-fails-tidy label Jan 10, 2020

zakorgy force-pushed the szeged:webgpu-constellation branch from 71d5f8b to 3740754 Jan 10, 2020

gterzian suggested changes Jan 11, 2020

View changes

Only start WebGPU thread if an adapter is requested

a751b1c

Fix returned error types in WebGPU

Loading status checks…

430248e

zakorgy force-pushed the szeged:webgpu-constellation branch from 3740754 to 430248e Jan 13, 2020

highfive added S-awaiting-merge and removed S-awaiting-review labels Jan 13, 2020

highfive added S-tests-failed and removed S-awaiting-merge labels Jan 13, 2020

highfive added S-awaiting-merge and removed S-tests-failed labels Jan 13, 2020

highfive added S-tests-failed and removed S-awaiting-merge labels Jan 13, 2020

highfive added S-awaiting-merge and removed S-tests-failed labels Jan 13, 2020

bors-servo merged commit 430248e into servo:master Jan 13, 2020
2 checks passed

2 checks passed

Community-TC (pull_request) TaskGroup: success
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Jan 13, 2020

atouchet mentioned this pull request Jan 13, 2020

In AsyncWGPUListener, reject promise with OperationError #25492

Closed

servo / servo

Only start WebGPU thread if an adapter is requested #25030

Only start WebGPU thread if an adapter is requested #25030

zakorgy commented Dec 3, 2019 •

edited

highfive commented Dec 3, 2019

highfive commented Dec 3, 2019

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

zakorgy commented Dec 5, 2019

bors-servo commented Dec 5, 2019

zakorgy commented Jan 9, 2020

This comment has been minimized.

gterzian left a comment •

edited

zakorgy commented Jan 9, 2020

zakorgy commented Jan 9, 2020

gterzian left a comment

This comment has been minimized.

This comment has been minimized.

gterzian left a comment

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.

zakorgy commented Jan 13, 2020

gterzian commented Jan 13, 2020

bors-servo commented Jan 13, 2020

zakorgy commented Jan 13, 2020

bors-servo commented Jan 13, 2020

bors-servo commented Jan 13, 2020

jdm commented Jan 13, 2020

bors-servo commented Jan 13, 2020

bors-servo commented Jan 13, 2020

jdm commented Jan 13, 2020

bors-servo commented Jan 13, 2020

bors-servo commented Jan 13, 2020

servo / servo

Join GitHub today

Only start WebGPU thread if an adapter is requested #25030

Only start WebGPU thread if an adapter is requested #25030

Conversation

zakorgy commented Dec 3, 2019 • edited

highfive commented Dec 3, 2019

highfive commented Dec 3, 2019

This comment has been minimized.

gterzian Dec 3, 2019 • edited

This comment has been minimized.

This comment has been minimized.

gterzian Dec 4, 2019 • edited

This comment has been minimized.

gterzian Dec 3, 2019 • edited

zakorgy commented Dec 5, 2019

bors-servo commented Dec 5, 2019

zakorgy commented Jan 9, 2020

This comment has been minimized.

gterzian left a comment • edited

zakorgy commented Jan 9, 2020

zakorgy commented Jan 9, 2020

gterzian left a comment

This comment has been minimized.

gterzian Jan 10, 2020 • edited

This comment has been minimized.

gterzian left a comment

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.

zakorgy commented Jan 13, 2020

gterzian commented Jan 13, 2020

bors-servo commented Jan 13, 2020

zakorgy commented Jan 13, 2020

bors-servo commented Jan 13, 2020

bors-servo commented Jan 13, 2020

jdm commented Jan 13, 2020

bors-servo commented Jan 13, 2020

bors-servo commented Jan 13, 2020

jdm commented Jan 13, 2020

bors-servo commented Jan 13, 2020

bors-servo commented Jan 13, 2020

zakorgy commented Dec 3, 2019 •

edited

gterzian Dec 3, 2019 •

edited

gterzian Dec 4, 2019 •

edited

gterzian Dec 3, 2019 •

edited

gterzian left a comment •

edited

gterzian Jan 10, 2020 •

edited