Skip to content

Conversation

@Kangz
Copy link
Collaborator

@Kangz Kangz commented Aug 23, 2021

All objects are refcounted except for encoders that can have simpler lifetime semantics. +CC @pythonesque

Superseeds #15

@Kangz Kangz requested review from austinEng, kainino0x and kvark August 23, 2021 14:06
typedef void (*WGPUProcCommandEncoderPushDebugGroup)(WGPUCommandEncoder commandEncoder, char const * groupLabel);
typedef void (*WGPUProcCommandEncoderResolveQuerySet)(WGPUCommandEncoder commandEncoder, WGPUQuerySet querySet, uint32_t firstQuery, uint32_t queryCount, WGPUBuffer destination, uint64_t destinationOffset);
typedef void (*WGPUProcCommandEncoderWriteTimestamp)(WGPUCommandEncoder commandEncoder, WGPUQuerySet querySet, uint32_t queryIndex);
typedef void (*WGPUProcCommandEncoderFree)(WGPUCommandEncoder commandEncoder);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we use the same naming suffix for the meaning of release/free for all objects?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically using Release for all objects, just that sometimes objects don't have Reference?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah exactly

Copy link
Collaborator

@kainino0x kainino0x Aug 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since these objects are special anyway, can't we just say encoder/bundle get freed by Finish, and pass encoders get freed by EndPass?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still should have a way to delete it (without errors) if you choose to never call Finish(). Finish() results in errors if you encoded something invalid.

@grovesNL
Copy link
Member

What would you think about exposing the Release/Free functions now and consider exposing the Reference functions later?

At least anecdotally I don't think we've been asked for Reference functions in wgpu-native yet (for existing applications/language bindings) but Release/Drop does exist. Shared refcounts could provide them with some performance benefits if they're currently doing their own reference counting anyway. On the other hand, if most people tend to wrap this into some standard reference counting mechanism (e.g. shared_ptr, Rc, GC destructors, etc.) anyway, then they probably wouldn't use the Reference functions (instead holding onto a single reference internally and calling Release once).

@Kangz
Copy link
Collaborator Author

Kangz commented Aug 23, 2021

There's been quite some discussion in #15 and the wgpu Matrix channel and I think we arrived to an agreement that encoders shouldn't be refcounted, but the rest of the objects could (so I think they should). wgpu-rs has refcounting for most of them already anyways.

The refcount on the device, texture and buffer has been quite useful when using this API in Chromium already.

@grovesNL
Copy link
Member

For what it's worth, I think the API is reasonable and I don't feel too strongly about it either way. I'm just not sure that a lot of projects will use the Reference functions because generally these types will be wrapped in some way (e.g. into a larger struct that is refcounted, or language-specific bindings that have some idiomatic way to do refcounting), especially because shared_ptr and Arc offer a slightly richer API already. If that's the case, my (small) concerns are:

  • The API surface increases a bit even though the reference functions aren't commonly used
    • Probably not a big deal -- people could ignore Reference functions if they don't want to use them
  • For some types, wgpu-native might need to add unnecessary refcounting internally in the C bindings (e.g. wrapping in an Arc in wgpu-native) just to implement the Reference API
    • This isn't ideal if most people aren't using this API because it could slightly decrease performance
  • In the future we might also want to consider exposing typical refcounting functionality like weak references, the actual counts of active strong/weak references, etc.
    • If additional functionality were exposed, this could either grow the surface API further
    • If not exposed, it could limit the usefulness of Reference, so projects could end up with some WebGPU types using Reference/Release directly with other types using something like shared_ptr+weak_ptr+Release in finalizers when they need weak references

The refcount on the device, texture and buffer has been quite useful when using this API in Chromium already.

Just for my own understanding, would you mind elaborating how it it benefits Chromium? Do you mean it improves performance because of the shared refcount and/or or simplifies things because some level of wrapper (e.g. shared_ptr or similar) isn't necessary?

@kvark
Copy link
Collaborator

kvark commented Aug 23, 2021

There's been quite some discussion in #15 and the wgpu Matrix channel and I think we arrived to an agreement that encoders shouldn't be refcounted, but the rest of the objects could (so I think they should). wgpu-rs has refcounting for most of them already anyways.

This is about to change with @pythonesque work. And that's exactly why we were hesitating in the beginning: we didn't want to draw ourselves in the corner with forced refcounting.
Speaking of this, I recall the agreement about the encoder, but I don't recall the agreement about "rest of the objects could (so I think they should)". How does @pythonesque see the current positions?

@Kangz
Copy link
Collaborator Author

Kangz commented Aug 24, 2021

Just for my own understanding, would you mind elaborating how it it benefits Chromium?

Code in Chromium to deal with swapchain textures and other cross-systems images in the GPU process has complicated lifetime semantics. Having a refcounted object means that each piece of the code can keep a reference to the object to make sure it doesn't disappear unexpectedly. The more general idea is that every large project will have some very nice code, and some amount of spaghetti. Refcounting helps with lifetime semantics in the spaghetti.

@grovesNL
Copy link
Member

Code in Chromium to deal with swapchain textures and other cross-systems images in the GPU process has complicated lifetime semantics.

Thank you for the context! Is there anything that would prevent some kind of refcounting container (e.g. shared_ptr<WGPUTexture>, Arc<WGPUTexture>) from being used in those cases?

@Kangz
Copy link
Collaborator Author

Kangz commented Aug 24, 2021

Nothing prevents using refcounting containers. They would just add unnecessary indirection since these objects are already internally refcounted.

@doug-moen
Copy link

Nothing prevents using refcounting containers. They would just add unnecessary indirection since these objects are already internally refcounted.

When programming in C++, I use boost::intrusive_ptr instead of std::shared_ptr as a smart pointer class when the object already has its own internal reference count. I only need to provide overloaded definitions of intrusive_ptr_add_ref and intrusive_ptr_release to increment and decrement the internal reference counts. This provides the safety of smart pointers without the overhead of adding a second layer of reference counting if I used shared_ptr.

@grovesNL mentioned weak references. While std::shared_ptr implements weak references, it does so at the cost of additional memory and runtime overhead. The boost::intrusive_ptr doesn't support weak references, but it is also more efficient. I don't think that the WebGPU C library should support weak references, because that adds complexity and also imposes a performance penalty on applications that don't need the feature. The alternative is to wrap pointers in std::shared_ptr if you need weak references.

@Kangz
Copy link
Collaborator Author

Kangz commented Nov 18, 2021

Folding into the updated version of #15

@Kangz Kangz closed this Nov 18, 2021
@Kangz Kangz deleted the lifetime branch November 18, 2021 15:01
@kainino0x kainino0x added the lifetimes Lifetimes of object and memory allocations label May 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lifetimes Lifetimes of object and memory allocations

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants