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

RFC: Clone-ability Guidelines #2457

Open
kvark opened this issue Oct 29, 2018 · 5 comments
Open

RFC: Clone-ability Guidelines #2457

kvark opened this issue Oct 29, 2018 · 5 comments

Comments

@kvark
Copy link
Member

kvark commented Oct 29, 2018

This is an RFC for #2454, also related to #2452

There has been a lot of discussion about gfx-hal types having move semantics. More often than not, it stands in a way of working with the API. The type system integration that the move semantics provides isn't helping much because of the overall level unsafety we expose. The reason for types to not being Clone at the API level is because it allows the backends to use non-clonable data in the implementation. In practice, dealing with native API often involves some sort of sharing: Vulkan handles are just copyable machine words, D3D12 has ComPtr, and Metal's objects are refcounted internally. At the end of the day, if none of the backends require the move semantics for a particular type, there is one less reason to not have Clone on it. One particularly annoying aspect of the move semantics is that it doesn't play well with Rust's drop(&mut self). Thus, if a structure owns some graphics objects, and it wants to clean up after itself, it needs to resort to some run-time owning tricks like Option or ManuallyDrop.

This aspect of the API affects both gfx-hal and wgpu-rs, and it's very important to get right.

Proposed Guideline

Enable Clone on objects (at the API level) if all the conditions are met:

  1. All the backend implementations can (or already) do Clone
  2. Unique borrowing doesn't make sense or is not used

In order to see how this would be applied, let's split all graphics objects in the following groups:

  • Singletons: instance, physical adapter, device. Not sure about shose
  • Pools: descriptor pool, command pool. These have to be externally-synchronized in Vulkan, which we express with &mut self method. For this reason, I suggest them to keep the move semantics.
  • Resources: texture, buffer, view. These don't have methods and may be needed in multiple places. Perfect example to have Clone on them.
  • Descriptors: descriptor set, descriptor set layout, pipeline layout
  • Commands: command buffer, queue. Clearly needs to keep the move semantics, especially in wgpu-rs where there are encoders that borrow command buffers.
  • Synchronizers: fence, semaphore, event. It looks like these could be Clone as well, although I'm not sure if anybody would benefit from them.

Note: this is a proposal! comments are welcome.

@zakarumych
Copy link

Command buffer is only clonable type and you want to make it nonclonnable and other types clonnable...

@kvark
Copy link
Member Author

kvark commented Nov 1, 2018

Command buffers being clon-eable is a very unfortunate artifact, as I've been complaining about it since the beginning. Why do you want them to be clone-able?

@zakarumych
Copy link

zakarumych commented Nov 1, 2018

Because you want them in at least two places. Near command pool to reuse next time. Near queue to submit when all command buffers for submission are ready. Maybe even store submission objects and resubmit them each frame after recording.

@bbrown683
Copy link
Contributor

bbrown683 commented Nov 6, 2018

Being able to clone synchronizers would allow something like below without using std::iter::repeat_with.

let mut acquire_semaphores = 
        iter::repeat(device.borrow().get_device().create_semaphore().unwrap())
    .take(image_count as _)
    .collect();

I think a lot of other types could benefit from similar initialization.

@zakarumych
Copy link

@bbrown683 Cloning resource gives you new value that represent the very same resource.
So it would be invalid to use cloned semaphore for synchronizing multiple image acquisition.

@kvark kvark pinned this issue Dec 28, 2018
@kvark kvark unpinned this issue Mar 26, 2019
@kvark kvark removed the api: hal label Jun 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants