Bug 237861 – [WebGPU] Repeated calls to wgpuDeviceGetQueue() are supposed to return the same pointer

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

[WebGPU] Repeated calls to wgpuDeviceGetQueue() are supposed to return the same pointer

[Myles C. Maxfield]

Created attachment 454644 [details]
Patch

[Kimmo Kinnunen]

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"

[Kimmo Kinnunen]

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 ?

[Myles C. Maxfield]

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.

[Myles C. Maxfield]

Committed r291315 (248454@trunk): <https://commits.webkit.org/248454@trunk>

[Myles C. Maxfield]

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.

[Radar WebKit Bug Importer]

<rdar://problem/90330891>

[Dan Glastonbury]

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;

[Dan Glastonbury]

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;