Metal revised memory types #2076
Conversation
kvark
force-pushed
the
mtl-memory
branch
2 times, most recently
from
4fdbb23 to
76903c0
Compare
| @@ -65,6 +66,7 @@ impl Shared { | |||
| pub(crate) fn new(device: metal::Device) -> Self { | |||
| Shared { | |||
| queue_pool: Mutex::new(command::QueuePool::default()), | |||
| aux_queue: Mutex::new(device.new_command_queue()), | |||
There was a problem hiding this comment.
What happens if we exceed 64 temporary command buffers?
There was a problem hiding this comment.
I don't think this is a problem, since we commit those command buffers right after acquiring them. So in the worst case there is going to be a bit of a delay for GPU to finish.
There was a problem hiding this comment.
I guess it depends on how many active deferred commands buffers we actually expect in real use cases, i.e. if it becomes hundreds in practice it would be unfortunate to hit that delay often IMO
There was a problem hiding this comment.
Hmm. Theoretically, it can become an issue, but it's too early to worry about this right now
| @@ -1103,7 +1103,7 @@ impl CommandBuffer { | |||
| while data.len() < offset + constants.len() { | |||
There was a problem hiding this comment.
Is anything responsible for shrinking oversized data?
There was a problem hiding this comment.
it's getting shrunk in clear_resources, unless you mean the allocation?
There was a problem hiding this comment.
👍 I didn't notice the clear in reset_resources
|
Thanks for taking a look! There appear to be no major concerns. |
2076: Metal revised memory types r=grovesNL a=kvark Fixes #2069 ~~Includes #2070 (please only review the last 2 commits)~~ The PR effectively removes all the complicated memory allocation tracking from the Metal backend, instead relying on mapping the available storage types (with CPU caching policy) more accurately to what can be represented in Vulkan API. With this in, we slice through the whole `memory` section without crashes. The only issue I failed to fix/workaround is filed as #2077 PR checklist: - [x] `make` succeeds (on *nix) - [x] `make reftests` succeeds - [x] tested examples with the following backends: metal Co-authored-by: Dzmitry Malyshau <[email protected]>
| .lock() | ||
| .unwrap() | ||
| .wait_idle(&self.shared.device); | ||
| debug!("waiting for idle"); |
There was a problem hiding this comment.
Any reason not to share the wait_idle implementation across device and command?
Fixes #2069
Includes #2070 (please only review the last 2 commits)The PR effectively removes all the complicated memory allocation tracking from the Metal backend, instead relying on mapping the available storage types (with CPU caching policy) more accurately to what can be represented in Vulkan API.
With this in, we slice through the whole
memorysection without crashes. The only issue I failed to fix/workaround is filed as #2077PR checklist:
makesucceeds (on *nix)make reftestssucceeds