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

Remove RawCommandBuffer: Clone bound #2247

Closed
kvark opened this issue Jul 18, 2018 · 7 comments
Closed

Remove RawCommandBuffer: Clone bound #2247

kvark opened this issue Jul 18, 2018 · 7 comments

Comments

@kvark
Copy link
Member

kvark commented Jul 18, 2018

This is one of the worst sounding API aspects of HAL at the moment.
It's currently defined as:

pub trait RawCommandBuffer<B: Backend>: Clone + Any + Send + Sync

This was likely needed for secondary command buffers and submissions.
Steps to fix:

  1. remove the Clone bound
  2. fix build errors
  3. update portability
  4. profit!
@zakarumych
Copy link

zakarumych commented Jul 18, 2018

It would require to change CommadBuffer API.
Currently it borrows CommandPool and releases when converted to Submit. After that CommandPool and Submit both get RawCommandPool.
Although I couldn't find in Vulkan spec any restriction for recording only one CommandBuffer from each CommandPool simultaneously.

@kvark
Copy link
Member Author

kvark commented Jul 18, 2018

Yeah, my IIRC + gut feeling is that it only conflicts with the type-safish part of HAL. Between the raw backend API soundness and type-safeishness, I'd chose the former.

@zakarumych
Copy link

zakarumych commented Jul 18, 2018

Then user would have to wrap CommandBuffer into Box and transform to raw pointer to send one copy to submission and left one with CommandPool to reuse later.
I imagine that all types that are handlers in vulkan should have the same bounds one can expect from handler: Copy + Clone + Eq

@kvark
Copy link
Member Author

kvark commented Jul 19, 2018

I imagine that all types that are handlers in vulkan should have the same bounds one can expect from handler: Copy + Clone + Eq

That is totally not the way we are going right now. The approach of having "real" Vulkan handles was extensively analyzed in #2206, and we concluded to continue with the move semantics instead in HAL, for now at least.

@zakarumych
Copy link

While it helps to synchronize access to other objects, move semantics isn't very convenient for CommandBuffer though. That's why it has Clone bound right now.
Ideally you would convert ExecutableCommandBuffer into PendingCommandBuffer forbidding destroying and resetting it but allowing to get Submit object from it (multiple times if simultaneous use allowed). Then it can be converted back to ExecutableCommandBuffer or InvalidCommandBuffer when command execution complete (ensured by user thus unsafe).

@kvark
Copy link
Member Author

kvark commented Aug 3, 2018

I've been fighting with the type system for quite a bit on that, and I came to conclusion that our typed command pool is broken by design. It can't reasonably re-use command buffers until they finish execution on GPU, and it has no means to track it, so it can't function like a pool. The only reason it sort of worked is because it is a LIFO queue, so the next acquired buffer is likely done already...

TL;DR: the command pool needs to either become super more complex and fence the submissions (and track them), or be removed. Opinions?

@zakarumych
Copy link

Not only pool need to track command buffers but also command buffers need to track resources. All of this better to be done by higher level code. Leave hal's pool and buffers unsafe with safety comments

@kvark kvark added this to the HAL 0.1 release milestone Dec 19, 2018
bors bot added a commit that referenced this issue Dec 21, 2018
2514: Remove CommandBuffer: Clone bound r=omni-viral a=kvark

Fixes #2247

PR checklist:
- [x] `make` succeeds (on *nix)
- [x] `make reftests` succeeds
- [x] tested examples with the following backends:
- [ ] `rustfmt` run on changed code

The `CommandBuffer` type and borrow implementation are extremely confusing to me :(

Co-authored-by: Dzmitry Malyshau <[email protected]>
@bors bors bot closed this as completed in #2514 Dec 21, 2018
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

2 participants