[WebGPU] Repeated calls to wgpuDeviceGetQueue() are supposed to return the same pointer
Created attachment 454644 [details] Patch
Comment on attachment 454644 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=454644&action=review > Source/WebGPU/WebGPU/Adapter.mm:123 > // See the comment in Device::setLabel() about why we're not setting the label here. I think you can remove this comment. For all intents and purposes, passing label to the device constructor counts as initialising, e.g setting, the label for the device. > Source/WebGPU/WebGPU/Device.mm:56 > + if (deviceLabel && strlen(deviceLabel)) nit: we calculate the length of the label for the purpose of "isempty"
Comment on attachment 454644 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=454644&action=review > Source/WebGPU/WebGPU/Adapter.mm:171 > + callback(status, new WGPUDeviceImpl { device.releaseNonNull(), { queue } }, message); btw: Is there a reason for this pattern where we create new wrapper objects every time we communicate with another layer in the sw stack? I think it's employed here (invoking the wgpu client callback) as well as internally ?
Comment on attachment 454644 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=454644&action=review >> Source/WebGPU/WebGPU/Adapter.mm:123 >> // See the comment in Device::setLabel() about why we're not setting the label here. > > I think you can remove this comment. > For all intents and purposes, passing label to the device constructor counts as initialising, e.g setting, the label for the device. Good point. >> Source/WebGPU/WebGPU/Adapter.mm:171 >> + callback(status, new WGPUDeviceImpl { device.releaseNonNull(), { queue } }, message); > > btw: Is there a reason for this pattern where we create new wrapper objects every time we communicate with another layer in the sw stack? > > I think it's employed here (invoking the wgpu client callback) as well as internally ? There's a (bad) reason. Ideas for improvements are welcome. The reason is that the shared WebGPU.h header has: typedef struct WGPUDeviceImpl* WGPUDevice; However, it would be convenient if we could have our own names like this: namespace WebGPU { class Device { ... id<MTLDevice> m_device; ... } } I thought of 3 possible solutions: 1. Use reinterpret_cast<>() to freely convert between WGPUDeviceImpl* and WebGPU::Device*. This breaks C++'s type system, and is technically UB (I think) so I was hoping to avoid this option. 2. Don't have nice names, and don't use "namespace WebGPU": struct WGPUDeviceImpl { ... id<MTLDevice> m_device; ... } This is unfortunate because it foregoes C++'s built-in namespace support in favor of prefixes, and isn't the pattern we use for the rest of WebKit 3. Do a double-pointer-indirection: struct WGPUDeviceImpl { Ref<WebGPU::Device> device; }; This isn't great for performance (though it's probably negligible) and has a somewhat bad smell about having two types which mean the same thing. For the initial implementation, I chose option 3, under the assumption that it would be easy/mechanical to change later if a better option presents itself.
Committed r291315 (248454@trunk): <https://commits.webkit.org/248454@trunk>
Comment on attachment 454644 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=454644&action=review >>> Source/WebGPU/WebGPU/Adapter.mm:171 >>> + callback(status, new WGPUDeviceImpl { device.releaseNonNull(), { queue } }, message); >> >> btw: Is there a reason for this pattern where we create new wrapper objects every time we communicate with another layer in the sw stack? >> >> I think it's employed here (invoking the wgpu client callback) as well as internally ? > > There's a (bad) reason. Ideas for improvements are welcome. > > The reason is that the shared WebGPU.h header has: > > typedef struct WGPUDeviceImpl* WGPUDevice; > > However, it would be convenient if we could have our own names like this: > > namespace WebGPU { > class Device { > ... > id<MTLDevice> m_device; > ... > } > } > > I thought of 3 possible solutions: > > 1. Use reinterpret_cast<>() to freely convert between WGPUDeviceImpl* and WebGPU::Device*. This breaks C++'s type system, and is technically UB (I think) so I was hoping to avoid this option. > > 2. Don't have nice names, and don't use "namespace WebGPU": > > struct WGPUDeviceImpl { > ... > id<MTLDevice> m_device; > ... > } > > This is unfortunate because it foregoes C++'s built-in namespace support in favor of prefixes, and isn't the pattern we use for the rest of WebKit > > 3. Do a double-pointer-indirection: > > struct WGPUDeviceImpl { > Ref<WebGPU::Device> device; > }; > > This isn't great for performance (though it's probably negligible) and has a somewhat bad smell about having two types which mean the same thing. > > For the initial implementation, I chose option 3, under the assumption that it would be easy/mechanical to change later if a better option presents itself. I filed https://bugs.webkit.org/show_bug.cgi?id=237921 about it.
<rdar://problem/90330891>
Comment on attachment 454644 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=454644&action=review >>>> Source/WebGPU/WebGPU/Adapter.mm:171 >>>> + callback(status, new WGPUDeviceImpl { device.releaseNonNull(), { queue } }, message); >>> >>> btw: Is there a reason for this pattern where we create new wrapper objects every time we communicate with another layer in the sw stack? >>> >>> I think it's employed here (invoking the wgpu client callback) as well as internally ? >> >> There's a (bad) reason. Ideas for improvements are welcome. >> >> The reason is that the shared WebGPU.h header has: >> >> typedef struct WGPUDeviceImpl* WGPUDevice; >> >> However, it would be convenient if we could have our own names like this: >> >> namespace WebGPU { >> class Device { >> ... >> id<MTLDevice> m_device; >> ... >> } >> } >> >> I thought of 3 possible solutions: >> >> 1. Use reinterpret_cast<>() to freely convert between WGPUDeviceImpl* and WebGPU::Device*. This breaks C++'s type system, and is technically UB (I think) so I was hoping to avoid this option. >> >> 2. Don't have nice names, and don't use "namespace WebGPU": >> >> struct WGPUDeviceImpl { >> ... >> id<MTLDevice> m_device; >> ... >> } >> >> This is unfortunate because it foregoes C++'s built-in namespace support in favor of prefixes, and isn't the pattern we use for the rest of WebKit >> >> 3. Do a double-pointer-indirection: >> >> struct WGPUDeviceImpl { >> Ref<WebGPU::Device> device; >> }; >> >> This isn't great for performance (though it's probably negligible) and has a somewhat bad smell about having two types which mean the same thing. >> >> For the initial implementation, I chose option 3, under the assumption that it would be easy/mechanical to change later if a better option presents itself. > > I filed https://bugs.webkit.org/show_bug.cgi?id=237921 about it. Can you forward decl the namespaced type? eg: namespace WebGPU { class Device; } typedef WebGPU::Device* WGPUDevice;