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

[meta] Locking on Metal #2229

Open
4 of 8 tasks
kvark opened this issue Jul 12, 2018 · 3 comments
Open
4 of 8 tasks

[meta] Locking on Metal #2229

kvark opened this issue Jul 12, 2018 · 3 comments

Comments

@kvark
Copy link
Member

kvark commented Jul 12, 2018

This is a meta-issue to track all the locking we do in Metal backend. We need to check if they are appropriate, assume the worst use case, and try to optimize them.

  • service pipes - Optimize locking of ServicePipes #2227 . They are locked quite a bit in various places.
  • pass descriptor. Locked in begin_render_pass, so that we can mutate it and start a real pass. The problem is that an app can do a dozen of render passes from multiple threads on the same framebuffer. And starting one is accidentally the single heaviest operation of the backend...
  • shared queue. It is accessed on submission as well as any time we need to spawn a new command buffer. These shouldn't take long, so I'd not expect a congestion, but it does show up in the profile.
  • shared device. Locked whenever we need to create a resource, pipeline, or a new state. The cost should largely go away as the application warms up, but we need to double-check.
  • command buffer inner - Bring back UnsafeCell for command buffer internals of Metal #2226 . I don't think RefCell has any locking overhead.
  • descriptor pool inner - Metal pools improved #2235
  • fences - CommandBuffer based fencing in Metal #2252
  • swapchain frames. It doesn't look to matter so far. Changing the acquire to be blocking doesn't nearly affect the framerate.
@kvark
Copy link
Member Author

kvark commented Jul 16, 2018

I received a general advice from Nathan Froyd to use https://github.com/Amanieu/parking_lot . We need to check it out and benchmark.

bors bot added a commit that referenced this issue Jul 16, 2018
2235: Metal pools improved r=grovesNL a=kvark

Addresses part of  #2229 by ~~removing the descriptor pool lock entirely - it's meant to be externally synchronized by Vulkan anyway~~ moving the ranges out of the descriptor lock.
Fixes the correctness of a pool reset that's not created with the individual reset flag.

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>
@kvark kvark mentioned this issue Jul 17, 2018
4 tasks
bors bot added a commit that referenced this issue Jul 17, 2018
2241: Metal parking lot r=grovesNL a=kvark

Addresses a part of #2229
~~Based on #2240~~
PR checklist:
- [x] `make` succeeds (on *nix)
- [x] `make reftests` succeeds
- [ ] tested examples with the following backends:
- [ ] `rustfmt` run on changed code

~~Unfortunately, I see no performance advantage from switching to `parking_lot` so far. cc @Amanieu~~

After a series of unsuccessful attempts to hook up a third party library (what could possibly be easier, eh?), I got the performance numbers - we are up from 80 to almost 90, which is more than a 10% bump. Given that `parking_lot` is not a drop-in replacement for `std::sync`, I suggest we ship it by default and provide `antidote` as an optional dependency.

Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>
@kvark
Copy link
Member Author

kvark commented Jul 20, 2018

Major issues are fixed, I believe.

@kvark kvark closed this as completed Jul 20, 2018
@kvark
Copy link
Member Author

kvark commented Jul 30, 2018

Reviving this, since apparently our performance doesn't scale well enough from 2 to 4 cores. Not sure what else, if not locking, would affect it.

@kvark kvark reopened this Jul 30, 2018
@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

1 participant