Image struct can now dole out buckets in a thread-safe way.

This avoids the whole-image locking that was being done before,
which was causing a lot of thread contention.

src/image.rs

@@ -4,20 +4,29 @@ use std::io;
 use std::io::Write;
 use std::path::Path;
 use std::fs::File;
+use std::marker::PhantomData;
+use std::sync::Mutex;
+use std::cell::{RefCell, UnsafeCell};
+use std::mem;
+use std::cmp;
 
 use color::{XYZ, xyz_to_rec709e};
 
-#[derive(Debug, Clone)]
+#[derive(Debug)]
 pub struct Image {
-    data: Vec<XYZ>,
+    data: UnsafeCell<Vec<XYZ>>,
     res: (usize, usize),
+    checked_out_blocks: Mutex<RefCell<Vec<((u32, u32), (u32, u32))>>>, // (min, max)
 }
 
+unsafe impl Sync for Image {}
+
 impl Image {
     pub fn new(width: usize, height: usize) -> Image {
         Image {
-            data: vec![XYZ::new(0.0, 0.0, 0.0); width * height],
+            data: UnsafeCell::new(vec![XYZ::new(0.0, 0.0, 0.0); width * height]),
             res: (width, height),
+            checked_out_blocks: Mutex::new(RefCell::new(Vec::new())),
         }
     }
 
@@ -29,21 +38,54 @@ impl Image {
         self.res.1
     }
 
-    pub fn get(&self, x: usize, y: usize) -> XYZ {
+    pub fn get(&mut self, x: usize, y: usize) -> XYZ {
         assert!(x < self.res.0);
         assert!(y < self.res.1);
 
-        self.data[self.res.0 * y + x]
+        let data: &Vec<XYZ> = unsafe { mem::transmute(self.data.get()) };
+        data[self.res.0 * y + x]
     }
 
     pub fn set(&mut self, x: usize, y: usize, value: XYZ) {
         assert!(x < self.res.0);
         assert!(y < self.res.1);
 
-        self.data[self.res.0 * y + x] = value;
+        let data: &mut Vec<XYZ> = unsafe { mem::transmute(self.data.get()) };
+        data[self.res.0 * y + x] = value;
     }
 
-    pub fn write_ascii_ppm(&self, path: &Path) -> io::Result<()> {
+    pub fn get_bucket<'a>(&'a self, min: (u32, u32), max: (u32, u32)) -> Bucket<'a> {
+        let tmp = self.checked_out_blocks.lock().unwrap();
+        let mut bucket_list = tmp.borrow_mut();
+
+        // Make sure this won't overlap with any already checked out buckets
+        for bucket in bucket_list.iter() {
+            // Calculate the intersection between the buckets
+            let inter_min = (cmp::max(min.0, (bucket.0).0), cmp::max(min.1, (bucket.0).1));
+            let inter_max = (cmp::min(max.0, (bucket.1).0), cmp::min(max.1, (bucket.1).1));
+
+            // If it's not degenerate and not zero-sized, there's overlap, so
+            // panic.
+            if inter_min.0 < inter_max.0 && inter_min.1 < inter_max.1 {
+                panic!("Attempted to check out a bucket with pixels that are already checked out.");
+            }
+        }
+
+        // Clip bucket to image
+        let max = (cmp::min(max.0, self.res.0 as u32), cmp::min(max.1, self.res.1 as u32));
+
+        // Push bucket onto list
+        bucket_list.push((min, max));
+
+        Bucket {
+            min: min,
+            max: max,
+            img: unsafe { mem::transmute(self as *const Image) },
+            _phantom: PhantomData,
+        }
+    }
+
+    pub fn write_ascii_ppm(&mut self, path: &Path) -> io::Result<()> {
         // Open file.
         let mut f = io::BufWriter::new(try!(File::create(path)));
 
@@ -63,7 +105,7 @@ impl Image {
         Ok(())
     }
 
-    pub fn write_binary_ppm(&self, path: &Path) -> io::Result<()> {
+    pub fn write_binary_ppm(&mut self, path: &Path) -> io::Result<()> {
         // Open file.
         let mut f = io::BufWriter::new(try!(File::create(path)));
 
@@ -84,6 +126,51 @@ impl Image {
     }
 }
 
+#[derive(Debug)]
+pub struct Bucket<'a> {
+    min: (u32, u32),
+    max: (u32, u32),
+    img: *mut Image,
+    _phantom: PhantomData<&'a Image>,
+}
+
+impl<'a> Bucket<'a> {
+    pub fn get(&mut self, x: u32, y: u32) -> XYZ {
+        assert!(x >= self.min.0 && x < self.max.0);
+        assert!(y >= self.min.1 && y < self.max.1);
+
+        let img: &mut Image = unsafe { mem::transmute(self.img) };
+        let data: &Vec<XYZ> = unsafe { mem::transmute(img.data.get()) };
+
+        data[img.res.0 * y as usize + x as usize]
+    }
+
+    pub fn set(&mut self, x: u32, y: u32, value: XYZ) {
+        assert!(x >= self.min.0 && x < self.max.0);
+        assert!(y >= self.min.1 && y < self.max.1);
+
+        let img: &mut Image = unsafe { mem::transmute(self.img) };
+        let data: &mut Vec<XYZ> = unsafe { mem::transmute(img.data.get()) };
+
+        data[img.res.0 * y as usize + x as usize] = value;
+    }
+}
+
+impl<'a> Drop for Bucket<'a> {
+    fn drop(&mut self) {
+        let img: &mut Image = unsafe { mem::transmute(self.img) };
+        let tmp = img.checked_out_blocks.lock().unwrap();
+        let mut bucket_list = tmp.borrow_mut();
+
+        // Find matching bucket and remove it
+        let i = bucket_list.iter().position(|bucket| {
+            (bucket.0).0 == self.min.0 && (bucket.0).1 == self.min.1 &&
+            (bucket.1).0 == self.max.0 && (bucket.1).1 == self.max.1
+        });
+        bucket_list.swap_remove(i.unwrap());
+    }
+}
+
 fn srgb_gamma(n: f32) -> f32 {
     if n < 0.0031308 {
         n * 12.92

src/renderer.rs

@@ -3,8 +3,7 @@
 use std::path::Path;
 use std::cmp::min;
 use std::iter::Iterator;
-use std::sync::{Mutex, RwLock};
+use std::sync::RwLock;
-use std::cell::RefCell;
 use scoped_threadpool::Pool;
 use crossbeam::sync::MsQueue;
 
@@ -32,13 +31,8 @@ impl Renderer {
     pub fn render(&self, thread_count: u32) {
         let mut tpool = Pool::new(thread_count);
 
-        let image = Mutex::new(RefCell::new(Image::new(self.resolution.0, self.resolution.1)));
+        let mut image = Image::new(self.resolution.0, self.resolution.1);
-        let (img_width, img_height) = {
+        let (img_width, img_height) = (image.width(), image.height());
-            let i = image.lock().unwrap();
-            let w = i.borrow().width();
-            let h = i.borrow().height();
-            (w, h)
-        };
 
         let all_jobs_queued = RwLock::new(false);
 
@@ -55,6 +49,7 @@ impl Renderer {
         // Set up job queue
         let job_queue = MsQueue::new();
 
+
         // Render
         tpool.scoped(|scope| {
             // Spawn worker tasks
@@ -134,13 +129,15 @@ impl Renderer {
                         }
 
                         // Calculate color based on ray hits
-                        let img = img.lock().unwrap();
+                        {
-                        let mut img = img.borrow_mut();
+                            let min = (bucket.x, bucket.y);
-                        for path in paths.iter() {
+                            let max = (bucket.x + bucket.w, bucket.y + bucket.h);
-                            let mut col =
+                            let mut img_bucket = img.get_bucket(min, max);
-                                img.get(path.pixel_co.0 as usize, path.pixel_co.1 as usize);
+                            for path in paths.iter() {
-                            col += XYZ::from_spectral_sample(&path.color) / self.spp as f32;
+                                let mut col = img_bucket.get(path.pixel_co.0, path.pixel_co.1);
-                            img.set(path.pixel_co.0 as usize, path.pixel_co.1 as usize, col);
+                                col += XYZ::from_spectral_sample(&path.color) / self.spp as f32;
+                                img_bucket.set(path.pixel_co.0, path.pixel_co.1, col);
+                            }
                         }
                     }
                 });
@@ -172,10 +169,7 @@ impl Renderer {
 
 
         // Write rendered image to disk
-        {
+        let _ = image.write_binary_ppm(Path::new(&self.output_file));
-            let img = &image.lock().unwrap();
-            let _ = img.borrow().write_binary_ppm(Path::new(&self.output_file));
-        }
     }
 }