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

CommandBuffer based fencing in Metal #2252

Closed
kvark opened this issue Jul 19, 2018 · 0 comments · Fixed by #2254
Closed

CommandBuffer based fencing in Metal #2252

kvark opened this issue Jul 19, 2018 · 0 comments · Fixed by #2254

Comments

@kvark
Copy link
Member

kvark commented Jul 19, 2018

Current fences use a combination of Mutex + Condvar, keeping a hold of a bool signifying the signaled state. The waiting code is a bit non-trivial and incorporates the best practices of Condvar usage.

I was thinking that it might be easier to instead track the last submitted command buffer:

enum FenceInner {
  Idle { signalled: bool },
  Pending(metal::CommandBuffer),
}
type Fence = RefCell<FenceInner>;

Now, resetting a fence just puts it into Idle state. Submitting with a fence makes it remember the last command buffer in flight (with a strong reference, so it's not reused by the queue in the meantime).
Waiting for 0 time is now equivalent of checking the command buffer status for completion. Checking for infinite time is equivalent to waitUntilCompleted.

The only non-obvious part is waiting for an arbitrary amount of time. I was thinking that we may just pick a number (say, 1ms) and sleep on it then check again. I don't expect many apps to be very precise about waiting for a fence. At least, Dota doesn't use any numbers other than 0 or infinity.

The gains from this change are small but visible:

  • no more condvar magic, straightforward antidote use
  • less callbacks being assigned
bors bot added a commit that referenced this issue Jul 20, 2018
2254: Metal deferred command buffer optimizations r=grovesNL a=kvark

Fixes #2252
Fixes #2238

~~I'm not 100% convinced this is a good thing to fix, but had to try it anyway, so might as well file the PR :)
Please take a (critical) look.~~

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


Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>
@bors bors bot closed this as completed in #2254 Jul 20, 2018
@kvark kvark mentioned this issue Jul 20, 2018
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant