perf: read and write runs of consecutive sectors in one go

src/internal/alloc.rs

@@ -3,7 +3,7 @@ use crate::internal::{
 };
 use crate::WriteLeNumber;
 use fnv::FnvHashSet;
-use std::io::{self, Seek, Write};
+use std::io::{self, Read, Seek, Write};
 use std::mem::size_of;
 
 //===========================================================================//
@@ -162,6 +162,20 @@ impl<F> Allocator<F> {
     }
 }
 
+impl<F: Read + Seek> Allocator<F> {
+    /// Reads from `offset_within_sector` bytes into sector `sector_id` and
+    /// on through the sectors after it in the file; see
+    /// `Sectors::read_contiguous`.
+    pub fn read_contiguous(
+        &mut self,
+        sector_id: u32,
+        offset_within_sector: u64,
+        buf: &mut [u8],
+    ) -> io::Result<usize> {
+        self.sectors.read_contiguous(sector_id, offset_within_sector, buf)
+    }
+}
+
 impl<F: Seek> Allocator<F> {
     pub fn seek_within_header(
         &mut self,
@@ -232,6 +246,18 @@ impl<F: Write + Seek> Allocator<F> {
         Ok(new_sector_id)
     }
 
+    /// Writes `buf` starting `offset_within_sector` bytes into sector
+    /// `sector_id` and on through the sectors after it in the file; see
+    /// `Sectors::write_contiguous`.
+    pub fn write_contiguous(
+        &mut self,
+        sector_id: u32,
+        offset_within_sector: u64,
+        buf: &[u8],
+    ) -> io::Result<usize> {
+        self.sectors.write_contiguous(sector_id, offset_within_sector, buf)
+    }
+
     /// Allocates a new entry in the FAT, sets its value to `END_OF_CHAIN`, and
     /// returns the new sector number.
     fn allocate_sector(&mut self, init: SectorInit) -> io::Result<u32> {

src/internal/chain.rs

@@ -51,7 +51,44 @@ impl<'a, F> Chain<'a, F> {
     }
 }
 
+impl<'a, F> Chain<'a, F> {
+    /// How many sectors from `index` on are consecutive in the file (and so
+    /// can be read or written in one go), looking no further than needed
+    /// to cover `wanted` bytes from `offset_within_sector` into the first.
+    fn run_len(
+        &self,
+        index: usize,
+        offset_within_sector: u64,
+        wanted: usize,
+    ) -> usize {
+        let sector_len = self.allocator.sector_len() as u64;
+        let mut run = 1;
+        while index + run < self.sector_ids.len()
+            && self.sector_ids[index + run]
+                == self.sector_ids[index + run - 1] + 1
+            && run as u64 * sector_len - offset_within_sector < wanted as u64
+        {
+            run += 1;
+        }
+        run
+    }
+}
+
 impl<'a, F: Write + Seek> Chain<'a, F> {
+    /// Adds `count` sectors to the end of the chain.
+    fn extend_by(&mut self, count: usize) -> io::Result<()> {
+        for _ in 0..count {
+            let new_sector_id =
+                if let Some(&last_sector_id) = self.sector_ids.last() {
+                    self.allocator.extend_chain(last_sector_id, self.init)?
+                } else {
+                    self.allocator.begin_chain(self.init)?
+                };
+            self.sector_ids.push(new_sector_id);
+        }
+        Ok(())
+    }
+
     /// Resizes the chain to the minimum number of sectors large enough to old
     /// `new_len` bytes, allocating or freeing sectors as needed.
     pub fn set_len(&mut self, new_len: u64) -> io::Result<()> {
@@ -69,16 +106,7 @@ impl<'a, F: Write + Seek> Chain<'a, F> {
             }
             // TODO: init remainder of final sector
         } else {
-            for _ in self.sector_ids.len()..new_num_sectors {
-                let new_sector_id = if let Some(&last_sector_id) =
-                    self.sector_ids.last()
-                {
-                    self.allocator.extend_chain(last_sector_id, self.init)?
-                } else {
-                    self.allocator.begin_chain(self.init)?
-                };
-                self.sector_ids.push(new_sector_id);
-            }
+            self.extend_by(new_num_sectors - self.sector_ids.len())?;
         }
         Ok(())
     }
@@ -123,10 +151,16 @@ impl<'a, F: Read + Seek> Read for Chain<'a, F> {
         debug_assert!(current_sector_index < self.sector_ids.len());
         let current_sector_id = self.sector_ids[current_sector_index];
         let offset_within_sector = self.offset_from_start % sector_len;
-        let mut sector = self
-            .allocator
-            .seek_within_sector(current_sector_id, offset_within_sector)?;
-        let bytes_read = sector.read(&mut buf[0..max_len])?;
+        // Read through as many consecutive sectors as the buffer covers.
+        let run =
+            self.run_len(current_sector_index, offset_within_sector, max_len);
+        let run_len = run as u64 * sector_len - offset_within_sector;
+        let max_len = max_len.min(run_len as usize);
+        let bytes_read = self.allocator.read_contiguous(
+            current_sector_id,
+            offset_within_sector,
+            &mut buf[0..max_len],
+        )?;
         self.offset_from_start += bytes_read as u64;
         debug_assert!(self.offset_from_start <= total_len);
         Ok(bytes_read)
@@ -138,30 +172,35 @@ impl<'a, F: Write + Seek> Write for Chain<'a, F> {
         if buf.is_empty() {
             return Ok(0);
         }
-        let mut total_len = self.len();
+        let total_len = self.len();
+        debug_assert!(self.offset_from_start <= total_len);
         let sector_len = self.allocator.sector_len() as u64;
-        if self.offset_from_start == total_len {
-            let new_sector_id =
-                if let Some(&last_sector_id) = self.sector_ids.last() {
-                    self.allocator.extend_chain(last_sector_id, self.init)?
-                } else {
-                    self.allocator.begin_chain(self.init)?
-                };
-            self.sector_ids.push(new_sector_id);
-            total_len += sector_len;
-            debug_assert_eq!(total_len, self.len());
+        // Make room for the whole buffer at once.
+        let end = self.offset_from_start + buf.len() as u64;
+        if end > total_len {
+            let count = (end - total_len).div_ceil(sector_len) as usize;
+            self.extend_by(count)?;
         }
         let current_sector_index =
             (self.offset_from_start / sector_len) as usize;
         debug_assert!(current_sector_index < self.sector_ids.len());
         let current_sector_id = self.sector_ids[current_sector_index];
         let offset_within_sector = self.offset_from_start % sector_len;
-        let mut sector = self
-            .allocator
-            .seek_within_sector(current_sector_id, offset_within_sector)?;
-        let bytes_written = sector.write(buf)?;
+        // Write through as many consecutive sectors as the buffer covers.
+        let run = self.run_len(
+            current_sector_index,
+            offset_within_sector,
+            buf.len(),
+        );
+        let run_len = run as u64 * sector_len - offset_within_sector;
+        let max_len = buf.len().min(run_len as usize);
+        let bytes_written = self.allocator.write_contiguous(
+            current_sector_id,
+            offset_within_sector,
+            &buf[..max_len],
+        )?;
         self.offset_from_start += bytes_written as u64;
-        debug_assert!(self.offset_from_start <= total_len);
+        debug_assert!(self.offset_from_start <= self.len());
         Ok(bytes_written)
     }
 

src/internal/sector.rs

@@ -43,6 +43,32 @@ impl<F> Sectors<F> {
 }
 
 impl<F: Seek> Sectors<F> {
+    /// Seeks to `offset_within_sector` bytes into sector `sector_id`, having
+    /// checked that the `len` bytes from there stay within the file.
+    fn seek_contiguous(
+        &mut self,
+        sector_id: u32,
+        offset_within_sector: u64,
+        len: usize,
+    ) -> io::Result<()> {
+        let sector_len = self.sector_len() as u64;
+        debug_assert!(offset_within_sector <= sector_len);
+        let end =
+            sector_id as u64 * sector_len + offset_within_sector + len as u64;
+        let last_sector = end.saturating_sub(1) / sector_len;
+        if last_sector >= self.num_sectors as u64 {
+            invalid_data!(
+                "Tried to access sector {}, but sector count is only {}",
+                last_sector,
+                self.num_sectors
+            );
+        }
+        self.inner.seek(SeekFrom::Start(
+            (sector_id + 1) as u64 * sector_len + offset_within_sector,
+        ))?;
+        Ok(())
+    }
+
     pub fn seek_within_header(
         &mut self,
         offset_within_header: u64,
@@ -88,7 +114,39 @@ impl<F: Seek> Sectors<F> {
     }
 }
 
+impl<F: Read + Seek> Sectors<F> {
+    /// Reads into `buf` starting `offset_within_sector` bytes into sector
+    /// `sector_id` and continuing through the sectors that follow it in the
+    /// file, which the caller has established are the next ones of the
+    /// chain being read.  Makes a single read of the underlying file, so
+    /// may read fewer bytes than `buf` holds.
+    pub fn read_contiguous(
+        &mut self,
+        sector_id: u32,
+        offset_within_sector: u64,
+        buf: &mut [u8],
+    ) -> io::Result<usize> {
+        self.seek_contiguous(sector_id, offset_within_sector, buf.len())?;
+        self.inner.read(buf)
+    }
+}
+
 impl<F: Write + Seek> Sectors<F> {
+    /// Writes `buf` starting `offset_within_sector` bytes into sector
+    /// `sector_id` and continuing through the sectors that follow it in the
+    /// file, which the caller has established are the next ones of the
+    /// chain being written.  Makes a single write to the underlying file,
+    /// so may write fewer bytes than `buf` holds.
+    pub fn write_contiguous(
+        &mut self,
+        sector_id: u32,
+        offset_within_sector: u64,
+        buf: &[u8],
+    ) -> io::Result<usize> {
+        self.seek_contiguous(sector_id, offset_within_sector, buf.len())?;
+        self.inner.write(buf)
+    }
+
     /// Creates or resets the specified sector using the given initializer.
     pub fn init_sector(
         &mut self,

tests/contiguous_io.rs

@@ -0,0 +1,100 @@
+use cfb::{CompoundFile, Version};
+use std::io::{Cursor, Read, Seek, SeekFrom, Write};
+
+fn pattern(len: usize, seed: u8) -> Vec<u8> {
+    (0..len).map(|i| (i as u8).wrapping_mul(31).wrapping_add(seed)).collect()
+}
+
+/// A write that spans many sectors lands in one go and reads back the
+/// same, whether the sectors are consecutive in the file or not.
+#[test]
+fn multi_sector_writes_round_trip() {
+    let mut comp = CompoundFile::create_with_version(
+        Version::V3,
+        Cursor::new(Vec::new()),
+    )
+    .unwrap();
+    // 10 KiB streams take 20 sectors each of the 512-byte V3 sectors.
+    let a = pattern(10 * 1024, 1);
+    let b = pattern(10 * 1024 + 300, 2);
+    comp.create_stream("/a").unwrap().write_all(&a).unwrap();
+    comp.create_stream("/b").unwrap().write_all(&b).unwrap();
+    // Freeing /a and writing a bigger stream reuses its sectors, whose IDs
+    // are handed out last-freed-first, so the new chain is not consecutive.
+    comp.remove_stream("/a").unwrap();
+    let c = pattern(15 * 1024 + 17, 3);
+    comp.create_stream("/c").unwrap().write_all(&c).unwrap();
+    // Appending to an existing chain starts inside its last sector.
+    let tail = pattern(3000, 4);
+    {
+        let mut stream = comp.open_stream("/b").unwrap();
+        stream.seek(SeekFrom::End(0)).unwrap();
+        stream.write_all(&tail).unwrap();
+    }
+    let bytes = comp.into_inner().into_inner();
+    assert_eq!(bytes.len() % 512, 0, "file is whole sectors");
+
+    let mut comp = CompoundFile::open_strict(Cursor::new(bytes)).unwrap();
+    let mut read = |path: &str| {
+        let mut data = Vec::new();
+        comp.open_stream(path).unwrap().read_to_end(&mut data).unwrap();
+        data
+    };
+    assert_eq!(read("/c"), c);
+    let mut b_expected = b.clone();
+    b_expected.extend_from_slice(&tail);
+    assert_eq!(read("/b"), b_expected);
+}
+
+/// The FAT itself grows while a long chain is being allocated (a V3 FAT
+/// sector only covers 128 sectors), which splits the chain's sector IDs
+/// around the new FAT sectors.
+#[test]
+fn chains_that_outgrow_a_fat_sector_round_trip() {
+    let mut comp = CompoundFile::create_with_version(
+        Version::V3,
+        Cursor::new(Vec::new()),
+    )
+    .unwrap();
+    let data = pattern(600 * 512 + 100, 5);
+    comp.create_stream("/big").unwrap().write_all(&data).unwrap();
+    let small = pattern(5000, 6);
+    comp.create_stream("/small").unwrap().write_all(&small).unwrap();
+    let bytes = comp.into_inner().into_inner();
+    let mut comp = CompoundFile::open_strict(Cursor::new(bytes)).unwrap();
+    let mut back = Vec::new();
+    comp.open_stream("/big").unwrap().read_to_end(&mut back).unwrap();
+    assert_eq!(back, data);
+    back.clear();
+    comp.open_stream("/small").unwrap().read_to_end(&mut back).unwrap();
+    assert_eq!(back, small);
+}
+
+/// Reads that span consecutive sectors come back in large pieces, and
+/// still respect sector boundaries where the chain is not consecutive.
+#[test]
+fn reads_across_sector_runs() {
+    let mut comp = CompoundFile::create_with_version(
+        Version::V3,
+        Cursor::new(Vec::new()),
+    )
+    .unwrap();
+    let a = pattern(2048, 7);
+    comp.create_stream("/a").unwrap().write_all(&a).unwrap();
+    comp.create_stream("/b").unwrap().write_all(&[1; 512]).unwrap();
+    {
+        let mut stream = comp.open_stream("/a").unwrap();
+        stream.seek(SeekFrom::End(0)).unwrap();
+        stream.write_all(&pattern(1024, 8)).unwrap();
+    }
+    let mut expected = a.clone();
+    expected.extend_from_slice(&pattern(1024, 8));
+    let mut stream = comp.open_stream("/a").unwrap();
+    let mut back = vec![0; expected.len()];
+    stream.read_exact(&mut back).unwrap();
+    assert_eq!(back, expected);
+    stream.seek(SeekFrom::Start(700)).unwrap();
+    let mut piece = vec![0; 2000];
+    stream.read_exact(&mut piece).unwrap();
+    assert_eq!(piece, &expected[700..2700]);
+}