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

Make WebGPU resource creation async and prevent panic during shutdown if WebGPU is enabled. #26660

Merged
merged 1 commit into from May 27, 2020

Conversation

@kunalmohan
Copy link
Collaborator

kunalmohan commented May 26, 2020

  1. Make WebGPU resource creation async.
  2. Remove some unused code in WebGPURequest::RequestAdapter.
  3. Prevent panic during shutdown. Since WGPU thread is killed before script, sender and receiver in the script panic at either of the two places-
    a. If a buffer is still alive, script tries to send WebGPURequest::DestroyBuffer to server while dropping the buffer during shutdown.
    impl Drop for GPUBuffer {
    fn drop(&mut self) {
    self.Destroy()
    }
    }
    self.channel
    .0
    .send(WebGPURequest::DestroyBuffer(self.buffer.0))
    .unwrap();
    *self.state.borrow_mut() = GPUBufferState::Destroyed;

    b. Receiver in script-thread panics with RecvError as soon as sender on server side is dropped.
    recv(self.webgpu_port.borrow().as_ref().unwrap_or(&crossbeam_channel::never())) -> msg
    => FromWebGPUServer(msg.unwrap()),

r?@kvark


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

highfive commented May 26, 2020

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/script_thread.rs, components/script/dom/gpubuffer.rs, components/script/dom/gpudevice.rs
  • @KiChjang: components/script/script_thread.rs, components/script/dom/gpubuffer.rs, components/script/dom/gpudevice.rs
@highfive
Copy link

highfive commented May 26, 2020

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@kunalmohan kunalmohan added this to In progress in WebGPU MVP via automation May 26, 2020
@kvark
kvark approved these changes May 26, 2020
Copy link
Member

kvark left a comment

Looks great!

We still have a few senders in message variants that we need to consider fro removal, for example one in CommandEncoderFinish seems unnecessary.

@kunalmohan
Copy link
Collaborator Author

kunalmohan commented May 26, 2020

Right. I must have missed that. I'll remove that one here itself.

Remove some garbage code in adapter id checks
Prevent panic during shutdown if using WebGPU
@kunalmohan kunalmohan force-pushed the kunalmohan:gpu-async-resource branch from a66689a to f4d0183 May 26, 2020
@jdm
Copy link
Member

jdm commented May 26, 2020

@bors-servo r=kvark

@bors-servo
Copy link
Contributor

bors-servo commented May 26, 2020

📌 Commit f4d0183 has been approved by kvark

@bors-servo
Copy link
Contributor

bors-servo commented May 27, 2020

Testing commit f4d0183 with merge 07eb9ab...

@bors-servo
Copy link
Contributor

bors-servo commented May 27, 2020

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

@bors-servo bors-servo merged commit 07eb9ab into servo:master May 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 May 27, 2020
@kunalmohan kunalmohan deleted the kunalmohan:gpu-async-resource branch May 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.

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