Rewrite perf parser to be more robust

This also changes accelerant's perf parsing code to factor in the period
of a sample when summing hit counts.

Author: camelid

perfparser-py/Cargo.lock

@@ -10,3 +10,4 @@ crate-type = ["cdylib"]
 
 [dependencies]
 pyo3 = "0.23.3"
+perfparser = { path = "../perfparser" }

perfparser-py/Cargo.toml

@@ -1,16 +1,17 @@
 use std::cmp;
 use std::collections::HashMap;
-use std::io::{self, BufRead, BufReader};
+use std::io;
 use std::path::Path;
 use std::process::Command;
 
+use perfparser::Parser;
 use pyo3::{pyclass, pymethods};
 
 use crate::LineLoc;
 
 pub fn run_perf_script(data_path: &Path) -> io::Result<Vec<u8>> {
     let output = Command::new("perf")
-        .args(&["script", "-Fip,srcline", "--full-source-path", "-i"])
+        .args(&["script", "-F+srcline", "--full-source-path", "-i"])
         .arg(data_path)
         .output()?;
     if output.status.success() {
@@ -27,14 +28,22 @@ pub fn parse_and_attribute<R: io::Read>(r: R, project_root: &Path) -> io::Result
             .and_then(Path::to_str)
             .map(str::to_owned)
     };
-    let mut lines = BufReader::new(r).lines().filter_map(Result::ok);
+    let parser = Parser::new(r);
     let mut hit_count = HashMap::new();
 
-    loop {
-        match extract_srcline_from_perf_entry(&mut lines, is_srcline_good) {
-            Err(ExtractError::Done) => break,
-            Err(ExtractError::Invalid) => continue,
-            Ok(loc) => *hit_count.entry(loc).or_default() += 1,
+    for event in parser {
+        let lineloc = event
+            .stack
+            .iter()
+            .filter_map(|frame| frame.srcline.as_ref())
+            .find_map(|srcline| {
+                is_srcline_good(Path::new(&srcline.path)).map(|path| LineLoc {
+                    path,
+                    line: srcline.line as u64,
+                })
+            });
+        if let Some(lineloc) = lineloc {
+            *hit_count.entry(lineloc).or_insert(0) += event.period.unwrap_or(1) as u64;
         }
     }
 
@@ -45,43 +54,6 @@ pub fn parse_and_attribute<R: io::Read>(r: R, project_root: &Path) -> io::Result
     })
 }
 
-enum ExtractError {
-    Invalid,
-    Done,
-}
-
-/// Parses a `perf script` entry from the provided iterator,
-/// and returns the first "good" line location from the stack trace,
-/// based on the provided callback.
-///
-/// Meant to be run on output from `perf script -Fip,srcline --full-source-path`.
-fn extract_srcline_from_perf_entry(
-    mut lines: impl Iterator<Item = String>,
-    is_srcline_good: impl Fn(&Path) -> Option<String>,
-) -> Result<LineLoc, ExtractError> {
-    loop {
-        // ip/sym
-        if lines.next().ok_or(ExtractError::Done)?.trim().is_empty() {
-            return Err(ExtractError::Invalid);
-        }
-        // srcline
-        let srcline_raw = lines.next().ok_or(ExtractError::Done)?;
-        let srcline = srcline_raw.trim();
-        if srcline.is_empty() {
-            return Err(ExtractError::Invalid);
-        }
-        let (loc, _) = srcline.split_once(" ").unwrap_or((srcline, ""));
-        if !loc.contains(':') {
-            return Err(ExtractError::Invalid);
-        }
-        let (path, lineno_str) = loc.rsplit_once(":").ok_or(ExtractError::Invalid)?;
-        let line = lineno_str.parse().map_err(|_| ExtractError::Invalid)?;
-        if let Some(path) = is_srcline_good(Path::new(path)) {
-            return Ok(LineLoc { path, line });
-        }
-    }
-}
-
 #[pyclass]
 #[derive(Debug)]
 pub struct AttributedPerf {

perfparser-py/src/perf.rs

@@ -0,0 +1,6 @@
+[package]
+name = "perfparser"
+version = "0.1.0"
+edition = "2024"
+
+[dependencies]

perfparser/Cargo.lock

@@ -0,0 +1,8 @@
+use perfparser::Parser;
+
+fn main() {
+    let parser = Parser::new(std::io::stdin());
+    for event in parser {
+        println!("{:?}", event);
+    }
+}

perfparser/Cargo.toml

@@ -0,0 +1,168 @@
+// TODO: make this more generic rather than specific to accelerant's use case
+
+use std::io::{self, BufRead, BufReader};
+use std::mem;
+
+#[derive(Debug, Clone, Default)]
+pub struct Event {
+    pub period: Option<usize>,
+    pub kind: String,
+    pub stack: Vec<StackFrame>,
+}
+
+#[derive(Debug, Clone, Default)]
+pub struct StackFrame {
+    pub funcname: String,
+    pub srcline: Option<SourceLine>,
+}
+
+#[derive(Debug, Clone)]
+pub struct SourceLine {
+    pub path: String,
+    pub line: usize,
+}
+
+const SPECIAL_UNKNOWN: &str = "[unknown]";
+
+pub struct Parser<R> {
+    src: BufReader<R>,
+    state: ParserState,
+    cur_event: Event,
+}
+
+impl<R: io::Read> Parser<R> {
+    pub fn new(reader: R) -> Self {
+        Self {
+            src: BufReader::new(reader),
+            state: ParserState::Start,
+            cur_event: Event::default(),
+        }
+    }
+
+    fn parse_event_line(&mut self, line: &str) -> Result<(), ()> {
+        let mut chunks = line.trim().split(':').filter(|s| !s.is_empty());
+        let _ = chunks.next();
+        let Some(period_and_kind) = chunks.next() else {
+            return Err(());
+        };
+        let Some((period_str, kind)) = period_and_kind.trim().split_once(' ') else {
+            return Err(());
+        };
+        self.cur_event.period = period_str.parse().ok();
+        self.cur_event.kind = kind.to_owned();
+
+        if let Some(single_stack_line) = chunks.next() {
+            // Combined event/stack line
+            if self.parse_stack_line(single_stack_line).is_err() {
+                self.state = ParserState::AfterEventLine;
+                return Err(());
+            }
+            self.state = ParserState::AfterCombinedLine;
+        } else {
+            self.state = ParserState::AfterEventLine;
+        }
+        Ok(())
+    }
+
+    fn parse_stack_line(&mut self, line: &str) -> Result<(), ()> {
+        let Some((_addr, rest)) = line.trim().split_once(' ') else {
+            return Err(());
+        };
+        let (funcname, module) = rest
+            .rsplit_once(" (")
+            .and_then(|(f, m)| m.strip_suffix(')').map(|m| (f, m)))
+            .unwrap_or((rest, ""));
+        let (funcname, _offset) = funcname.rsplit_once('+').unwrap_or((funcname, ""));
+
+        self.cur_event.stack.push(StackFrame {
+            funcname: funcname.to_owned(),
+            srcline: None,
+        });
+        if funcname != SPECIAL_UNKNOWN || module != SPECIAL_UNKNOWN {
+            self.state = ParserState::AfterStackLine;
+        }
+        Ok(())
+    }
+
+    fn parse_src_line(&mut self, line: &str) -> Result<(), ()> {
+        let line = line.trim();
+        let (srcinfo, _module) = line.rsplit_once(' ').unwrap_or((line, ""));
+        let Some((path, lineno_str)) = srcinfo.rsplit_once(':') else {
+            self.state = ParserState::AfterSrcLine;
+            return Ok(());
+        };
+        let Ok(lineno) = lineno_str.parse::<usize>() else {
+            return Err(());
+        };
+        if let Some(last_frame) = self.cur_event.stack.last_mut() {
+            last_frame.srcline = Some(SourceLine {
+                path: path.to_owned(),
+                line: lineno,
+            });
+        }
+        self.state = ParserState::AfterSrcLine;
+        Ok(())
+    }
+}
+
+impl<R: io::Read> Iterator for Parser<R> {
+    type Item = Event;
+
+    fn next(&mut self) -> Option<Self::Item> {
+        let mut line = String::new();
+        loop {
+            line.clear();
+            let bytes_read = self.src.read_line(&mut line).ok()?;
+            if bytes_read == 0 {
+                // EOF
+                if self.state != ParserState::Start {
+                    let event = mem::take(&mut self.cur_event);
+                    self.state = ParserState::Start;
+                    return Some(event);
+                } else {
+                    return None;
+                }
+            }
+
+            let line = line.trim();
+            if line.is_empty() {
+                self.state = ParserState::Start;
+                return Some(mem::take(&mut self.cur_event));
+            }
+
+            let result = match self.state {
+                ParserState::Start => self.parse_event_line(line),
+                ParserState::AfterEventLine => self.parse_stack_line(line),
+                ParserState::AfterCombinedLine => {
+                    maybe_handle_weird_line(line, self.parse_src_line(line));
+                    self.state = ParserState::Start;
+                    return Some(mem::take(&mut self.cur_event));
+                }
+                ParserState::AfterStackLine => self.parse_src_line(line),
+                ParserState::AfterSrcLine => self.parse_stack_line(line),
+            };
+            maybe_handle_weird_line(line, result);
+        }
+    }
+}
+
+fn maybe_handle_weird_line(line: &str, result: Result<(), ()>) {
+    if result.is_err() {
+        // FIXME: use logging infrastructure instead
+        eprintln!("Weird line: {}", line);
+    }
+}
+
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+enum ParserState {
+    /// Ready to parse a new event.
+    Start,
+    /// After parsing the first line of the event.
+    AfterEventLine,
+    /// After parsing a combined event/stack line.
+    AfterCombinedLine,
+    /// After parsing a stack line.
+    AfterStackLine,
+    /// After parsing a stack srcline line.
+    AfterSrcLine,
+}