| Summary: | [WebGPU] Create a path of Ref<>s between Instance and Queue | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Myles C. Maxfield <mmaxfield> | ||||||||
| Component: | WebGPU | Assignee: | Myles C. Maxfield <mmaxfield> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | dino, djg, kkinnunen, webkit-bug-importer | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | WebKit Nightly Build | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Bug Depends on: | 237861 | ||||||||||
| Bug Blocks: | 237583, 237869 | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Myles C. Maxfield
2022-03-14 18:52:44 PDT
Created attachment 454646 [details]
Patch
Created attachment 454647 [details]
Patch
Comment on attachment 454647 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=454647&action=review > Source/WebGPU/WebGPU/Device.h:89 > + Is there a reason this is duplicated across the whole hierarchy? In the end there's only one item that can schedule work: the Instance. Why are we holding device that holds adapter that holds instance instead of holding instance directly? Comment on attachment 454647 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=454647&action=review >> Source/WebGPU/WebGPU/Device.h:89 >> + > > Is there a reason this is duplicated across the whole hierarchy? > In the end there's only one item that can schedule work: the Instance. > > Why are we holding device that holds adapter that holds instance instead of holding instance directly? Yeah, I think you're right that this can be more straightforward. Objects are going to need the device pointer eventually, though, to implement the "valid to use with" check: https://gpuweb.github.io/gpuweb/#abstract-opdef-valid-to-use-with So I think the chain can be Objects -> Device -> Instance, rather than Objects -> Other objects -> Device -> Adapter -> Instance. Created attachment 454758 [details]
Patch
Comment on attachment 454758 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=454758&action=review > Source/WebGPU/WebGPU/Queue.h:61 > + void scheduleWork(Instance::WorkItem&&); Still, I think it's a bit strange to have the schedulework per class. I'd imagine it'd be: void Queue::someFunction() { instance()->scheduleWork() } Queue::someFunction Instance::scheduleWork instead of: Queue::someFunction Queue::scheduleWork Device::scheduleWork Instance::scheduleWork Comment on attachment 454758 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=454758&action=review >> Source/WebGPU/WebGPU/Queue.h:61 >> + void scheduleWork(Instance::WorkItem&&); > > Still, I think it's a bit strange to have the schedulework per class. > > > I'd imagine it'd be: > void Queue::someFunction() > { > instance()->scheduleWork() > } > > Queue::someFunction > Instance::scheduleWork > > instead of: > Queue::someFunction > Queue::scheduleWork > Device::scheduleWork > Instance::scheduleWork Right, makes sense. The Queue::scheduleWork() is valuable, I think, so I think the best option is in the middle: Queue::someFunction Queue::scheduleWork Instance::scheduleWork Committed r291365 (248497@trunk): <https://commits.webkit.org/248497@trunk> |