wgpuAdapterGetProperties is sync but requestAdapterInfo is async

[kainino0x]

Originally posted by @beaufortfrancois in emscripten-core/emscripten#21072 (comment)

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.

Originally posted by @kainino0x in emscripten-core/emscripten#21072 (comment)

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

[beaufortfrancois]

Would it make sense to replace wgpuAdapterGetProperties with wgpuAdapterRequestAdapterInfo instead?

[Kangz]

Somehow include in the wgpuInstanceRequestAdapter call whether you want to retrieve the properties or not

Seems sensible? At least in C it doesn't seem super necessary to keep the retrieval of info async.

[rajveermalviya]

Seems like this is something emscripten layer would have to workaround, because in the native it doesn't make sense to make it async, info callbacks can be especially annoying when enumerating adapters.

and also would unnecessarily access fingerprintable info that the page may not actually need.

not sure how to avoid that

[kainino0x]

and also would unnecessarily access fingerprintable info that the page may not actually need.

not sure how to avoid that

That's the idea of "Somehow include in the wgpuInstanceRequestAdapter call whether you want to retrieve the properties or not": if you don't ask for it in RequestAdapterOptions, then we don't need to add the extra delay or the extra data access.

I think that's a pretty good option. Maybe it would look something like a new field in RequestAdapterOptions that's a bitfield with just 1 bit so far (0 = don't requestAdapterInfo(), 1 = do requestAdapterInfo()). Later on we might have extra bits to ask for more info.

[kainino0x]

Feb 1 meeting:

• (brief discussion, sorry didn’t minute it)
• Better to have it match the JS spec. Not too bad with promises
• Maybe propose an addition to the JS API:
  • 1. sync getter for the adapter info you currently have
  • 2. option to request info in requestAdapter
• Could do this in C API without a JS spec change, but not necessary to diverge.

[beaufortfrancois]

Out of curiosity, what are the next steps?

[kainino0x]

Add WGPUAdapterRequestAdapterInfo to the C API (on our side, implement in Dawn) matching the usual patterns for async things. It will use futures, but we could have a stopgap version without futures if we really need one.

[beaufortfrancois]

Thanks! Here's #269

[kainino0x]

Add WGPUAdapterRequestAdapterInfo to the C API (on our side, implement in Dawn) matching the usual patterns for async things. It will use futures, but we could have a stopgap version without futures if we really need one.

Hm, I forgot we also wanted to remove wgpuAdapterGetProperties.
I also forgot WGPUAdapterProperties has extra info in it.
We'll need a proposal, I'm not sure if we want to move the extra info to an extension of WGPUAdapterInfo (how?) or keep GetProperties for this extra info (but as a native-only extension with no implementation in Emscripten)

[kainino0x]

Adding agenda label, but I'd like to get a proposal written down (from someone) before we discuss it.

[kainino0x]

(oops reposted old minutes)

[kainino0x]

Feb 15 meeting:

• Previously we said we would just mirror the JS API into C for requestAdapterInfo. But in order to match we also need to do something about GetProperties. AdapterProperties has a few extra fields (vendorID, deviceID, adapterType, backendType).
  • Remove GetProperties and move its extra data to some extension of RequestAdapterInfo?
  • Keep GetProperties as a native-only extension? (with just those 4 fields?)
• RM: Think we should make it an extension struct
• KN: If it’s an extension on a struct passed to the callback, then it will create a backward-compat risk because applications need to handle chained struct types they don’t know about.
• …
• LK: Have the application pass the chained struct into RequestAdapterInfo, and all the callback does is tell you it’s done?
• KN: Yeah, probably better. I think we had an open issue about this, not sure what happened to it. We should make any other similar cases do the same thing. I should go through and check if there are others.
• RM: Requires FreeMembers on those structs.
  • (not a problem. we already have this, as AdapterPropertiesFreeMembers)

[kainino0x]

• I should go through and check if there are others.

It is also a problem for GetCompilationInfo, which is more complicated because both WGPUCompilationInfo and WGPUCompilationMessage are extensible. Filed #272

[kainino0x]

The API that I think we came to for AdapterInfo - I took some liberties in filling in details, in particular moving backendType and adapterType to the core struct because it's sometimes possible to populate them in Wasm.

typedef struct WGPUAdapterInfo {
    WGPUChainedStructOut * nextInChain;
    char const * vendor;
    char const * architecture;
    char const * device;
    char const * description;
    // Wasm can always tell you this, it's WGPUBackendType_WebGPU
    WGPUBackendType backendType;
    // Wasm may occasionally know this (e.g. if forceFallbackAdapter=true). Otherwise Unknown.
    WGPUAdapterType adapterType;
} WGPUAdapterInfo WGPU_STRUCTURE_ATTRIBUTE;

// Chains off WGPUAdapterInfo
typedef struct WGPUAdapterInfoVendorDeviceID {
    WGPUChainedStruct chain;
    uint32_t vendorID;
    uint32_t deviceID;
} WGPUAdapterInfoNative WGPU_STRUCTURE_ATTRIBUTE;

WGPUAdapterInfoVendorDeviceID's SType would be in the 0x0001_xxxx range: #214 (comment)

EDIT: had a wrong signature. removed it, posted correct one below.

[kainino0x]

(Unfortunately this doesn't make CompilationInfo in #272 any easier)

[Kangz]

The proposal above looks good, and in native/wire I believe the wgpuAdapterRequestAdapterInfo wouldn't need to be called. Also aren't getters supposed to return a WGPUBool?

[kainino0x]

Oh yes, right, I copied yours with the bool and deleted the wrong one. (Later will become a WGPUStatus or whatever)

[kainino0x]

Feb 29 meeting:

• KN: (explanation of new proposal)
• LK: Now the extension chain to GetAdapterInfo doesn’t serve as a request, because the requestAdapterInfo is separate
• Tentatively OK
• (But then we discussed more in Passing extensible structures to application callbacks, backward-compatibly #272…)

[kainino0x]

(comment moved to #272 (comment))

[kainino0x]

Posted a different proposal on #272 (comment).
I think we'll continue the discussion there so we can design both together.

[kainino0x]

A few more thoughts for now.

• If we do end up adding a bit of info to the requestAdapterInfo call in JS, it may just be one bit ("allow permission prompt"), hence it wouldn't have lined up with the allowedSTypes array discussed in Passing extensible structures to application callbacks, backward-compatibly #272 like I thought it would. One more point against the allowedSTypes proposal.
• I remembered that if we are thinking about the browser measuring the amount of fingerprint bits the page has accessed, then actually the requestAdapterInfo call is irrelevant, what actually matters is which attribute getters of GPUAdapterInfo are called.

  • I filed Issues with representing some JS interfaces as C structs #279 about this problem generally.

  • I think the thing I said before about calling requestAdapterInfo in Wasm RequestAdapter was wrong:

    would unnecessarily access fingerprintable info that the page may not actually need

    This means we could simplify by calling JS requestAdapterInfo unconditionally when creating an adapter and just providing GetAdapterInfo.

  • If we did getters they could be simplified by flattening GPUAdapterInfo into GPUAdapter, putting the 4 getters on GPUAdapter.

  • We could alternatively pass a bitmask into GetAdapterInfo (or if we don't add that, then into RequestAdapterInfo) with one bit per field, so the Wasm bindings only have to access the fields requested.

[kainino0x]

Mar 7 meeting:

• Should we do the proposal of making this synchronous in C by calling .requestAdapterInfo inside wgpuInstanceRequestAdapter?
  • Seriously considering it, but doesn’t match JS API.
• Should we try to fix the JS API?
  • Yes. Go propose it.

[beaufortfrancois]

Out of curiosity, how would you fix the JS API?

[kainino0x]

I haven't gotten around to opening a proposal yet but the gist is add the adapter info as adapter.adapterInfo or adapter.getAdapterInfo(). If there's a way to get adapter info without a prompt, then IMO there's no reason it needs to be async. We don't need async in order to dynamically decide what to expose / measure what we've exposed; properties on GPUAdapterInfo are already getters.

[kainino0x]

gpuweb/gpuweb#4536

[kainino0x]

Mar 28 meeting:

• JS API folks were OK with a sync API, so opened a PR and expecting it to be approved
• No problem here

[kainino0x]

• JS API folks were OK with a sync API, so opened a PR and expecting it to be approved

(And we can always do the requestAdapterInfo-in-wgpuInstanceRequestAdapter trick regardless of whether it is.)

Actual design depended on #279, but that's resolved now too, so we need a proposal. Most likely we just make it look like GetProperties but restructure as needed.

[kainino0x]

I think we reuse the structs and FreeMembers definitions proposed above, and the getter is simply as Corentin wrote it:

typedef struct WGPUAdapterInfo {
    WGPUChainedStructOut * nextInChain;
    char const * vendor;
    char const * architecture;
    char const * device;
    char const * description;
    // Wasm can always tell you this, it's WGPUBackendType_WebGPU
    WGPUBackendType backendType;
    // Wasm may occasionally know this (e.g. if forceFallbackAdapter=true). Otherwise Unknown.
    WGPUAdapterType adapterType;
} WGPUAdapterInfo WGPU_STRUCTURE_ATTRIBUTE;

// Chains off WGPUAdapterInfo
typedef struct WGPUAdapterInfoVendorDeviceID {
    WGPUChainedStruct chain;
    uint32_t vendorID;
    uint32_t deviceID;
} WGPUAdapterInfoVendorDeviceID WGPU_STRUCTURE_ATTRIBUTE;

WGPU_EXPORT wgpuAdapterInfoFreeMembers(WGPUAdapterInfo adapterInfo) WGPU_FUNCTION_ATTRIBUTE;

WGPU_EXPORT WGPUStatus wgpuAdapterGetInfo(WGPUAdapter adapter,
    WGPUAdapterInfo *info) WGPU_FUNCTION_ATTRIBUTE;

or "GetAdapterInfo". I've proposed adapter.info in the JS API but it could be bikeshedded; we should try to match it.

[kainino0x]

typedef struct WGPUAdapterInfo {
    WGPUChainedStructOut * nextInChain;
    char const * vendor;
    char const * architecture;
    char const * device;
    char const * description;
    // Wasm can always tell you this, it's WGPUBackendType_WebGPU
    WGPUBackendType backendType;
    // Wasm may occasionally know this (e.g. if forceFallbackAdapter=true). Otherwise Unknown.
    WGPUAdapterType adapterType;
    // Vendor/device ID are zero if you can't get them (like on Wasm)
    uint32_t vendorID;
    uint32_t deviceID;
} WGPUAdapterInfo WGPU_STRUCTURE_ATTRIBUTE;

WGPU_EXPORT wgpuAdapterInfoFreeMembers(WGPUAdapterInfo adapterInfo) WGPU_FUNCTION_ATTRIBUTE;

WGPU_EXPORT WGPUStatus wgpuAdapterGetInfo(WGPUAdapter adapter,
    WGPUAdapterInfo *info) WGPU_FUNCTION_ATTRIBUTE;

[kainino0x]

Apr 11 meeting:

• Need to figure out the API shape
• CF: How do you know if you can use the VendorDeviceID chain?
  • KN: Hm, didn’t propose an instance feature for this…
    • AE: or adapter feature
  • Need one?
  • Not worth it, probably just bake it into the main struct
• AE: what about fingerprinting and JS checking what properties you access?
  • KN: conclusion was: probably doesn’t matter for any C program from web assembly - because they’re going to do actual real work, so we probably have to assume they know everything that we would expose in the adapter info anyway
    • architecture, from draw/compute results
    • limits, from validation results
  • Inline the vendor/device id
• code just posted looks good

[beaufortfrancois]

Shall we close this issue?

[kainino0x]

Yes, I think so. Thanks!

[kainino0x]

Oops (#304), we forgot to remove WGPUAdapterProperties/wgpuAdapterGetProperties along with that. The new wgpuAdapterGetInfo replaces both wgpuAdapterRequestAdapterInfo and wgpuAdapterGetProperties (that's why we added the vendor/device ID to the struct).