Skip to content

Remove GPUFence#1217

Merged
kvark merged 3 commits intogpuweb:mainfrom
kvark:fence
Nov 23, 2020
Merged

Remove GPUFence#1217
kvark merged 3 commits intogpuweb:mainfrom
kvark:fence

Conversation

@kvark
Copy link
Copy Markdown
Contributor

@kvark kvark commented Nov 10, 2020

This is a follow-up to #1169 which just removes the fences from the API, as requested - being a per-requisite for multi-queue landing.

Note: strictly speaking, GPUFence was introduced without much argument very early in the API. The main goal was - to match the native APIs (which have slightly different semantics there, btw), and to anticipate helping with future changes in WebGPU, such as buffer mapping and multi-queue synchronization. Well, now we have a clear idea of the API changes, since we are in the future, and it turns out we don't really need fences. So if we want them to stay, we need to argue for them being (re-) introduced. As it stands now, a single Queue.onCompletion() replaces fences just fine, and this is what this PR does.


Preview | Diff

@kvark kvark requested review from Kangz and kdashg November 10, 2020 17:22
@github-actions
Copy link
Copy Markdown
Contributor

Previews, as seen at the time of posting this comment:
WebGPU | IDL
WGSL
394fbe6

Copy link
Copy Markdown
Contributor

@Kangz Kangz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM in a single-queue world but onCompletion will most likely get an optional GPUSignalValue when multiqueue is added.

Co-authored-by: Corentin Wallez <corentin@wallez.net>
spec/index.bs Outdated

GPUFence createFence(optional GPUFenceDescriptor descriptor = {});
undefined signal(GPUFence fence, GPUFenceValue signalValue);
Promise<undefined> onCompletion();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jdashg suggested maybe renaming this to something like onSignalComplete since this is no longer on a fence object where the context is clear.

I said:

Alternatively maybe we should try to make this not sound like an event handler (with the word "on" at the front).

E.g. workCompleted or something

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onWorkCompleted? I don't know if there's a naming convention for promises in WebIDL.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onPendingWorkCompleted?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned on makes it sound like an event handler. One example (more similar to our device.lost) is:
https://www.w3.org/TR/service-workers/#ref-for-dom-serviceworkercontainer-ready
but other than that most promise-returning methods are verbs, since, you know, they're methods.

@kainino0x
Copy link
Copy Markdown
Contributor

Editors to bikeshed the onCompletion name

@kvark kvark merged commit 8728398 into gpuweb:main Nov 23, 2020
@kvark kvark deleted the fence branch November 23, 2020 22:49
@kainino0x kainino0x added the multi-queue Part of the multi-queue feature label Dec 16, 2020
ben-clayton pushed a commit to ben-clayton/gpuweb that referenced this pull request Sep 6, 2022
This PR adds unimplemented stubs for the `distance` builtin.

Issue: gpuweb#1217
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

multi-queue Part of the multi-queue feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants