Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename getBindGroupLayout() to createBindGroupLayout() #3804

Closed
wants to merge 1 commit into from

Conversation

litherum
Copy link
Contributor

@litherum litherum commented Feb 3, 2023

Every time it's called, it returns a new object. This is observable from Javascript like so:

let bgl = pipeline.createBindGroupLayout();
bgl.foo = "bar";
let bgl2 = pipeline.createBindGroupLayout();
assert(bgl2.foo == undefined);

Because it returns a new object each time, naming it "create" makes it more clear.

Every time it's called, it returns a new object. This is observable from Javascript like so:

let bgl = pipeline.createBindGroupLayout();
bgl.foo = "bar";
let bgl2 = pipeline.createBindGroupLayout();
assert(bgl2.foo == undefined);

Because it returns a new object each time, naming it "create" makes it more clear.
@litherum litherum changed the title Rename getBindGroupLayout() to createBindGroupLayout() Rename getBindGroupLayout() to createBindGroupLayout() Feb 3, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Feb 3, 2023

Previews, as seen when this build job started (020879e):
WebGPU webgpu.idl | Explainer | Correspondence Reference
WGSL grammar.js | wgsl.lalr.txt

@kainino0x kainino0x added this to the V1.0 milestone Feb 3, 2023
@kainino0x
Copy link
Contributor

I'm unsure if this is less confusing. It's a new object handle, with a new label, yes, but bind group layouts are otherwise by-value objects. If you getBindGroupLayout() twice there's almost no difference between the two objects.

I will note the semantics of the C API have been questioned before (and not litigated in webgpu-headers): "get" makes it seem it returns the same object handle with the same label field (which in Dawn it does) but also like it does not increment the refcount (which in Dawn it does) (if there ends up being a refcount in the API).

@litherum
Copy link
Contributor Author

litherum commented Feb 4, 2023

I will note the semantics of the C API have been questioned before (and not litigated in webgpu-headers): "get" makes it seem it returns the same object handle with the same label field (which in Dawn it does) but also like it does not increment the refcount (which in Dawn it does) (if there ends up being a refcount in the API).

Yes, we are struggling with this rn. We are jumping through some moderately-herculean hoops to make sure:

  1. Javascript can retain, or stop retaining, anything it feels like, and if Javascript retains a reference to something, that thing should continue to be valid and useful in the future, regardless of whether or not its creator object gets GC'ed
  2. WebGPU.h's getFoo() does not bump an internal reference count
  3. WGPU objects are not reference counted (the internal reference counting isn't exposed through WebGPUExt.h)
  4. There are no retain cycles

But I think this rename is valuable even ignoring WebGPU.h. The function returns a new thing. We could have designed it differently, to not return a new thing (where the web process would internally retain the thing it returns, for future calls). But we didn't.

Bind group layouts are immutable, so the semantics of getBindGroupLayout really are "it creates a new object," even if an implementation chooses to have multiple wrappers pointing to the same object under the hood.

webkit-commit-queue pushed a commit to litherum/WebKit that referenced this pull request Feb 5, 2023
…emantics

https://bugs.webkit.org/show_bug.cgi?id=251732
rdar://105030025

Reviewed by Dean Jackson.

This is a partial revert of 259609@main. You would think that, because the name of "getBindGroupLayout()"
starts with the word "get," that it would have "get" semantics. However, the spec actually explicitly
describes that it has "create" semantics. This patch updates the implementation to have those semantics.

The spec specifically says "A new GPUBindGroupLayout wrapper is returned each time."

gpuweb/gpuweb#3804 is a PR to the spec to rename the functions, to be more clear
about their behavior.

* Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUComputePipelineImpl.cpp:
(PAL::WebGPU::ComputePipelineImpl::getBindGroupLayout):
* Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUComputePipelineImpl.h:
* Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPURenderPipelineImpl.cpp:
(PAL::WebGPU::RenderPipelineImpl::getBindGroupLayout):
* Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPURenderPipelineImpl.h:
* Source/WebCore/PAL/pal/graphics/WebGPU/WebGPUComputePipeline.h:
* Source/WebCore/PAL/pal/graphics/WebGPU/WebGPURenderPipeline.h:
* Source/WebGPU/WebGPU/APIConversions.h:
(WebGPU::releaseToAPI):
* Source/WebGPU/WebGPU/ComputePipeline.h:
* Source/WebGPU/WebGPU/ComputePipeline.mm:
(WebGPU::ComputePipeline::getBindGroupLayout):
(wgpuComputePipelineGetBindGroupLayout):
* Source/WebGPU/WebGPU/RenderPipeline.h:
* Source/WebGPU/WebGPU/RenderPipeline.mm:
(WebGPU::RenderPipeline::getBindGroupLayout):
(wgpuRenderPipelineGetBindGroupLayout):
* Source/WebKit/GPUProcess/graphics/WebGPU/RemoteComputePipeline.cpp:
(WebKit::RemoteComputePipeline::getBindGroupLayout):
* Source/WebKit/GPUProcess/graphics/WebGPU/RemoteRenderPipeline.cpp:
(WebKit::RemoteRenderPipeline::getBindGroupLayout):
* Source/WebKit/WebProcess/GPU/graphics/WebGPU/RemoteComputePipelineProxy.cpp:
(WebKit::WebGPU::RemoteComputePipelineProxy::getBindGroupLayout):
* Source/WebKit/WebProcess/GPU/graphics/WebGPU/RemoteComputePipelineProxy.h:
* Source/WebKit/WebProcess/GPU/graphics/WebGPU/RemoteRenderPipelineProxy.cpp:
(WebKit::WebGPU::RemoteRenderPipelineProxy::getBindGroupLayout):
* Source/WebKit/WebProcess/GPU/graphics/WebGPU/RemoteRenderPipelineProxy.h:
* Source/WebKit/WebProcess/GPU/graphics/WebGPU/RemoteShaderModuleProxy.cpp:
(WebKit::WebGPU::RemoteShaderModuleProxy::compilationInfo):

Canonical link: https://commits.webkit.org/259866@main
@Kangz
Copy link
Contributor

Kangz commented Feb 5, 2023

While I agree that get is a bit misleading, create is much more so because it sounds like a new layout will be computed when instead it already implicitly exists. reflectBindGroupLayout if we absolutely need to rename?

@kainino0x
Copy link
Contributor

kainino0x commented Feb 8, 2023

To try to put down some thoughts from yesterday's editor meeting, and others, into text:

  • The word "create" was originally meant to refer to creating the backing object, not the JS handle.
  • The naming friction is caused by nested many-to-one relationships (many js references to one js object, many js objects to one webgpu object). The following attempts to resolve this:
    • It was raised that most objects can be thought of as pure values without distinct identities, not handles, and re-wrapping an object can be thought of as a copy/clone, rather than a new handle. Since labels are properties of the handle, labels still work in this model. This model works for many objects, like samplers. It works here, albeit with some refactoring of the mental model: an automatic bind group layout is a value containing a pipeline value, and a pipeline is a value containing some unique ID rather than having an intrinsic identity.
    • In this model, the most accurate word here would be "clone" or "copy".
    • Of course, this doesn't work for anything with multiple handles sharing mutable state: device, texture, buffer, and query set (encoders will probably never have multiple handles). I think it is more confusing for the mental model to have some objects be pure values, and others not, when we could simply describe them all as handles. (Refer also to past hand-wringing over which states are on which timelines, Distinction between content-timeline and device-timeline state #2962.)
  • I think it's important to convey the idea that this is giving you an existing thing and not creating a thing: specifically, no argument to this function can change what it returns (just which one it returns). For this reason I favor "get", or if necessary a more semantic-agnostic word like "query". (We can explain the JS semantics in documentation.)
  • 99% of the time I don't think programmers will care whether two variables holding a webgpu object hold the same js object or different js objects so I still think "get" is fine.

Well, I've upset myself with the number of words I wrote about the choice of a single word, but here we are.

@toji
Copy link
Member

toji commented Feb 17, 2023

Consensus from the F2F: Don't change this function, but rename compilationInfo() to getCompilationInfo() (See #3805)

@toji toji closed this Feb 17, 2023
@kdashg
Copy link
Contributor

kdashg commented Feb 28, 2023

GPU Web F2F 2023-02-16/17

Bikeshedding names #3804 #3805 [Myles/Brandon/Kai?]

  • Rename getBindGroupLayout() to createBindGroupLayout() #3804
  • MM: we have a function, it has "create" semantics, but it's called "get".
  • KN: counterpoint: that's not what we intended "create" to mean. "create" creates a WebGPU object, not a JS object. Yes, it creates a JS object, nothing else matches it.
  • JS: don't we have another createBindGroupLayout?
  • KN: yes, on the device.
  • JS: other examples of similar names on different objects?
  • KN: yes, destroy().
  • MM: you can create it with a BGL or a set of BGLs. This thing will either return the BGL specified, or an autogenerated one. Doesn't actually create anything.
  • KG: leave it then?
  • MM: OK, let's close it.
  • Rename compilationInfo() to createCompilationInfo() #3805
  • MM: similar, creates new object from JS's perspective.
  • KG: does this IDL say "new object"?
  • BJ: don't think so, but we could say that.
  • DJ: why does the other one say "get"?
  • BJ: this is a function call rather than a getter. Add'l work done, getting back new result every time. If it was overhead-free - I'd probably drop the "get" from the other one.
  • CW: getBGL(), you pass it the BGL index. Has to be a function for this reason.
  • MM: if you wanted getBGL to be a property, would be a Promise<FrozenArray>. Not proposing that.
  • DJ: previous one, call it multiple times, you get back different objects?
  • BJ: Chrome's impl - multiple JS objects pointing to the same Dawn object.
  • MM: if you use an expando property, different on different return values. Deduplicating, you mess up the debug labels. Programming model is - each one has a label. From JS perspective, it's exactly create semantics.
  • DN: does Wasm have different creation semantics?
  • KG: everything's const through JS.
  • KN: in Dawn without wire, we give you the same pointer each time. With the wire, we might not know - give you a handle.
  • CW: back to compilationInfo().
  • KG: think it should be compilationInfo() if it's the same object, createCompilationInfo() if different objects.
  • BJ: if same object, I'd want it to be a getter.
  • CW: it's a Promise.
  • CW: what about "request"?
  • Discussion.
  • KN: there's a real reason these have different names. "get" is correct in my mental model. This one, there's no GPU object it represents. I don't think there's anything wrong with these names.
  • KG: think they all feel tolerable to me.
  • BJ: that's the key for me as well. Maybe don't line up the same way, but nothing unambiguously better. Nobody will get their depth buffer inverted because these are incorrectly named.
  • Could make this "get"..
  • KN: can't put SameObject on something that returns a Promise. You could call it again, may need to throw an Exception, can't throw, have to reject, but can't. Can't be the same Promise.
  • CW: call it getCompilationInfo?
  • KN: it's concretely different from getBGL. Don't see why it should have to be the same.
  • KN: nothing against "get", just don't think it's concretely better.
  • CW: strong preference for anything here?
  • RC: going back, why can't we return the same JS object from getBGL?
  • KN: we can.
  • CW: difficulty is - we have a cross-process model. If you expect same JS wrapper, then given JS wrapper to createPipelineLayout, then get the BGL, you'd expect the explicit one you returned much earlier. Difficulty: what if your Pipeline is an error on the service side? It doesn't give you a BGL. It gives you an error BGL. Suddenly, you call getBGL - Error BGL.
  • BJ: concrete problem WebGL ran into.
  • KR: yes. We had to retain entire graphs of objects from the WebGLRenderingContextBase. Don't want to make the same mistake again. Very complex in multiple browser impls. In Chrome we had to wait for a brand new primitive to show up in the garbage collector (Member<T>). Multithreading issues in WebKit w.r.t. the garbage collector.
  • Discussion.
  • CW: resolved to "getCompilationInfo".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants