Bug 238719 - [WebGPU] Add support for the "is valid to use with" operation to WebGPU objects
Summary: [WebGPU] Add support for the "is valid to use with" operation to WebGPU objects
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGPU (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on: 238711
Blocks: 238720
  Show dependency treegraph
 
Reported: 2022-04-03 23:38 PDT by Myles C. Maxfield
Modified: 2022-04-07 18:58 PDT (History)
4 users (show)

See Also:


Attachments
Patch (37.66 KB, patch)
2022-04-03 23:44 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (56.92 KB, patch)
2022-04-04 01:05 PDT, Myles C. Maxfield
kkinnunen: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2022-04-03 23:38:42 PDT
.
Comment 1 Myles C. Maxfield 2022-04-03 23:44:20 PDT
Created attachment 456535 [details]
Patch
Comment 2 Myles C. Maxfield 2022-04-04 01:05:24 PDT
Created attachment 456546 [details]
Patch
Comment 3 Kimmo Kinnunen 2022-04-07 02:57:39 PDT
Comment on attachment 456546 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=456546&action=review

also changes refcounts to thread safe (which is probably good)

> Source/WebGPU/WebGPU/ObjectBase.h:40
> +    ~ObjectBase();

Surprisingly ThreadSafeRefCounted has a footgun  where the destructor is not a virtual.
So written this way, Ref<ObjectBase> would never run the subclass destructors, which is a hard to find down the line if you ever end up using that.
It's better to mark this virtual.

If you don't intend to support Ref<ObjectBase>, then it's better to leave the threadsaferefcounted in the held classes.
Comment 4 Myles C. Maxfield 2022-04-07 12:18:31 PDT
Comment on attachment 456546 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=456546&action=review

>> Source/WebGPU/WebGPU/ObjectBase.h:40
>> +    ~ObjectBase();
> 
> Surprisingly ThreadSafeRefCounted has a footgun  where the destructor is not a virtual.
> So written this way, Ref<ObjectBase> would never run the subclass destructors, which is a hard to find down the line if you ever end up using that.
> It's better to mark this virtual.
> 
> If you don't intend to support Ref<ObjectBase>, then it's better to leave the threadsaferefcounted in the held classes.

Thinking about this more, I think there may actually be a better way of doing this. The whole point of this ObjectBase class is just to hold the isValidToUseWith() function, which is already templated. So, if it's already templated, it doesn't really need to actually exist in the base class. It can just be a free function and call .device() on the templated type. Doing it that way will get rid of this whole type hierarchy, and the problem you're describing here, as well as reducing total amount of code.
Comment 5 Myles C. Maxfield 2022-04-07 13:23:16 PDT
Committed r292558 (249396@trunk): <https://commits.webkit.org/249396@trunk>
Comment 6 Radar WebKit Bug Importer 2022-04-07 13:24:24 PDT
<rdar://problem/91440590>
Comment 7 Myles C. Maxfield 2022-04-07 15:50:28 PDT
Committed r292574 (?): <https://commits.webkit.org/r292574>
Comment 8 Dan Glastonbury 2022-04-07 18:58:30 PDT
(In reply to Myles C. Maxfield from comment #4)
> [...] It
> can just be a free function and call .device() on the templated type. Doing
> it that way will get rid of this whole type hierarchy, and the problem
> you're describing here, as well as reducing total amount of code.

I like the sound of that!