emscripten icon indicating copy to clipboard operation
emscripten copied to clipboard

WebGPU: Return useful information in wgpuAdapterGetProperties

Open austinEng opened this issue 1 year ago • 9 comments

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

austinEng avatar Jan 12 '24 19:01 austinEng

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.

beaufortfrancois avatar Jan 22 '24 14:01 beaufortfrancois

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 https://github.com/webgpu-native/webgpu-headers/issues/266

kainino0x avatar Jan 22 '24 23:01 kainino0x

Once https://github.com/emscripten-core/emscripten/pull/22031/ is merged, I'll update wgpuAdapterGetProperties as well as it's very similar.

beaufortfrancois avatar Jun 05 '24 08:06 beaufortfrancois

You can follow WIP at https://github.com/beaufortfrancois/emscripten/compare/info...beaufortfrancois:emscripten:properties?expand=1

beaufortfrancois avatar Jun 06 '24 07:06 beaufortfrancois

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.

kainino0x avatar Jun 06 '24 19:06 kainino0x

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

beaufortfrancois avatar Jun 11 '24 09:06 beaufortfrancois

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

kainino0x avatar Jun 12 '24 00:06 kainino0x

  • Remove it from webgpu.h

I've started https://github.com/webgpu-native/webgpu-headers/pull/305

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

I've started https://github.com/emscripten-core/emscripten/pull/22088

  • Add a deprecation warning in Dawn

I'll work on that then

beaufortfrancois avatar Jun 12 '24 09:06 beaufortfrancois

  • 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 avatar Jul 03 '24 13:07 beaufortfrancois

I believe we can close this issue as next time we upstream changes in Emscripten, getProperties won't be there anymore according to https://github.com/emscripten-core/emscripten/issues/21552#issuecomment-2319481925 @kainino0x What do you think?

beaufortfrancois avatar Sep 02 '24 12:09 beaufortfrancois

Agreed, thanks!

kainino0x avatar Sep 07 '24 00:09 kainino0x