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

WebGPU: Return useful information in wgpuAdapterGetProperties #21072

Closed
austinEng opened this issue Jan 12, 2024 · 11 comments
Closed

WebGPU: Return useful information in wgpuAdapterGetProperties #21072

austinEng opened this issue Jan 12, 2024 · 11 comments
Labels

Comments

@austinEng
Copy link
Contributor

Currently, the implementation returns empty strings in all of the vendorName, architecture, device, name, etc.. fields.
It should be implemented by forwarding the fields from GPUAdapterInfo

@kainino0x

@beaufortfrancois
Copy link
Contributor

I'd be happy to implement this.

FYI I naively thought it would be as simple as something like this:

   wgpuAdapterGetProperties: (adapterId, properties) => {
-    {{{ gpu.makeCheckDescriptor('properties') }}}
-    {{{ makeSetValue('properties', C_STRUCTS.WGPUAdapterProperties.vendorID, '0', 'i32') }}};
-    {{{ makeSetValue('properties', C_STRUCTS.WGPUAdapterProperties.vendorName, '0', 'i32') }}};
-    {{{ makeSetValue('properties', C_STRUCTS.WGPUAdapterProperties.architecture, '0', 'i32') }}};
-    {{{ makeSetValue('properties', C_STRUCTS.WGPUAdapterProperties.deviceID, '0', 'i32') }}};
-    {{{ makeSetValue('properties', C_STRUCTS.WGPUAdapterProperties.name, '0', 'i32') }}};
-    {{{ makeSetValue('properties', C_STRUCTS.WGPUAdapterProperties.driverDescription, '0', 'i32') }}};
-    {{{ makeSetValue('properties', C_STRUCTS.WGPUAdapterProperties.adapterType, gpu.AdapterType.Unknown, 'i32') }}};
-    {{{ makeSetValue('properties', C_STRUCTS.WGPUAdapterProperties.backendType, gpu.BackendType.WebGPU, 'i32') }}};
-    {{{ makeSetValue('properties', C_STRUCTS.WGPUAdapterProperties.compatibilityMode, '0', 'i32') }}};
+    var adapter = WebGPU.mgrAdapter.get(adapterId);
+    {{{ runtimeKeepalivePush() }}}
+    adapter["requestAdapterInfo"]().then((info) => {
+      {{{ runtimeKeepalivePop() }}}
+      {{{ gpu.makeCheckDescriptor('properties') }}}
+      {{{ makeSetValue('properties', C_STRUCTS.WGPUAdapterProperties.vendorID, '0', 'i32') }}};
+
+      var vendorNameSize = lengthBytesUTF8(info.vendor) + 1;
+      var vendorNamePtr = _malloc(vendorNameSize);
+      stringToUTF8(info.vendor, vendorNamePtr, vendorNameSize);
+
+      {{{ makeSetValue('properties', C_STRUCTS.WGPUAdapterProperties.vendorName, 'vendorNamePtr', '*') }}};
...

But it turns out it's not. @kainino0x Any idea why this code does not work?
I assume it's because of the async nature of it. All the code in this file using promises have callback while this one has not.

@kainino0x
Copy link
Collaborator

kainino0x commented Jan 22, 2024

Hm, right... I forgot that it was async in JS but sync in C.

I think to make this work we would need to have wgpuInstanceRequestAdapter actually do both requestAdapter AND requestAdapterInfo, then store off the results in case wgpuAdapterGetProperties is called later. I'm not sure if this is what we had in mind. It would work, but would make wgpuInstanceRequestAdapter slightly slower, and also would unnecessarily access fingerprintable info that the page may not actually need.

Alternatives would be:

  • Make it async in C
  • Somehow include in the wgpuInstanceRequestAdapter call whether you want to retrieve the properties or not

Filed webgpu-native/webgpu-headers#266

@beaufortfrancois
Copy link
Contributor

Once #22031 is merged, I'll update wgpuAdapterGetProperties as well as it's very similar.

@beaufortfrancois
Copy link
Contributor

@kainino0x
Copy link
Collaborator

I don't think we need to fix GetProperties, we are going to remove it anyway. Better to just tell people to use the new GetInfo if they want this data - I think it's probably stable at this point.

@beaufortfrancois
Copy link
Contributor

What are the next steps then? Shall we remove it from webgpu.h first, then deprecate its support in Emscripten and Dawn?

@kainino0x
Copy link
Collaborator

Yes, I think:

  • Remove it from webgpu.h
  • Optionally add a warnOnce in Emscripten (it never worked anyway though so it doesn't really matter)
  • Add a deprecation warning in Dawn

@beaufortfrancois
Copy link
Contributor

beaufortfrancois commented Jun 12, 2024

  • Remove it from webgpu.h

I've started webgpu-native/webgpu-headers#305

  • Optionally add a warnOnce in Emscripten (it never worked anyway though so it doesn't really matter)

I've started #22088

  • Add a deprecation warning in Dawn

I'll work on that then

@beaufortfrancois
Copy link
Contributor

  • Add a deprecation warning in Dawn

I'll work on that then

Here's the WIP CL: https://dawn-review.googlesource.com/c/dawn/+/193043

@beaufortfrancois
Copy link
Contributor

I believe we can close this issue as next time we upstream changes in Emscripten, getProperties won't be there anymore according to #21552 (comment)
@kainino0x What do you think?

@kainino0x
Copy link
Collaborator

Agreed, thanks!

@kainino0x kainino0x closed this as not planned Won't fix, can't repro, duplicate, stale Sep 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants