comment cleanup

src/main.rs

@@ -23,23 +23,26 @@ impl Range {
 
 #[derive(Debug)]
 pub struct Blob {
-    // todo: consider fallible collections to detect OOM on append
-
     // space complexity O(n + m)
     // where n is number of bytes
-    // and m is number of ranges
+    // where m is number of ranges
+    // (m should usually be smaller than n, unless reads are very sparse)
     // -------------------------
-    // Selected VecDeque<Box<[u8;N]>>> for fast push/pop/access
-    // pop_front may re-allocate, but better than vector most of time
-    // also since we use box, we are only copying pointers to the arrays, not each u8
+    // Selected VecDeque<Box<[u8;N]>>> for
+    // fast pop_front and random access, both are O(1)
+    // push_back is usually O(1), but may re-allocate and copy O(n)
+    // also since we use box, we are only copying pointers to the arrays, not each u8_array
     // -------------------------
-    // also tried using LinkedList, but slow reading when ranges were inside nodes
+    // also tried using LinkedList, but slow reading O(n^2) when ranges were inside nodes
     // and indexing into LinkedList nodes doesn't seem to jive well with Rust's ownership model
     // especially when considering multi-threaded access
     // -------------------------
     // Vec<Option<Box<[u8;N]>>> also worth considering, but instead of pop
     // using None slots to maintain indexes, if arrays are likely to be sparse
     // this would probably be preferrable
+    // -------------------------
+    // note: consider fallible collections to detect OOM on append if needed
+    //       this code will panic if we run out of memory
     arrays: VecDeque<Box<[u8; ARRAY_SIZE]>>, // complexity variable n
     dropped: usize,
     ranges: Vec<Range>, // valid, global   // complexity variable m
@@ -64,10 +67,10 @@ impl Blob {
         )
     }
 
-    // appending complexity time, worst: O(n + a + m)
+    // appending complexity time, worst: O(n + a + r)
     // where n is bytes written
-    // and a is arrays added
-    // and m is ranges added, usually 0 or 1, but reallocation could copy (m)
+    // where a is arrays added
+    // where r is ranges added, usually 0 or 1, but reallocation could copy (r)
     fn append(&mut self, input: &[u8]) -> usize {
         if input.is_empty() {
             return 0;
@@ -80,16 +83,18 @@ impl Blob {
             let within = global_offset % ARRAY_SIZE;
             let physical = global_offset / ARRAY_SIZE - self.dropped;
             if physical == self.arrays.len() {
-                // O(1) to push_back
+                // O(1) for single push_back
+                // O(a) for all arrays that are necessary for the write
                 self.arrays.push_back(Box::new([0u8; ARRAY_SIZE]));
             }
-            // write to array - worst: O(n)
+            // write to array - O(n)
             let take = (ARRAY_SIZE - within).min(input.len() - written);
             let array = &mut self.arrays[physical];
             array[within..within + take].copy_from_slice(&input[written..written + take]);
             written += take;
         }
-        // update the ranges vector - O(1)
+        // update the ranges vector - usually O(1)
+        // but could push could re-allocate and copy: O(r)
         let new_range = Range::new(self.end_index, self.end_index + input.len());
         match self.ranges.last_mut() {
             Some(last) if last.end == new_range.start => last.end = new_range.end,
@@ -100,10 +105,10 @@ impl Blob {
         written
     }
 
-    // read time complexity, worst: O(n + m + d)
+    // read time complexity, worst: O(n + r + d)
     // where n is bytes read
-    // and m is ranges added. Usually O(1), but could be O(m) on re-allocation copy
-    // and d is arrays dropped
+    // where r is ranges checked for already-read bytes
+    // where d is arrays dropped
     fn read(&mut self, start: usize, len: usize) -> Result<Vec<u8>, BlobError> {
         if len == 0 {
             return Ok(Vec::new());
@@ -113,6 +118,8 @@ impl Blob {
             return Err(BlobError::InvalidRange);
         }
 
+        // check if range is valid - O(r)
+        // todo: we should be able to early exit here once we've gone too far
         let read_range = Range::new(start, start + len);
         let Some(idx) = self.ranges.iter().position(|r| r.contains(&read_range)) else {
             return Err(BlobError::BytesAlreadyRead);
@@ -128,7 +135,7 @@ impl Blob {
             copied += take;
         }
 
-        // update valid ranges - worst case: O(m), usual case O(1)
+        // update valid ranges - usually: O(1), but could re-allocate O(r)
         let vr = self.ranges[idx];
         let mut remainder = Vec::new();
         if vr.start < read_range.start {
@@ -139,17 +146,17 @@ impl Blob {
         }
         self.ranges.splice(idx..idx + 1, remainder);
 
-        // reclaim arrays below the lowest unread byte
-        // this will keep memory footprint relatively small
-        // at low cost O(1) or worst on re-allocation: O(number_of_arrays)
-        // compared to a Vec<u8> O(n)
-        // could also use a Vec<Option<Box>>, but this would leave None slots
+        // reclaim arrays below the lowest unread byte - O(d)
+        // this will keep memory footprint relatively small unless reads are sparse
+        // using Vec<Option<Box<...>>> would handle sparse data better for more stable addressing,
+        // but it leaves None slots that cannot be reclaimed as effectively
         let min_unread = self
             .ranges
             .first()
             .map(|r| r.start)
             .unwrap_or(self.end_index);
         while self.dropped < min_unread / ARRAY_SIZE {
+            // note: if data is particularly sensitive we should consider zeroing the array on drop
             self.arrays.pop_front();
             self.dropped += 1;
         }
@@ -167,18 +174,35 @@ enum BlobError {
 }
 
 struct BlobManager {
-    // fine if we assume:
-    // * IDs are sequentially generated
-    // * and IDs can be re-used
-    blobs: RwLock<Vec<Option<Arc<Mutex<Blob>>>>>,
+    // blob indexing is left up to the caller, but
+    // vector is fine if we assume IDs are sequentially generated
+    // fast access: O(1)
+    // usually fast push_back: O(1), unless re-allocation: O(number_of_blobs)
+    // space complexity: O(number_of_blobs * blob_size)
+    // ------------------------
     // Also note RwLock (create) starvation might be a concern if caller is
     // append/read heavy due to platform-defined fairness
-    // todo: better approach?
     // RwLock benefits shrink as create traffic becomes heavy
+    // Could address with sharding the collection into n collection shards
+    // and determine which shard to use with id % n
+    // Each shard would have it's own RwLock
+    // ------------------------
+    // If blob creation is arbitrary by ID then
+    // RwLock<HashMap<usize, Arc<Mutex<Blob>>>> would be a better fit.
+    // There wouldn't be a bunch of empty slots so we save on memory
+    // but we would pay a bit in access for hash computation and collision handling
+    // average insert: O(1), but can be long if there are many collisions O(n)
+    // average access: O(1), but can be long if there are many collisions O(n)
+    // ------------------------
+    // If blob IDs are created more arbitrarily
+    // it would probably good to provide an API that quickly pops unused IDs
+    // from a collection (probably VecDeque) so the user doesn't need to poll
+    // the create function on Error
+    // This would make adding blob deletion easier which could get us more
+    // memory space when using big numbers of blobs
+    blobs: RwLock<Vec<Option<Arc<Mutex<Blob>>>>>,
 }
 
-// todo: blob indexing is left up to the caller maybe add ID generator
-//       for cross-thread synchronization or change approach
 impl BlobManager {
     pub fn new() -> Self {
         Self {
@@ -191,7 +215,11 @@ impl BlobManager {
         if let Some(Some(_)) = blobs.get(id) {
             return Err(BlobError::BlobExists);
         } else if id >= blobs.len() {
-            // todo: better strategy possible?
+            // note on resizing tradeoff: we assumed above that IDs are sequentially generated
+            // so it might be better to extend the vector by a larger amount all at once (say 2*id)
+            // if we are more interested in paying that cost upfront.
+            // though moving to the cost upfront would make even less sense if IDs are added arbitrarily.
+            // leaving as is for now
             blobs.resize_with(id + 1, || None);
         }
         blobs[id] = Some(Arc::new(Mutex::new(Blob::new())));
@@ -219,7 +247,8 @@ impl BlobManager {
             }
         };
         let read = blob.lock().unwrap().read(start, len)?; // propagate error to caller
-        // todo: if read clears a blob entirely should the blob be removed?
+        // note: if read clears a blob entirely we maintain the blob for eternal blobs
+        //       though could free up some memory if we delete when we don't need anymore
         Ok(read)
     }
 }