[WebGPU] popErrorScope causes an IPC assertion #9584

mwyrzykowski · 2023-02-03T06:30:40Z

`d5a1264`

[WebGPU] popErrorScope causes an IPC assertion
https://bugs.webkit.org/show_bug.cgi?id=251666
<radar://104993240>

Reviewed by Myles C. Maxfield.

GPUDevice.popErrorScope is supposed to be asynchronous, so use sendWithAsyncReply
instead of sendSync.

* Source/WebKit/GPUProcess/graphics/WebGPU/RemoteDevice.messages.in:
* Source/WebKit/WebProcess/GPU/graphics/WebGPU/RemoteDeviceProxy.cpp:
(WebKit::WebGPU::RemoteDeviceProxy::popErrorScope):
* Source/WebKit/WebProcess/GPU/graphics/WebGPU/RemoteDeviceProxy.h:

Canonical link: https://commits.webkit.org/260413@main

8802095

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	✅ 🧪 api-mac	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk1	~~🧪 api-gtk~~
	✅ 🛠 tv	✅ 🧪 mac-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
❌ 🛠 🧪 merge	✅ 🛠 watch-sim

webkit-early-warning-system · 2023-02-03T06:30:58Z

EWS run on previous version of this PR (hash a2fc827)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk1	~~🧪 api-gtk~~
	✅ 🛠 tv	✅ 🧪 mac-wk2
	✅ 🛠 tv-sim	❌ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
	✅ 🛠 watch-sim

litherum · 2023-02-03T07:19:28Z

Source/WebGPU/WebGPU/Device.mm

@@ -287,20 +287,16 @@ static void captureFrame(id<MTLDevice> captureObject)
    // https://gpuweb.github.io/gpuweb/#dom-gpudevice-poperrorscope

    if (!validatePopErrorScope()) {
-        instance().scheduleWork([callback = WTFMove(callback)]() mutable {


Ditto for the other patch. This function philosophically should be async; we should be making it properly async rather than temporarily making it sync.

OK for temporary code, but I think we can do better.

Why should it be async and doesn't scheduleWork() run on the same dispatch queue which the containing function runs? If so, what is the benefit to making it async?

Because that's the API contract of promises, you are guaranteed your promise's .then callback won't run in the middle of your popErrorScope() call. I'm trying to match the API behavior of the WebGPU IDL as much as possible.

That's correct but we shouldn't impact the web api for due to how the native API is structured. Instead I think we should ensure the contract on the native API in a way that does not impact the web API.

I'm confused. An app that wants to use WebGPU.framework is going to expect that it behaves the same way as the web API does, right?

I think so however native WebGPU still seems under development.

My point is that making the callback asynchronous here negatively impacts users of the web api as now they incur additional latency for the task to return, when it was already asynchronous from the website’s perspective.

If we want the native API to appear the same, we should implement that in a way that doesn’t add latency to the web api. This patch doesn’t address that as the web api is priority.

Is the difference measurable?

If the answer is "yes", I propose we add a boolean to WGPUInstanceCocoaDescriptor that represents whether violating the asynchronicity guarantees of the spec is more desirable than upholding them, and set the value of the boolean to true in WebKit and false everywhere else, and then implement both codepaths, and use the boolean to select between them.

If the answer is "no" then we should just use the async codepath everywhere.

Probably not measurable especially given the discussion is about popErrorScope

I will try the new IPC sendToStreamWithAsyncReply to see if we can resolve this issue that way. I'm kind of opposed to using NotStreamEncodableReply as that is a workaround

@litherum PR updated with sendWithAsyncReply, I reverted the changes to Device.mm

webkit-early-warning-system · 2023-02-04T00:18:19Z

EWS run on previous version of this PR (hash d32cbd7)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	~~🛠 wpe~~	⏳ 🛠 🧪 win
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk	⏳ 🛠 wincairo
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	~~🧪 api-mac~~	~~🧪 gtk-wk2~~
	~~🧪 api-ios~~	~~🧪 mac-wk1~~	~~🧪 api-gtk~~
⏳ 🛠 🧪 jsc	✅ 🛠 tv	~~🧪 mac-wk2~~
	✅ 🛠 tv-sim	~~🧪 mac-AS-debug-wk2~~
	✅ 🛠 watch	~~🧪 mac-wk2-stress~~
	⏳ 🛠 watch-sim

webkit-early-warning-system · 2023-02-04T00:24:19Z

EWS run on previous version of this PR (hash 2b62217)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk
✅ 🧪 webkitperl	⏳ 🧪 ios-wk2	✅ 🧪 api-mac	⏳ 🧪 gtk-wk2
	⏳ 🧪 api-ios	✅ 🧪 mac-wk1	⏳ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
	✅ 🛠 watch-sim

webkit-early-warning-system · 2023-02-16T21:13:56Z

EWS run on current version of this PR (hash 8802095)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	✅ 🧪 api-mac	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk1	~~🧪 api-gtk~~
	✅ 🛠 tv	✅ 🧪 mac-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
❌ 🛠 🧪 merge	✅ 🛠 watch-sim

https://bugs.webkit.org/show_bug.cgi?id=251666 <radar://104993240> Reviewed by Myles C. Maxfield. GPUDevice.popErrorScope is supposed to be asynchronous, so use sendWithAsyncReply instead of sendSync. * Source/WebKit/GPUProcess/graphics/WebGPU/RemoteDevice.messages.in: * Source/WebKit/WebProcess/GPU/graphics/WebGPU/RemoteDeviceProxy.cpp: (WebKit::WebGPU::RemoteDeviceProxy::popErrorScope): * Source/WebKit/WebProcess/GPU/graphics/WebGPU/RemoteDeviceProxy.h: Canonical link: https://commits.webkit.org/260413@main

webkit-commit-queue · 2023-02-17T02:41:40Z

Committed 260413@main (d5a1264): https://commits.webkit.org/260413@main

Reviewed commits have been landed. Closing PR #9584 and removing active labels.

mwyrzykowski self-assigned this Feb 3, 2023

mwyrzykowski added the WebGPU For bugs in WebGPU label Feb 3, 2023

mwyrzykowski requested review from litherum, kkinnunen-apple, djg and tadeuzagallo February 3, 2023 06:31

litherum approved these changes Feb 3, 2023

View reviewed changes

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Feb 3, 2023

mwyrzykowski removed the merging-blocked Applied to prevent a change from being merged label Feb 4, 2023

mwyrzykowski force-pushed the eng/WebGPU-popErrorScope-causes-an-IPC-assertion branch from a2fc827 to d32cbd7 Compare February 4, 2023 00:17

mwyrzykowski force-pushed the eng/WebGPU-popErrorScope-causes-an-IPC-assertion branch from d32cbd7 to 2b62217 Compare February 4, 2023 00:23

litherum mentioned this pull request Feb 4, 2023

[WebGPU] forceFallbackAdapter CTS test fails (251629) #9583

Merged

mwyrzykowski force-pushed the eng/WebGPU-popErrorScope-causes-an-IPC-assertion branch from 2b62217 to 8802095 Compare February 16, 2023 20:58

mwyrzykowski requested a review from cdumez as a code owner February 16, 2023 20:58

mwyrzykowski added the merge-queue Applied to send a pull request to merge-queue label Feb 17, 2023

webkit-early-warning-system force-pushed the eng/WebGPU-popErrorScope-causes-an-IPC-assertion branch from 8802095 to a60f8cc Compare February 17, 2023 02:38

webkit-early-warning-system force-pushed the eng/WebGPU-popErrorScope-causes-an-IPC-assertion branch from a60f8cc to d5a1264 Compare February 17, 2023 02:41

webkit-early-warning-system merged commit d5a1264 into WebKit:main Feb 17, 2023

webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Feb 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WebGPU] popErrorScope causes an IPC assertion #9584

[WebGPU] popErrorScope causes an IPC assertion #9584

mwyrzykowski commented Feb 3, 2023 •

edited by webkit-early-warning-system

webkit-early-warning-system commented Feb 3, 2023 •

edited

litherum Feb 3, 2023

mwyrzykowski Feb 3, 2023

litherum Feb 4, 2023 •

edited

mwyrzykowski Feb 4, 2023

litherum Feb 4, 2023

mwyrzykowski Feb 4, 2023

litherum Feb 4, 2023

litherum Feb 4, 2023

mwyrzykowski Feb 4, 2023

mwyrzykowski Feb 16, 2023

webkit-early-warning-system commented Feb 4, 2023 •

edited

webkit-early-warning-system commented Feb 4, 2023 •

edited

webkit-early-warning-system commented Feb 16, 2023 •

edited

webkit-commit-queue commented Feb 17, 2023

[WebGPU] popErrorScope causes an IPC assertion #9584

[WebGPU] popErrorScope causes an IPC assertion #9584

Conversation

mwyrzykowski commented Feb 3, 2023 • edited by webkit-early-warning-system

webkit-early-warning-system commented Feb 3, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

litherum Feb 4, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webkit-early-warning-system commented Feb 4, 2023 • edited

webkit-early-warning-system commented Feb 4, 2023 • edited

webkit-early-warning-system commented Feb 16, 2023 • edited

webkit-commit-queue commented Feb 17, 2023

mwyrzykowski commented Feb 3, 2023 •

edited by webkit-early-warning-system

webkit-early-warning-system commented Feb 3, 2023 •

edited

litherum Feb 4, 2023 •

edited

webkit-early-warning-system commented Feb 4, 2023 •

edited

webkit-early-warning-system commented Feb 4, 2023 •

edited

webkit-early-warning-system commented Feb 16, 2023 •

edited