[sagudev]

Connections
#10103 (comment)
This will directly fix the case in #7017 (comment)

Description
We could vendor all own types in wgpu-core-remote-types like it was suggested at the beginning of #10103, but we could just split wgt types into WebGPU and WGPU parts. I think this actually works great for WebGPU backend as it makes us better forward compatible if spec adds new values into them that we do not know how to use (currently such new bits would map into WGPU bits).

For typical usage API is same, there is just additional API of components and different bits representation which are probably not used much in the wild.

Testing
Just refactor

Squash or Rebase?
Rebase

Checklist

• I self-reviewed and fully understand this PR.
• WebGPU implementations built with wgpu may be affected behaviorally.
• Validation and feature gates are in place to confine behavioral changes.
• Tests demonstrate the validation and altered logic works.
• CHANGELOG.md entries for the user-facing effects of this change are present.
• The PR is minimal, and doesn't make sense to land as multiple PRs.
• Commits are logically scoped and individually reviewable.
• The PR description has enough context to understand the motivation and solution implemented.

[sagudev]

Technically we could have [u16; 2], but it makes easier to have u32 for webgpu part for conversions.

[teoxoy]

currently such new bits would map into WGPU bits

This is a really good observation that would likely bite us down the line.

[teoxoy]

We could vendor all own types in wgpu-core-remote-types like it was suggested at the beginning of #10103, but we could just split wgt types into WebGPU and WGPU parts.

It might still be worth decoupling the flags types at the wgpu-core-remote API by making them all u32s since that's what the WebGPU API actually accepts (typedef [EnforceRange] unsigned long GPUXFlags;).

[teoxoy]

@andyleiserson thoughts on this approach? It seems like a good way forward.

[sagudev]

We could vendor all own types in wgpu-core-remote-types like it was suggested at the beginning of #10103, but we could just split wgt types into WebGPU and WGPU parts.

It might still be worth decoupling the flags types at the wgpu-core-remote API by making them all u32s since that's what the WebGPU API actually accepts (typedef [EnforceRange] unsigned long GPUXFlags;).

I am not sure I understand. BufferUsagesWebGPU is u32 as this is also helps webgpu backend of wgpu.

[teoxoy]

I just mean having it be spelled as u32 rather than BufferUsagesWebGPU at the wgpu-core-remote API so that there are no questions about invalid bits at wgpu-core-remote's level - they would just be validation errors raised in wgpu-core.

Whenever I see a bitflags type I always think that it must only contain valid bits (which is not always true but I lean that way when I see one in source code).

[andyleiserson]

This seems reasonable to me. I had assumed that our API contract around native extensions was "bit values subject to change if claimed by the spec", but I'm not sure if that's stated anywhere, and it may be worth taking a minor breaking change to have a better story.

I agree with @teoxoy that the ability of bitflags to carry invalid bits is contrary to the "principle of least surprise" (right next to contains not being named contains_all), and it isn't what I would choose from first principles, but it also seems like it's an established step on the wgpu learning curve at this point. If we were going to make a change, some unvalidated newtype plus a new always-validated downstream type appeals to me more than raw integer on IPC plus downstream bitflags.

It does seem like some non-browser buy-in would be nice on both this change and #10218.

[andyleiserson]

My understanding is that wgpu extensions should be supported in Deno when the strict WebGPU flag is not active.

[sagudev]

My understanding is that wgpu extensions should be supported in Deno when the strict WebGPU flag is not active.

should -> could, because they are not currently. My idea for this is to have optional additional field wgpu_usages in descriptor that would then map into wgpu parts.

[andyleiserson]

The comment I left in GPUAdapter.features is perhaps a bit misleading:

wgpu/deno_webgpu/adapter.rs

Line 105 in e6af9e9

// Only expose WebGPU features, not wgpu native-only features

... but it is talking about what's exposed if you look at the features property. requestDevice validates against the original wgpu feature list.

wgpu/deno_webgpu/adapter.rs

Line 129 in e6af9e9

let supported_features = self.wgpu_adapter.features();

So I believe it is possible to enable native features, or at least, that was my intent, I'm not sure if we have a test for doing so or if anybody actually does.

Edit: this was discussed in #7813 (comment). We could actually do better now, we could expose the native features conditional on the STRICT_WEBGPU flag.

[sagudev]

I looks like it's possible to enable them, but it might not be possible to actually use them (no constants exposed but that is easily bypassed at least before this PR, no conversions for wgpu native objects).

[andyleiserson]

Seems like the easy options are either to change the type in Deno GPUBufferDescriptor to u64 and unpack the wgpu usages from the high bits, or to leave the wgpu usages inaccessible from Deno JS for the time being (and then worry about accessing extension APIs with the u64 or some other approach if the use case comes up).

@crowlKats thoughts?

[sagudev]

I think we are already doing second option in practice (at least for buffer usages, because even when they are all enabled one cannot meaningfully use them as ray tracing is otherwise not exposed in deno).

[sagudev]

If we were going to make a change, some unvalidated newtype plus a new always-validated downstream type appeals to me more than raw integer on IPC plus downstream bitflags.

I agree, too bad bitflags does not have validated variant. Anyway in wgc & remote we will always use unvalidated one and I prefer type rich bitflags rather then some opaque u32.

[andyleiserson]

It's possible we will want to plumb a native feature through the remoting at some point to enable experimental standard-track features, but using BufferUsagesWebGPU here still seems okay, we can change it the need arises.

[sagudev]

Experimental standard-track features should probably already go into BufferUsagesWebGPU, so webgpu backend can also use them.

[andyleiserson]

The expectation is that this is source-compatible in most cases, right? Meaning expressions like BufferUsages::MAP_WRITE | BufferUsages::VERTEX would continue to work, it's only a breaking change if somebody actually wants to treat BufferUsages as an integer?

Even so, it might be worth putting this in the "major changes" section of the changelog, or at least, saying something like "in most cases this is not expected to be a breaking change, but if you convert BufferUsages to/from other integers, some updates may be required" in the changelog entry.

[andyleiserson]

Seems like the easy options are either to change the type in Deno GPUBufferDescriptor to u64 and unpack the wgpu usages from the high bits, or to leave the wgpu usages inaccessible from Deno JS for the time being (and then worry about accessing extension APIs with the u64 or some other approach if the use case comes up).

@crowlKats thoughts?

[sagudev]

The expectation is that this is source-compatible in most cases, right? Meaning expressions like BufferUsages::MAP_WRITE | BufferUsages::VERTEX would continue to work, it's only a breaking change if somebody actually wants to treat BufferUsages as an integer?

That is correct.

Even so, it might be worth putting this in the "major changes" section of the changelog, or at least, saying something like "in most cases this is not expected to be a breaking change, but if you convert BufferUsages to/from other integers, some updates may be required" in the changelog entry.

Will do.