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 WebGPU ErrorScopes #27285

Merged
merged 4 commits into from Jul 17, 2020
Merged

Implement WebGPU ErrorScopes #27285

merged 4 commits into from Jul 17, 2020

Conversation

kunalmohan
Copy link
Collaborator

@kunalmohan kunalmohan commented Jul 15, 2020

This PR covers the following-

  1. update wgpu-core (Rustify RenderPipelineDescriptor)
  2. Add initial implementation of pushErrorScope and popErrorScope.
  3. Use existing BGL if an equivalent already exists.

A brief explanation of the ErrorScope Implementation-

  1. We store HashMap<u64, ErrorScopeInfo> in GPUDevice. ErrorScopeInfo is a new struct used to store all necessary info for an ErrorScope.
  2. We simultaneously store Vec<u64> to keep track of the ErrorScope hierarchy. next_scope_id is used to get the scope_id for a new Scope.
  3. We send the scope_id of top-level scope to the server in the message for an operation. The server performs the operation and sends back the scope_id with the result message (only if a scope_id was sent from the client. Otherwise no message is sent.).
  4. We store a HashMap<WebGPUDevice, Dom<GPUDevice>> in GlobalScope to access them from ScriptThread.
  5. While creating a GPUDevice we send the PipelineId of the pipeline it was created in so that when the server send the response, we can access the device from the GlobalScope of that Pipeline.
  6. We generate the relevant GPUError in GlobalScope(if required) can call the necessary fn in GPUDevice.

This implementation is pretty rough and not complete at all. Broadly the following cases aren't handled at the moment-

  1. ErrorFilter is not checked when an error is captured.
  2. We don't propagate an error to enclosing scope if a scope did not capture it.
  3. UncapturedErrorEvents is not implemented.

As for Equivalent BGLs we now store Vec<(Vec<wgt::BindGroupLayoutEntry>, Dom<GPUBindGroupLayout>)> in GPUDevice and compare Vec<wgt::BindGroupLayoutEntry> for new BGL with the existing ones and return one if it is found. Otherwise a new one is created and an entry added to the Vec.

r?@kvark


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • There are tests for these changes

@highfive
Copy link

highfive commented Jul 15, 2020

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/webidls/GPUValidationError.webidl, components/script/dom/gpuadapter.rs, components/script/dom/webidls/GPUOutOfMemoryError.webidl, components/script/dom/gpudevicelostinfo.rs, components/script/dom/bindings/codegen/Bindings.conf and 9 more
  • @KiChjang: components/script/dom/webidls/GPUValidationError.webidl, components/script/dom/gpuadapter.rs, components/script/dom/webidls/GPUOutOfMemoryError.webidl, components/script/dom/gpudevicelostinfo.rs, components/script/dom/bindings/codegen/Bindings.conf and 9 more

@highfive
Copy link

highfive commented Jul 15, 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 Jul 15, 2020
components/script/dom/globalscope.rs Show resolved Hide resolved
components/script/dom/gpudevice.rs Outdated Show resolved Hide resolved
components/script/dom/gpudevice.rs Outdated Show resolved Hide resolved
components/script/dom/gpudevice.rs Outdated Show resolved Hide resolved
components/script/dom/gpudevice.rs Outdated Show resolved Hide resolved
components/webgpu/lib.rs Outdated Show resolved Hide resolved
components/script/dom/gpudevice.rs Outdated Show resolved Hide resolved
components/script/dom/gpudevice.rs Outdated Show resolved Hide resolved
components/webgpu/lib.rs Outdated Show resolved Hide resolved
Copy link
Member

@kvark kvark left a comment

I think I understand the code better now, and it's almost there.
Great work! Just one last round

components/script/dom/gpudevice.rs Outdated Show resolved Hide resolved
components/script/dom/gpudevice.rs Outdated Show resolved Hide resolved
components/script/dom/gpudevice.rs Outdated Show resolved Hide resolved
components/webgpu/lib.rs Show resolved Hide resolved
components/webgpu/lib.rs Outdated Show resolved Hide resolved
kvark
kvark approved these changes Jul 17, 2020
Copy link
Member

@kvark kvark left a comment

beautiful, thank you!

@kunalmohan
Copy link
Collaborator Author

kunalmohan commented Jul 17, 2020

@kvark I have attempted to solve the GPUDevice cleanup issue. I am not completely happy with this, but this should work until we come up with a better solution.

kvark
kvark approved these changes Jul 17, 2020
Copy link
Member

@kvark kvark left a comment

Looks fine for now, thank you!

@kvark
Copy link
Member

kvark commented Jul 17, 2020

@bors-servo r+

@bors-servo
Copy link
Contributor

bors-servo commented Jul 17, 2020

📌 Commit 785497a has been approved by kvark

@bors-servo
Copy link
Contributor

bors-servo commented Jul 17, 2020

Testing commit 785497a with merge 9cd22a0...

@bors-servo
Copy link
Contributor

bors-servo commented Jul 17, 2020

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

@bors-servo bors-servo merged commit 9cd22a0 into servo:master Jul 17, 2020
2 checks passed
WebGPU MVP automation moved this from In progress to Done Jul 17, 2020
@kunalmohan kunalmohan deleted the async-error branch Jul 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
WebGPU MVP
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants