Skip digest checks when reading unwind info (#275)

This check was put in place to make sure we aren't loading corrupted
unwind information. This is an unlikely thing to happen but we are
paying a high cost, especially in memory-constrained environments as
unwind information will be evicted and re-read from disk with the
consequent digest checks.

Keeping the digest is very useful for debugging purposes, but checking
it on every read is not worth it performance wise, right now in some of
my tests, 50% of lightswitch's CPU time is spent computing the digest.

Some of the future things that could be done:
- switching to a faster hash algorithm and optimise the buffer sizes
- check a small % of reads

Test Plan
=========
CI

Author: javierhonduco

src/profiler.rs

@@ -1592,6 +1592,7 @@ impl Profiler {
                     &executable_path,
                     executable_id,
                     Some((start_low_address, start_high_address)),
+                    false,
                 )
             }
             Runtime::CLike => {
@@ -1613,6 +1614,7 @@ impl Profiler {
                         &executable_path,
                         executable_id,
                         None,
+                        false,
                     )
                 }
             }

src/unwind_info/manager.rs

@@ -82,8 +82,9 @@ impl UnwindInfoManager {
         executable_path: &Path,
         executable_id: ExecutableId,
         first_frame_override: Option<(u64, u64)>,
+        check_digest: bool,
     ) -> Result<Vec<CompactUnwindRow>, FetchUnwindInfoError> {
-        match self.read_from_cache(executable_id) {
+        match self.read_from_cache(executable_id, check_digest) {
             Ok(unwind_info) => Ok(unwind_info),
             Err(e) => {
                 if matches!(e, FetchUnwindInfoError::NotFound) {
@@ -103,6 +104,7 @@ impl UnwindInfoManager {
     fn read_from_cache(
         &self,
         executable_id: ExecutableId,
+        check_digest: bool,
     ) -> Result<Vec<CompactUnwindRow>, FetchUnwindInfoError> {
         let unwind_info_path = self.path_for(executable_id);
         let file = File::open(unwind_info_path).map_err(|e| {
@@ -116,7 +118,7 @@ impl UnwindInfoManager {
         let mut buffer = BufReader::new(file);
         let mut data = Vec::new();
         buffer.read_to_end(&mut data)?;
-        let reader = Reader::new(&data).map_err(FetchUnwindInfoError::Reader)?;
+        let reader = Reader::new(&data, check_digest).map_err(FetchUnwindInfoError::Reader)?;
 
         Ok(reader.unwind_info()?)
     }
@@ -223,6 +225,7 @@ mod tests {
                 &PathBuf::from("/proc/self/exe"),
                 ExecutableId(0xFABADA),
                 None,
+                true,
             );
             let manager_unwind_info = manager_unwind_info.unwrap();
             assert_eq!(unwind_info, manager_unwind_info);
@@ -240,6 +243,7 @@ mod tests {
             &PathBuf::from("/proc/self/exe"),
             ExecutableId(0xFABADA),
             None,
+            true,
         );
         assert!(manager_unwind_info.is_ok());
         let manager_unwind_info = manager_unwind_info.unwrap();
@@ -258,6 +262,7 @@ mod tests {
             &PathBuf::from("/proc/self/exe"),
             ExecutableId(0xFABADA),
             None,
+            true,
         );
         let manager_unwind_info = manager_unwind_info.unwrap();
         assert_eq!(unwind_info, manager_unwind_info);

src/unwind_info/persist.rs

@@ -145,12 +145,17 @@ pub enum ReaderError {
 pub struct Reader<'a> {
     header: Header,
     data: &'a [u8],
+    check_digest: bool,
 }
 
 impl<'a> Reader<'a> {
-    pub fn new(data: &'a [u8]) -> Result<Self, ReaderError> {
+    pub fn new(data: &'a [u8], check_digest: bool) -> Result<Self, ReaderError> {
         let header = Self::parse_header(data)?;
-        Ok(Reader { header, data })
+        Ok(Reader {
+            header,
+            data,
+            check_digest,
+        })
     }
 
     fn parse_header(data: &[u8]) -> Result<Header, ReaderError> {
@@ -190,22 +195,26 @@ impl<'a> Reader<'a> {
             let unwind_row_data = unwind_info_data
                 .get(step..step + unwind_row_size)
                 .ok_or(ReaderError::OutOfRange)?;
-            context.update(unwind_row_data);
+            if self.check_digest {
+                context.update(unwind_row_data);
+            }
             plain::copy_from_bytes(&mut unwind_row, unwind_row_data)
                 .map_err(|e| ReaderError::Generic(format!("{e:?}")))?;
             unwind_info.push(unwind_row);
         }
 
-        let mut buffer = [0; 8];
-        let _ = context
-            .finish()
-            .as_ref()
-            .read(&mut buffer)
-            .map_err(|e| ReaderError::Generic(e.to_string()));
-        let digest = u64::from_ne_bytes(buffer);
-
-        if self.header.unwind_info_digest != digest {
-            return Err(ReaderError::Digest);
+        if self.check_digest {
+            let mut buffer = [0; 8];
+            let _ = context
+                .finish()
+                .as_ref()
+                .read(&mut buffer)
+                .map_err(|e| ReaderError::Generic(e.to_string()));
+            let digest = u64::from_ne_bytes(buffer);
+
+            if self.header.unwind_info_digest != digest {
+                return Err(ReaderError::Digest);
+            }
         }
 
         Ok(unwind_info)
@@ -226,7 +235,7 @@ mod tests {
         let writer = Writer::new(&path, None);
         assert!(writer.write(&mut buffer).is_ok());
 
-        let reader = Reader::new(&buffer.get_ref()[..]);
+        let reader = Reader::new(&buffer.get_ref()[..], true);
         let unwind_info = reader.unwrap().unwind_info();
         assert!(unwind_info.is_ok());
         let unwind_info = unwind_info.unwrap();
@@ -247,7 +256,7 @@ mod tests {
             .write_all(unsafe { plain::as_bytes(&header) })
             .unwrap();
         assert!(matches!(
-            Reader::new(&buffer),
+            Reader::new(&buffer, true),
             Err(ReaderError::MagicNumber)
         ));
     }
@@ -263,7 +272,10 @@ mod tests {
         buffer
             .write_all(unsafe { plain::as_bytes(&header) })
             .unwrap();
-        assert!(matches!(Reader::new(&buffer), Err(ReaderError::Version)));
+        assert!(matches!(
+            Reader::new(&buffer, true),
+            Err(ReaderError::Version)
+        ));
     }
 
     #[test]
@@ -277,15 +289,22 @@ mod tests {
         buffer.seek(SeekFrom::End(-10)).unwrap();
         buffer.write_all(&[0, 0, 0, 0, 0, 0, 0]).unwrap();
 
-        let reader = Reader::new(&buffer.get_ref()[..]);
+        let reader = Reader::new(&buffer.get_ref()[..], true);
         let unwind_info = reader.unwrap().unwind_info();
         assert!(matches!(unwind_info, Err(ReaderError::Digest)));
+
+        let reader = Reader::new(&buffer.get_ref()[..], false);
+        let unwind_info = reader.unwrap().unwind_info();
+        assert!(unwind_info.is_ok());
     }
 
     #[test]
     fn test_header_too_small() {
         let buffer = Vec::new();
-        assert!(matches!(Reader::new(&buffer), Err(ReaderError::OutOfRange)));
+        assert!(matches!(
+            Reader::new(&buffer, true),
+            Err(ReaderError::OutOfRange)
+        ));
     }
 
     #[test]
@@ -301,7 +320,7 @@ mod tests {
             .write_all(unsafe { plain::as_bytes(&header) })
             .unwrap();
         assert!(matches!(
-            Reader::new(&buffer).unwrap().unwind_info(),
+            Reader::new(&buffer, true).unwrap().unwind_info(),
             Err(ReaderError::OutOfRange)
         ));
     }