Conversation
|
|
||
| interface GPUPipelineLayout { | ||
| }; | ||
|
|
There was a problem hiding this comment.
You should remove createPipelineLayout too. However, having a separate object can be helpful for signifying pipeline compatibility to the user.
There was a problem hiding this comment.
Removed createPipelineLayout too, thanks for the catch.
Pipeline layout wouldn't have been used for compatibility except for "bind group inheritance" if we choose to have that. The same inheritance can exist with pipelines so I'm not too worried.
The pipeline layouts only contained the list of bind group layouts and didn't add any value by being a separate object. Removing them reduces the API complexity.
kainino0x
left a comment
There was a problem hiding this comment.
Seems ok, I'd only be concerned because Vulkan seems to think it's necessary. In particular, "pipeline layouts are used as part of pipeline creation (see Pipelines), as part of binding descriptor sets (see Descriptor Set Binding), and as part of setting push constants (see Push Constant Updates)."
- pipeline creation: we still have the layout in pipeline creation
- binding descriptor sets: why don't we need it? we're not currently passing the pipeline layout into setBindGroups. Are we (in the implementation) doing it lazily (i.e. once we know what the actual pipeline, and thereore pipeline layout, is)?
- push constants: same as ^, probably, if we add them
This reminds me of #92 Following the same logic, There are two kind of API complexities:
This PR doesn't strictly reduce the complexities, it just trades (1) for (2). I believe this change makes the API harder to reason about. For example, consider the user who only creates 2 rendering pipelines and draws a bunch of objects mixed between them. The user realizes that the first bind group is actually only containing a single uniform buffer that isn't used by the second pipeline, and they consider "optimizing" it by removing the unused bind group layout. Depending on the background of the user, one of the two scenarios could happen:
You can see how this is related to "pit of success" and "spooky action at a distance" concepts that's been voiced before in the group. |
|
@kainino0x like you said, all of these issues should be workable if we apply things lazily. It is a bit more work for implementations for sure.
Like you said, it is a tradeoff. I don't think I hadn't thought about this proposal making it easier to produce pipeline layout / root signature layout incompatibilities but it is a good point. My hope is that with proper bind group inheritance rules we show developers that it is good for pipelines to be compatible. Basically this would be trading a lot of 1) for less of 2). I'm not 100% sure about it, and it is something that is easy to change post-snapshot if developers tell us they feel the pipeline layout is redundant. |
|
I ran this proposal by a contact on the D3D team. He is in agreement with @kvark. From a D3D12 perspective, separating the RootSignature from the PipelineState allows developers to make informed decisions about optimizing around shared root signatures. Drivers can also take advantage of caching (pre-fetching) some of the input data ahead without the need to specify the full PipelineState in a CommandList (which could kick off a shader compile operation). However, there is nothing that prevents this. As @kvark implies, this trades API convenience for execution efficiency. |
|
Thanks @RafaelCintron for the feedback. It seems like keeping the pipeline layout object makes sense as removing it could be hiding too much from implementations. Closing this as won't fix. |
* Expect GPUOutOfMemoryError in map_oom tests Fixes gpuweb#199. Also adds a helper to asynchronously expect errors. * OperationError for mapping after out-of-memory; expectGPUError
The pipeline layouts only contained the list of bind group layouts and
didn't add any value by being a separate object. Removing them reduces
the API complexity.