Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add GPUAdaperInfo to the spec #2660

Merged
merged 6 commits into from May 21, 2022
Merged

Add GPUAdaperInfo to the spec #2660

merged 6 commits into from May 21, 2022

Conversation

toji
Copy link
Member

@toji toji commented Mar 10, 2022

Fixes #2191
Fixes #2195

First pass at formalizing the Adapter Identifiers design doc into spec prose. Have taken feedback from the most recent call into account, though I anticipate we'll still need to do some iteration on this.

Things to note:

  • Filled in the "Adapter Identifiers" section under privacy considerations, so be sure to give that a look in addition to the algorithms.
  • Changed fullName to description. This feels like a more appropriate general term that will hopefully further discourage attempts to parse it. Also coincidentally matches the D3D12 name for the value.
  • Current algorithm doesn't have any scenario under which it rejects, following the sentiment voiced by @kdashg (I think) that it feels less fragile that way.
  • Removed nullable values in favor of either empty string or 0 in all cases.
  • Design doc states that requestUnmaskedAdapterInfo() should only be called during user activation, but PR doesn't include that restriction because it's unclear how to enforce that requirement in a worker.
  • As currently written the spec indicates that every call to adapter.info would create a new GPUAdapterInfo, which is probably not a big deal but worth considering.

Also on my mind:

  • I'm not particularly happy with the attribute name deviceId in this GPUAdaperInfo because it conflicts with the API's use of the term "Device", but I haven't landed on an alternative that I'm happy with. adapterId sounds too much like something that uniquely identifies the adapter or indicates that it's then Nth adapter for the system. adapterTypeId just sounds awkward. And in many cases this would literally be the PCI Device ID, plus that's what Vulkan and D3D12 call it. (Metal has no direct equivalent.)
  • I would like to write out a recommended mapping for how these values could surface the equivalents in Vulkan/D3D12/Metal, but I'm not clear where that should go? Maybe in the design doc? In any case, it would be useful to sanity check that we're surfacing approximately the same things across implementations, especially in regards to Metal which is the most unique in how it surfaces device identifiers.

Preview | Diff

@toji toji added this to the V1.0 milestone Mar 10, 2022
@github-actions
Copy link
Contributor

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

spec/index.bs Outdated
The name of the family or class of GPUs the [=adapter=] belongs to, if avaliable. Empty
string otherwise.

: <dfn>deviceId</dfn>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think deviceId is fine, but maybe pciDeviceId? not sure whether this would ever be anything else.

aside: I had a thought about whether it should be Id or ID, but this page specifically says "Id, except when the first word in a method or property".

Copy link
Contributor

Choose a reason for hiding this comment

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

Exposing PCI device IDs to the web sounds like a terrible idea

Copy link
Member Author

Choose a reason for hiding this comment

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

I understand that sentiment, but I'm not sure what a useful alternative would be.

This value is exposed directly by Vulkan and D3D12 and named "deviceId" by both APIs. As far as I can tell Metal has no direct equivalent, but it should be able to populate the architecture attribute more accurately than other systems, assuming that the GPU family is a reasonable value to surface there. (Please let me know if you feel it's not!)

We could surface it as a string instead, but the implication there is that we would need to maintain a lookup table of every known vendor/device ID pairing to the product name, which is a large ongoing engineering time commitment that I don't want to require if I can help it. (I'm already uncomfortable with the mapping that architecture implies on non-Metal systems.)

On the other hand, if we don't expose anything more detailed than architecture on non-Metal systems there will be a strong motivation for developers to attempt to parse the description field to identify the specific device, which I definitely don't want to encourage.

As such, while I'm not a big fan of throwing an opaque number at the developer considering how we intend for them to use it (identifying buggy devices) it fulfills the need. Worth noting that I think Chrome would always require some form of user consent before unmasking this value.

Copy link
Member Author

Choose a reason for hiding this comment

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

Had a conversation with @kenrussell today in which he pointed out that the ANGLE team has had previous experience trying to expose a value like the deviceId proposed here, and suggested that we use a string for that purpose instead. Specifically, he pointed out that one some devices the ANGLE team needed to report a 64 bit number. If we use a string here then there's a lot more flexibility in what can be reported.

I would still expect some platforms/UAs to report a PCI device ID here, probably just formatted as a hex string.

I also took the liberty of renaming the value to simply "device" since it's not going to be strictly numeric. Still have some mild concerns about the terminology conflict with GPUDevice and am open to alternatives, but I think the context will probably clear up any ambiguity about it's use.

spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
@toji
Copy link
Member Author

toji commented Apr 12, 2022

Forgot that the design doc (which I've updated to match changes made during this review) specified that unmasking adapter info required user activation, so I've added that to the requestAdapterInfo() algorithm.

@litherum: It would be ideal if we could get to a point where we can land this prior to sending a review request to PING. I expect the design doc to be the most critical piece for understanding the reasoning of why this feature exists and why it's structured the way it is, but the algorithms shown in the spec will also be important for understanding how that intent is executed. As such, a more thorough review would be appreciated!

@github-actions
Copy link
Contributor

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

@github-actions
Copy link
Contributor

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

@Kangz
Copy link
Contributor

Kangz commented May 17, 2022

@litherum, this PR has been blocked on your feedback for quite some time now (more than a month) with multiple pings. Unless you strongly object, I think we should go forward with landing it. You'd still be able to provide feedback via issues / pull requests but at least spec text would be there for iteration instead of staying in a PR.

@toji
Copy link
Member Author

toji commented May 21, 2022

Given that there's hasn't been a objection raised to merging this over the last few days, I'm going to do so now. As @Kangz mentioned, there is still ample room for feedback at this point, and I'm especially looking forward to hearing what the PING thinks about it. Hopefully this merge will make it easier for that review to happen.

@toji toji merged commit 2e1999f into main May 21, 2022
@toji toji deleted the adapter-info branch May 21, 2022 00:05
juj added a commit to juj/wasm_webgpu that referenced this pull request Aug 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Exposing GPU hardware information to web developers in WebGPU GPUAdapter.name considered harmful
5 participants