whatwg / webidl

Web IDL Standard
https://webidl.spec.whatwg.org/
Other
405 stars 162 forks source link

Allow dictionary default values to reference type members #717

Open grorg opened 5 years ago

grorg commented 5 years ago

Hello. Long time reader, first time issue raiser. Love your work.

In WebGPU, we have a lot of cases where we want to do something like this:

typedef unsigned long u32;
typedef u32 GPUColorWriteFlags;

interface GPUColorWriteBits {
    const u32 NONE = 0;
    const u32 RED = 1;
    const u32 GREEN = 2;
    const u32 BLUE = 4;
    const u32 ALPHA = 8;
    const u32 ALL = 15;
};

dictionary GPUColorStateDescriptor {
    GPUColorWriteFlags writeMask = GPUColorWriteBits.ALL;
};

Note the default value on the GPUColorStateDescriptor's writeMask. This is currently forbidden in WebIDL because:

DefaultValue ::
    ConstValue
    string
    [ ]
    null

I realise this isn't terribly common in Web APIs - most of the time your default value will be a constant value or a string from an enum. But it would be really nice to allow this type of dereferencing for WebGPU - saves you having to look up the constant value in the related interface.

annevk commented 5 years ago

Aside: I wonder if we should have a larger discussion around WebGL/WebGPU and IDL as the practices there deviate quite a bit from what we recommend to most other APIs. E.g., elsewhere the above API would be (effectively required to be) a dictionary.

grorg commented 5 years ago

@annevk review is most welcome!

The main issue we have is that some values in WebGPU are bitmasks, which don't translate terribly well to JS and WebIDL. Using dictionaries instead would be equally weird:

stateDescriptor.writeMask = { red: true, blue: true, alpha: true };

... which is what I think you're suggesting. Although that example isn't too bad, what happens with:

stateDescriptor.writeMask = { red: false, all: true };

With bitmasks there is a logical answer. With dictionaries you'd have to refer to the specification.

annevk commented 5 years ago

You'd have to adjust the design, perhaps not offer all at all. Presumably though bitmaps are used for performance reasons and using more "idiomatic" JavaScript would be bad for that?

Note that there's also questions your IDL doesn't answer without looking at the definition. E.g., what if pass 16? (Better IDL bitmask support might help with that, but then again the question is to what extent we want to encourage those kind of APIs, which seems like a bigger question we need to address somehow.)

grorg commented 5 years ago

We can't argue that performance is a driving factor, since the underlying platform graphics APIs might not use the same values in their bitmasks. In fact, I doubt they do.

But even if writeMask in my example was a dictionary type, how would I provide a default value of "all"?

annevk commented 5 years ago

There's not a direct mapping from bitmasks (for which I'm not sure we've got established JavaScript design patterns as they're avoided) that I know of. So you'd have to start from use cases and requirements and build something new up from there. (If you wanted to dig into this more the TAG and TC39 in particular would be good places to discuss this I think.)

grorg commented 5 years ago

OK. But if we put that issue aside, do you still think it is acceptable to have a default value that is a (const) member of another interface?

annevk commented 5 years ago

It depends on whether or not we want to endorse bitmasks. And then it depends on what the recommended design pattern around bitmasks is as to what needs to be exposed and how.

kainino0x commented 5 years ago

The value is not purely limited to bitmasks, e.g. imagine an interface with a DEFAULT_TIMEOUT constant used in a dictionary in one of it's member functions.

We can't argue that performance is a driving factor, since the underlying platform graphics APIs might not use the same values in their bitmasks.

I disagree. There is definitely way more overhead in taking in a dictionary and converting it (because of many accesses into a VM managed object) than converting one bitmask into another.

littledan commented 5 years ago

Constants are already marked as having their usage discouraged. What if, in conjunction with adding this permitted usage, we somehow made this recommendation somehow stronger, or even specifically referencing WebGL as a big incumbent user which will remain an exception to the rule?

grorg commented 5 years ago

So you're saying that you'd allow WebGPU to do it, but then recommend against what WebGPU is doing? That's a bit weird, but if it allows us to use a constant as a default value, then I'm ok with it.

The use of bitmasks is another issue.

annevk commented 5 years ago

As @kainino0x indicated I think the underlying issue here is that there might be a performance benefit to having API-styles that are more C-like and less JavaScript-like. And as you get closer to the metal it might make sense to endorse such styles, as already happened with WebGL. And it seems likely that with Wasm there might be an increase in demand for such APIs too.

On the flipside, this kind of thing came up for encodeInto() and there we decided (see https://github.com/whatwg/encoding/issues/69#issuecomment-434541423 onward) that allocating a new view and returning a dictionary of which the members might be used to create another view was acceptable and could likely be optimized away (after discussion with those implementing, to be clear). I'm not entirely convinced we've seen the end of that though.

cc @domenic @fitzgen @lukewagner

bzbarsky commented 5 years ago

My general take on this is that Web IDL exists to codify some union of "desirable" and "widespread" API design patterns. The aim is to be flexible enough to allow many things but make certain things easy.

In particular, there is a real cost to adding IDL features. It involves complexity in the IDL spec and in the multiple IDL parsers and validators (including critical ones that are already having trouble keeping up with spec changes, like the idlharness parser, but also the ones browsers use).

For the specific original motivating case here, I assume the concern with writing

    GPUColorWriteFlags writeMask = 15;

is that it's not clear to the spec reader what that 15 means in practice? I agree that this is a problem, and the "flexible non-built-in-feature" solution IDL offers for it at the moment is to write:

    GPUColorWriteFlags writeMask = 15 /* GPUColorWriteBits.ALL */;

to clearly communicate where this magic number comes from. It's a bit more burden on the spec author if the value of ALL changes, of course.

If we do want to have this sort of pattern in multiple places, then it may be worth creating dedicated IDL syntax for it. It's not clear to me whether that bar has been reached. I do think that adding some sort of syntax like this post-facto once it's clear that there is a real need for it has worked ok for IDL in the past.

So if my understanding of the problem that we're trying to solve here is correct, and if my proposed "solution" is acceptable for the moment, I would prefer to not add a dedicated feature for this until it's clear that we have more than one consumer.

grorg commented 5 years ago

As @kainino0x indicated I think the underlying issue here is that there might be a performance benefit to having API-styles that are more C-like and less JavaScript-like. And as you get closer to the metal it might make sense to endorse such styles, as already happened with WebGL. And it seems likely that with Wasm there might be an increase in demand for such APIs too.

For this particular API entry point, I slightly disagree. It won't be called in the rendering loop. Yeah, it will be slightly slower to map a dictionary through the bindings code compared to an int, and require allocation, but it wouldn't be terrible. There are plenty of other places in the current Web API where we do this (e.g. WebAnimations uses a dictionary that might contain a lot of style properties).

But his point is valid that there are entry points where we are trying to be as efficient as possible. And make the mapping to WASM as simple as possible (although we're JS-first).