Bug 237755 – [WebGPU] Allow for scheduling asynchronous work

• Component: New Bugs
• Status: RESOLVED FIXED

[Myles C. Maxfield]

[WebGPU] Allow for scheduling asynchronous work

[Myles C. Maxfield]

Created attachment 454444 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 454444 [details]
Patch

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

> Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUImpl.cpp:52
> +            scheduleWorkFunction(WTFMove(workItem));

This is a UAF. scheduleWorkFunction refers to the reference, not the function itself.

[Myles C. Maxfield]

Created attachment 454566 [details]
Patch

[Kimmo Kinnunen]

Comment on attachment 454566 [details]
Patch

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

Sorry for not understanding the system.. 

Initially I don't understand the distinction:
WGPUWorkItem  -- I see this as a task coming from wgpu and webkit executing it.

WorkItem -- I see this as a task coming from WebKit going to wgpu, which initially doesn't make sense. Why would we ask wgpu to invoke this item, why wouldn't we just invoke it? Is it then coming back as WGPUWorkItem for ordering reasons?

> Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUImpl.cpp:50
> +    ScheduleWorkFunction localScheduleWorkFunction = WTFMove(scheduleWorkFunction);

scheduleWorkFunction is a block-equivalent..

> Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUImpl.cpp:57
> +        ^(WGPUWorkItem workItem)

this is a block..

> Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUImpl.cpp:59
> +            localScheduleWorkFunction(makeBlockPtr(workItem));

makeBlockPtr makes a block..

workItem is a block..

Maybe there's now some nested blocks that don't need to be blocks?

> Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUImpl.h:45
> +    // and anything captured by a block needs to be copyable.

I think this is now somehow mixed, or I'm not undertstanding everything.
We capture WTF::Functions as blocks via makeBlockPtr in WorkQueueCocoa.mm.

> Source/WebGPU/WebGPU/Instance.h:53
> +    using WorkItem = std::function<void(void)>;

This appears to be somehow redundant?

If the work is sent by WGPU as WGPUWorkItem, then WebKit should just be able to apply it via WGPUWorkItem and no need to wrap it redundantly in std::function?

[Myles C. Maxfield]

Comment on attachment 454566 [details]
Patch

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

>> Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUImpl.cpp:59
>> +            localScheduleWorkFunction(makeBlockPtr(workItem));
> 
> makeBlockPtr makes a block..
> 
> workItem is a block..
> 
> Maybe there's now some nested blocks that don't need to be blocks?

Yeah, let me see if I can get rid of some.

>> Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUImpl.h:45
>> +    // and anything captured by a block needs to be copyable.
> 
> I think this is now somehow mixed, or I'm not undertstanding everything.
> We capture WTF::Functions as blocks via makeBlockPtr in WorkQueueCocoa.mm.

Good point! Let me investigate more.

>> Source/WebGPU/WebGPU/Instance.h:53
>> +    using WorkItem = std::function<void(void)>;
> 
> This appears to be somehow redundant?
> 
> If the work is sent by WGPU as WGPUWorkItem, then WebKit should just be able to apply it via WGPUWorkItem and no need to wrap it redundantly in std::function?

These exists for layering. The WGPU types shouldn't exist outside WebCore/PAL/pal/graphics/WebGPU/Impl.

[Myles C. Maxfield]

Created attachment 454624 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 454566 [details]
Patch

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

>>> Source/WebGPU/WebGPU/Instance.h:53
>>> +    using WorkItem = std::function<void(void)>;
>> 
>> This appears to be somehow redundant?
>> 
>> If the work is sent by WGPU as WGPUWorkItem, then WebKit should just be able to apply it via WGPUWorkItem and no need to wrap it redundantly in std::function?
> 
> These exists for layering. The WGPU types shouldn't exist outside WebCore/PAL/pal/graphics/WebGPU/Impl.

Oh, whoops, I was confused when I made this comment ☝️. The reason is because I prefer using C++ lambdas wherever possible because they are more expressive than C blocks (e.g. lambdas can be moved from, etc.). So, internally to the framework, I'm using lambdas, and they get converted to block pointers at framework boundaries.

[Kimmo Kinnunen]

Comment on attachment 454624 [details]
Patch

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

> Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUImpl.cpp:48
> +        scheduleWorkFunction(makeBlockPtr(workItem));

WGPUWorkItem is a block, so it shouldn't need to be wrapped in a block?
Also, the call could move the `workItem`..

> Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUImpl.h:44
> +    using ScheduleWorkFunction = Function<void(WorkItem&&)>;

> The reason is because I prefer using C++ lambdas wherever possible because they are more expressive than C blocks (e.g. lambdas can be moved from, etc.). So, internally to the framework, I'm using lambdas, and they get converted to block pointers at framework boundaries.

The ScheduleWorkFunction will not move anything from WorkItem (which wrapsWGPUWorkItem) . It won't move anything out of WGPUWorkItem either, as that is just a block pointer (opaque function with context.) WorkItem cannot be used in any other way than called.

So you are redundantly wrapping the WGPUWorkItem call inside a redundant call.

I think it's ok if your requirements are:
1) Not expose that blocks exist in this layer of the SW stack (sw layer == WebKit WebGPU client)
2) Not expose WGPU* types  in this layer of the SW stack

However, since both of these are given forever in WebKit, having the extra indirection forever sounds silly.
I'd imagine WGPU* types are like uint32_t types: they're the fundamental types this particular implementation is being built on.

[Kimmo Kinnunen]

Comment on attachment 454624 [details]
Patch

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

> Source/WebGPU/WebGPU/Instance.h:54
> +    void scheduleWork(WorkItem&&);

In fact, if you keep this as a wrapped thingy, then your "WorkItem" could be a CompletionHandler.
E.g. 
Function == "I give this to you and you may call this once or more times"
CompletionHandler = "I give this to you and you may call this once"

This will come up in later patches, where you hold list of items.

> Source/WebGPU/WebGPU/Instance.mm:89
> +            workItem();

Example iteration related to the CompletionHandler

if you have 700 items in the list, it may be desired for the very nit-picker client that the closures held by each of the functions are discarded as soon as they're not needed, e.g. immediately after called.

CompletionHandler expresses this.

[Myles C. Maxfield]

Comment on attachment 454624 [details]
Patch

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

>> Source/WebGPU/WebGPU/Instance.h:54
>> +    void scheduleWork(WorkItem&&);
> 
> In fact, if you keep this as a wrapped thingy, then your "WorkItem" could be a CompletionHandler.
> E.g. 
> Function == "I give this to you and you may call this once or more times"
> CompletionHandler = "I give this to you and you may call this once"
> 
> This will come up in later patches, where you hold list of items.

Yes, CompletionHandler is a great idea. Asynchronous functions will take a CompletionHandler, whereas functions like Device::setUncapturedErrorCallback() will take a Function.

[Myles C. Maxfield]

Comment on attachment 454624 [details]
Patch

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

>>> Source/WebGPU/WebGPU/Instance.h:54
>>> +    void scheduleWork(WorkItem&&);
>> 
>> In fact, if you keep this as a wrapped thingy, then your "WorkItem" could be a CompletionHandler.
>> E.g. 
>> Function == "I give this to you and you may call this once or more times"
>> CompletionHandler = "I give this to you and you may call this once"
>> 
>> This will come up in later patches, where you hold list of items.
> 
> Yes, CompletionHandler is a great idea. Asynchronous functions will take a CompletionHandler, whereas functions like Device::setUncapturedErrorCallback() will take a Function.

I started migrating to CompletionHandler, but it's ending up being bigger than the original patch, so I'll separate this migration to a follow-up patch.

[Myles C. Maxfield]

(In reply to Kimmo Kinnunen from comment #8)
> The ScheduleWorkFunction will not move anything from WorkItem (which
> wrapsWGPUWorkItem) . It won't move anything out of WGPUWorkItem either, as
> that is just a block pointer (opaque function with context.) WorkItem cannot
> be used in any other way than called.
> 
> So you are redundantly wrapping the WGPUWorkItem call inside a redundant
> call.
> 
> I think it's ok if your requirements are:
> 1) Not expose that blocks exist in this layer of the SW stack (sw layer ==
> WebKit WebGPU client)
> 2) Not expose WGPU* types  in this layer of the SW stack
> 
> However, since both of these are given forever in WebKit, having the extra
> indirection forever sounds silly.
> I'd imagine WGPU* types are like uint32_t types: they're the fundamental
> types this particular implementation is being built on.

Yeah, I think this is kind of important for layering - I think both your 1) and 2) are true. I think I disagree about the comparison with uint32_t - that is clearly shared among many libraries and layers, whereas WGPU is just an implementation detail.

And, since the source comes from a WTF::BlockPtr, copying to a second WTF::BlockPtr is free - it doesn't copy the context.

[Myles C. Maxfield]

Comment on attachment 454624 [details]
Patch

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

>> Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUImpl.cpp:48
>> +        scheduleWorkFunction(makeBlockPtr(workItem));
> 
> WGPUWorkItem is a block, so it shouldn't need to be wrapped in a block?
> Also, the call could move the `workItem`..

Can you describe what you mean by "it shouldn't need to be wrapped in a block?"

[Myles C. Maxfield]

I'm going to land this making the adjustments that I can, make a follow-up patch about CompletionHandler, and then make another follow-up patch according to Kimmo's yet-to-be-made comments when he makes them.

[Myles C. Maxfield]

Committed r291313 (248452@trunk): <https://commits.webkit.org/248452@trunk>

[Radar WebKit Bug Importer]

<rdar://problem/90330396>

[Myles C. Maxfield]

Leaving bug open because there's more discussion to be had.

[Myles C. Maxfield]

Comment on attachment 454624 [details]
Patch

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

>>>> Source/WebGPU/WebGPU/Instance.h:54
>>>> +    void scheduleWork(WorkItem&&);
>>> 
>>> In fact, if you keep this as a wrapped thingy, then your "WorkItem" could be a CompletionHandler.
>>> E.g. 
>>> Function == "I give this to you and you may call this once or more times"
>>> CompletionHandler = "I give this to you and you may call this once"
>>> 
>>> This will come up in later patches, where you hold list of items.
>> 
>> Yes, CompletionHandler is a great idea. Asynchronous functions will take a CompletionHandler, whereas functions like Device::setUncapturedErrorCallback() will take a Function.
> 
> I started migrating to CompletionHandler, but it's ending up being bigger than the original patch, so I'll separate this migration to a follow-up patch.

https://bugs.webkit.org/show_bug.cgi?id=237925 [WebGPU] Migrate from WTF::Function to WTF::CompletionHandler

[Kimmo Kinnunen]

workItem needs makeBlockPtr