Skip to content

Add a way of setting the initial label for the default queue#2645

Merged
litherum merged 1 commit intogpuweb:mainfrom
litherum:defaultqueuelabel
Mar 15, 2022
Merged

Add a way of setting the initial label for the default queue#2645
litherum merged 1 commit intogpuweb:mainfrom
litherum:defaultqueuelabel

Conversation

@litherum
Copy link
Copy Markdown
Contributor

@litherum litherum commented Mar 7, 2022 •
edited by pr-preview bot
Loading

@litherum litherum requested review from kainino0x and toji March 7, 2022 05:07
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 7, 2022

Previews, as seen when this build job started (1c6ac3d):
WebGPU | IDL
WGSL
Explainer

@kainino0x kainino0x added the tacit resolution candidate Editors may be able to resolve and move to tacit resolution queue label Mar 9, 2022
@kainino0x
Copy link
Copy Markdown
Contributor

kainino0x commented Mar 9, 2022

This strikes me as a little awkward, but otherwise fine. I suppose we could instead specify something like, the queue's initial label is always "queue for " + device.name?

@toji
Copy link
Copy Markdown
Member

toji commented Mar 9, 2022

I agree with Kai. The functionality isn't bad, but it's an usual place to put it. Obviously there's not an alternative, however, since the queue is created with the device.

A couple of thoughts:

  • It's not clear to me what situations having a label at initialization would be necessary for? It's certainly nice to be able to set them at creation time, but I don't see it as problematic to set this label after device creation.
  • Is there a chance that we'll need to have other queue creation parameters in the future? Especially if we begin allowing creation of multiple queues. If so, then maybe we should generalize this a bit further preemptively and accept a full GPUQueueDescriptor when requesting the device. Something like:
adapter.requestDevice({
  requiredFeatures: [],
  defaultQueue: {
    label: 'myQueue',
    theoreticalFoo: 'bar'
  },
});

@kainino0x
Copy link
Copy Markdown
Contributor

kainino0x commented Mar 9, 2022

Good thoughts, I agree with both. I'm OK with either leaving this out entirely or going ahead and adding a queue descriptor inside the device descriptor.

@litherum litherum force-pushed the defaultqueuelabel branch from 1c6ac3d to 3bfeb72 Compare March 15, 2022 02:58
@litherum
Copy link
Copy Markdown
Contributor Author

litherum commented Mar 15, 2022

I've updated the PR to add a GPUQueueDescriptor which inherits from GPUObjectBase and has no members.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 15, 2022

Previews, as seen when this build job started (3bfeb72):
WebGPU | IDL
WGSL
Explainer

toji
toji approved these changes Mar 15, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@toji toji left a comment

Choose a reason for hiding this comment

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

Perfect! Kai and I talked about this yesterday and agree this is thr right path forward, glad to see you had it ready to go!

LGTM with one very minor change request.

spec/index.bs Outdated
dictionary GPUDeviceDescriptor : GPUObjectDescriptorBase {
sequence<GPUFeatureName> requiredFeatures = [];
record<DOMString, GPUSize64> requiredLimits = {};
GPUQueueDescriptor defaultQueueDescriptor = {};
Copy link
Copy Markdown
Member

@toji toji Mar 15, 2022

Choose a reason for hiding this comment

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

Elsewhere in the spec even when we have nested descriptors we don't refer to them as such, so I have a mild preference for leaving the name here as simply defaultQueue.

Copy link
Copy Markdown
Contributor

@kainino0x kainino0x Mar 15, 2022

Choose a reason for hiding this comment

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

No opinion :) Rest LGTM

kainino0x
kainino0x approved these changes Mar 15, 2022
View reviewed changes
spec/index.bs Outdated
dictionary GPUDeviceDescriptor : GPUObjectDescriptorBase {
sequence<GPUFeatureName> requiredFeatures = [];
record<DOMString, GPUSize64> requiredLimits = {};
GPUQueueDescriptor defaultQueueDescriptor = {};
Copy link
Copy Markdown
Contributor

@kainino0x kainino0x Mar 15, 2022

Choose a reason for hiding this comment

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

No opinion :) Rest LGTM

@litherum litherum force-pushed the defaultqueuelabel branch from 3bfeb72 to b938754 Compare March 15, 2022 20:13
@litherum litherum enabled auto-merge (rebase) March 15, 2022 20:13
@litherum litherum merged commit 00f11e1 into gpuweb:main Mar 15, 2022
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 15, 2022

Previews, as seen when this build job started (b938754):
WebGPU | IDL
WGSL
Explainer

@litherum litherum deleted the defaultqueuelabel branch March 16, 2022 19:05
@kainino0x kainino0x removed the tacit resolution candidate Editors may be able to resolve and move to tacit resolution queue label May 16, 2022
juj added a commit to juj/wasm_webgpu that referenced this pull request Aug 18, 2022
@juj
IDL: Add support for setting the debug label of defaultQueue on devic…
89bbec9 
…e creation. gpuweb/gpuweb#2645
@ErichDonGubler ErichDonGubler mentioned this pull request Aug 8, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kainino0x kainino0x kainino0x approved these changes

@toji toji toji approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@litherum @kainino0x @toji