Skip to content

Commit

Permalink
Merge #2241
Browse files Browse the repository at this point in the history 
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>
  • Loading branch information
@bors @kvark
bors[bot] and kvark committed Jul 17, 2018
2 parents 3129d5c + 48ed4f7 commit 23a1c0e
Show file tree
Hide file tree
Showing 8 changed files with 74 additions and 82 deletions.
3 changes: 3 additions & 0 deletions Makefile
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ EXCLUDES:=
FEATURES_EXTRA:=mint serialize
FEATURES_HAL:=
FEATURES_HAL2:=
CHECK_SPECIAL:=-p gfx-backend-vulkan

SDL2_DEST=$(HOME)/deps
SDL2_CONFIG=$(SDL2_DEST)/usr/bin/sdl2-config
Expand Down Expand Up @@ -32,6 +33,7 @@ else
ifeq ($(UNAME_S),Darwin)
EXCLUDES+= --exclude gfx-backend-vulkan
FEATURES_HAL=metal
CHECK_SPECIAL=-p gfx-backend-metal --features antidote
endif
endif

Expand All @@ -46,6 +48,7 @@ help:
check:
@echo "Note: excluding \`warden\` here, since it depends on serialization"
cargo check --all $(EXCLUDES) --exclude gfx-warden
#cargo check $(CHECK_SPECIAL) # TODO: Condva API is different between antidote and parking_lot
cd examples && cargo check --features "gl"
cd examples && cargo check --features "$(FEATURES_HAL)"
cd examples && cargo check --features "$(FEATURES_HAL2)"
Expand Down
2 changes: 2 additions & 0 deletions src/backend/metal/Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,3 +32,5 @@ core-foundation = "0.6"
core-graphics = "0.14"
smallvec = "0.6"
spirv_cross = "0.9"
antidote = { version = "1.0", optional = true }
parking_lot = "0.6"
48 changes: 20 additions & 28 deletions src/backend/metal/src/command.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,12 @@ use {
};
use {conversions as conv, native, soft, window};
use internal::{BlitVertex, Channel, ClearKey, ClearVertex, ServicePipes};
use lock::Mutex;

use std::borrow::Borrow;
use std::cell::RefCell;
use std::ops::{Deref, Range};
use std::sync::{Arc, Mutex};
use std::sync::Arc;
use std::{iter, mem, slice, time};

use hal::{buffer, command as com, error, memory, pool, pso};
Expand Down Expand Up @@ -103,11 +104,11 @@ impl QueueInner {
let _pool = AutoreleasePool::new();
// note: we deliberately don't hold the Mutex lock while waiting,
// since the completion handlers need to access it.
let (cmd_buf, token) = queue.lock().unwrap().spawn();
let (cmd_buf, token) = queue.lock().spawn();
cmd_buf.set_label("empty");
cmd_buf.commit();
cmd_buf.wait_until_completed();
queue.lock().unwrap().release(token);
queue.lock().release(token);
}
}

Expand Down Expand Up @@ -891,7 +892,7 @@ impl CommandBufferInner {
pub(crate) fn reset(&mut self, shared: &Shared) {
if let Some(CommandSink::Immediate { token, mut encoder_state, .. }) = self.sink.take() {
encoder_state.end();
shared.queue.lock().unwrap().release(token);
shared.queue.lock().release(token);
}
self.retained_buffers.clear();
self.retained_textures.clear();
Expand Down Expand Up @@ -1276,7 +1277,7 @@ impl CommandQueue {
if let Some(ref system) = sem.system {
system.wait(!0);
}
if let Some(swap_image) = sem.image_ready.lock().unwrap().take() {
if let Some(swap_image) = sem.image_ready.lock().take() {
let start = time::Instant::now();
let count = swap_image.wait_until_ready();
if let Some(ref mut counters) = self.perf_counters {
Expand All @@ -1301,7 +1302,7 @@ impl RawCommandQueue<Backend> for CommandQueue {

self.wait(submit.wait_semaphores.iter().map(|&(s, _)| s));

let queue = self.shared.queue.lock().unwrap();
let queue = self.shared.queue.lock();
let (mut num_immediate, mut num_deferred) = (0, 0);

for buffer in submit.cmd_buffers {
Expand Down Expand Up @@ -1361,7 +1362,7 @@ impl RawCommandQueue<Backend> for CommandQueue {
let block = ConcreteBlock::new(move |_cb: *mut ()| -> () {
// release the fence
if let Some(ref f) = moved_fence {
*f.mutex.lock().unwrap() = true;
*f.mutex.lock() = true;
f.condvar.notify_all();
}
// signal the semaphores
Expand Down Expand Up @@ -1393,7 +1394,7 @@ impl RawCommandQueue<Backend> for CommandQueue {
{
self.wait(wait_semaphores);

let queue = self.shared.queue.lock().unwrap();
let queue = self.shared.queue.lock();
let command_buffer = queue.raw.new_command_buffer();
command_buffer.set_label("present");
record_empty(command_buffer);
Expand Down Expand Up @@ -1536,7 +1537,7 @@ impl CommandBuffer {
let mut inner = self.inner.borrow_mut();
let mut pre = inner.sink().pre_render();
if !pre.is_void() {
let mut pipes = self.shared.service_pipes.lock().unwrap();
let mut pipes = self.shared.service_pipes.lock();
if let Some(com) = self.state.sync_depth_stencil(&mut *pipes, &self.shared.device) {
pre.issue(com)
}
Expand All @@ -1549,7 +1550,7 @@ impl com::RawCommandBuffer<Backend> for CommandBuffer {
self.reset(false);
//TODO: Implement secondary command buffers
let sink = if flags.contains(com::CommandBufferFlags::ONE_TIME_SUBMIT) {
let (cmd_buffer, token) = self.shared.queue.lock().unwrap().spawn();
let (cmd_buffer, token) = self.shared.queue.lock().spawn();
CommandSink::Immediate {
cmd_buffer,
token,
Expand Down Expand Up @@ -1601,9 +1602,7 @@ impl com::RawCommandBuffer<Backend> for CommandBuffer {
R: RangeArg<buffer::Offset>,
{
let mut inner = self.inner.borrow_mut();
let pipes = self.shared.service_pipes
.lock()
.unwrap();
let pipes = self.shared.service_pipes.lock();
let pso = pipes.get_fill_buffer();

let start = *range.start().unwrap_or(&0);
Expand Down Expand Up @@ -1669,7 +1668,6 @@ impl com::RawCommandBuffer<Backend> for CommandBuffer {
) {
let src = self.shared.device
.lock()
.unwrap()
.new_buffer_with_data(
data.as_ptr() as _,
data.len() as _,
Expand Down Expand Up @@ -1908,9 +1906,7 @@ impl com::RawCommandBuffer<Backend> for CommandBuffer {
let mut inner = self.inner.borrow_mut();

// issue a PSO+color switch and a draw for each requested clear
let mut pipes = self.shared.service_pipes
.lock()
.unwrap();
let mut pipes = self.shared.service_pipes.lock();

for clear in clears {
let mut key = ClearKey {
Expand Down Expand Up @@ -2182,9 +2178,7 @@ impl com::RawCommandBuffer<Backend> for CommandBuffer {
// Note: we don't bother to restore any render states here, since we are currently
// outside of a render pass, and the state will be reset automatically once
// we enter the next pass.
let mut pipes = self.shared.service_pipes
.lock()
.unwrap();
let mut pipes = self.shared.service_pipes.lock();
let ServicePipes {
ref library,
ref sampler_states,
Expand Down Expand Up @@ -2468,7 +2462,7 @@ impl com::RawCommandBuffer<Backend> for CommandBuffer {

// we are going to modify the RP descriptor here, so
// locking to avoid data races.
let descriptor = framebuffer.descriptor.lock().unwrap();
let descriptor = framebuffer.descriptor.lock();

let mut num_colors = 0;
let mut full_aspects = Aspects::empty();
Expand Down Expand Up @@ -2525,7 +2519,7 @@ impl com::RawCommandBuffer<Backend> for CommandBuffer {
};

self.state.framebuffer_inner = framebuffer.inner.clone();
let mut pipes = self.shared.service_pipes.lock().unwrap();
let mut pipes = self.shared.service_pipes.lock();
let com_ds = if full_aspects.intersects(Aspects::DEPTH | Aspects::STENCIL) {
self.state.sync_depth_stencil(&mut *pipes, &self.shared.device)
} else {
Expand Down Expand Up @@ -2595,7 +2589,7 @@ impl com::RawCommandBuffer<Backend> for CommandBuffer {
pipeline.rasterizer_state.clone(),
));

let mut pipes = self.shared.service_pipes.lock().unwrap();
let mut pipes = self.shared.service_pipes.lock();
pre.issue(self.state.sync_depth_stencil(&mut *pipes, &self.shared.device).unwrap());
if set_stencil_references {
pre.issue(soft::RenderCommand::SetStencilReferenceValues(
Expand Down Expand Up @@ -2648,7 +2642,7 @@ impl com::RawCommandBuffer<Backend> for CommandBuffer {
for (set_index, desc_set) in sets.into_iter().enumerate() {
match *desc_set.borrow() {
native::DescriptorSet::Emulated { ref pool, ref layouts, ref sampler_range, ref texture_range, ref buffer_range } => {
let data = pool.read().unwrap();
let data = pool.read();
let mut sampler_base = sampler_range.start as usize;
let mut texture_base = texture_range.start as usize;
let mut buffer_base = buffer_range.start as usize;
Expand Down Expand Up @@ -2824,7 +2818,7 @@ impl com::RawCommandBuffer<Backend> for CommandBuffer {
}];
match *desc_set.borrow() {
native::DescriptorSet::Emulated { ref pool, ref layouts, ref sampler_range, ref texture_range, ref buffer_range } => {
let data = pool.read().unwrap();
let data = pool.read();
let mut sampler_base = sampler_range.start as usize;
let mut texture_base = texture_range.start as usize;
let mut buffer_base = buffer_range.start as usize;
Expand Down Expand Up @@ -2960,9 +2954,7 @@ impl com::RawCommandBuffer<Backend> for CommandBuffer {
T: IntoIterator,
T::Item: Borrow<com::BufferCopy>,
{
let pipes = self.shared.service_pipes
.lock()
.unwrap();
let pipes = self.shared.service_pipes.lock();
let compute_pipe = pipes.get_copy_buffer();
let wg_size = MTLSize {
width: compute_pipe.thread_execution_width(),
Expand Down
Loading

0 comments on commit 23a1c0e

Please sign in to comment.