From 48c55c49842910d8c3e59a4b9c1509da52d6dda9 Mon Sep 17 00:00:00 2001
From: Simonas Kazlauskas <git@kazlauskas.me>
Date: Sun, 19 Jul 2020 01:52:32 +0300
Subject: [PATCH] Add named "additional data" support

This allows representing KV pairs of data in e.g. chrome profiles more
faithfully.
---
 analyzeme/src/event.rs          | 109 +++++++++++++++++++++-----------
 analyzeme/src/lib.rs            |   2 +-
 analyzeme/src/testing_common.rs |  11 ++--
 crox/src/main.rs                |   6 +-
 measureme/src/event_id.rs       |  14 ++--
 mmview/src/main.rs              |   8 ++-
 6 files changed, 100 insertions(+), 50 deletions(-)

diff --git a/analyzeme/src/event.rs b/analyzeme/src/event.rs
index 2c548c0..0c0567b 100644
--- a/analyzeme/src/event.rs
+++ b/analyzeme/src/event.rs
@@ -1,13 +1,28 @@
 use crate::timestamp::Timestamp;
-use memchr::memchr;
 use std::borrow::Cow;
 use std::time::Duration;
 
+#[derive(Clone, Eq, PartialEq, Hash, Debug)]
+pub struct Argument<'a> {
+    pub name: Option<Cow<'a, str>>,
+    pub value: Cow<'a, str>,
+}
+
+impl<'a> Argument<'a> {
+    pub fn new(value: &'a str) -> Self {
+        Self { name: None, value: Cow::from(value) }
+    }
+
+    pub fn new_named(name: &'a str, value: &'a str) -> Self {
+        Self { name: Some(Cow::from(name)), value: Cow::from(value) }
+    }
+}
+
 #[derive(Clone, Eq, PartialEq, Hash, Debug)]
 pub struct Event<'a> {
     pub event_kind: Cow<'a, str>,
     pub label: Cow<'a, str>,
-    pub additional_data: Vec<Cow<'a, str>>,
+    pub additional_data: Vec<Argument<'a>>,
     pub timestamp: Timestamp,
     pub thread_id: u32,
 }
@@ -38,7 +53,7 @@ impl<'a> Event<'a> {
         }
     }
 
-    pub(crate) fn parse_event_id(event_id: Cow<'a, str>) -> (Cow<'a, str>, Vec<Cow<'a, str>>) {
+    pub(crate) fn parse_event_id(event_id: Cow<'a, str>) -> (Cow<'a, str>, Vec<Argument<'a>>) {
         let event_id = match event_id {
             Cow::Owned(s) => Cow::Owned(s.into_bytes()),
             Cow::Borrowed(s) => Cow::Borrowed(s.as_bytes()),
@@ -75,52 +90,58 @@ struct Parser<'a> {
     pos: usize,
 }
 
-const SEPARATOR_BYTE: u8 = measureme::event_id::SEPARATOR_BYTE.as_bytes()[0];
+const ARGUMENT_VALUE_TAG_BYTE: u8 = measureme::event_id::ARGUMENT_VALUE_TAG_BYTE.as_bytes()[0];
+const ARGUMENT_NAME_TAG_BYTE: u8 = measureme::event_id::ARGUMENT_NAME_TAG_BYTE.as_bytes()[0];
 
 impl<'a> Parser<'a> {
     fn new(full_text: Cow<'a, [u8]>) -> Parser<'a> {
         Parser { full_text, pos: 0 }
     }
 
-    fn peek(&self) -> u8 {
-        self.full_text[self.pos]
-    }
-
     fn parse_label(&mut self) -> Result<Cow<'a, str>, String> {
         assert!(self.pos == 0);
-        self.parse_separator_terminated_text()
+        let text = self.parse_text()?;
+        if text.is_empty() {
+            return self.err("<label> is empty");
+        } else {
+            Ok(text)
+        }
     }
 
-    fn parse_separator_terminated_text(&mut self) -> Result<Cow<'a, str>, String> {
+    fn parse_text(&mut self) -> Result<Cow<'a, str>, String> {
         let start = self.pos;
-
-        let end = memchr(SEPARATOR_BYTE, &self.full_text[start..])
-            .map(|pos| pos + start)
-            .unwrap_or(self.full_text.len());
-
-        if start == end {
-            return self.err("Zero-length <text>");
-        }
-
-        self.pos = end;
-
-        if self.full_text[start..end].iter().any(u8::is_ascii_control) {
-            return self.err("Found ASCII control character in <text>");
-        }
-
-        Ok(self.substring(start, end))
+        self.pos += self.full_text[start..]
+            .iter()
+            .take_while(|c| !u8::is_ascii_control(c))
+            .count();
+        Ok(self.substring(start, self.pos))
     }
 
-    fn parse_arg(&mut self) -> Result<Cow<'a, str>, String> {
-        if self.peek() != SEPARATOR_BYTE {
-            return self.err(&format!(
-                "Expected '\\x{:x}' char at start of <argument>",
-                SEPARATOR_BYTE
-            ));
+    fn parse_arg(&mut self) -> Result<Argument<'a>, String> {
+        let name = if let Some(&byte) = self.full_text.get(self.pos) {
+            if byte == ARGUMENT_NAME_TAG_BYTE {
+                self.pos += 1;
+                Some(self.parse_text()?)
+            } else {
+                None
+            }
+        } else {
+            None
+        };
+        let value = if let Some(&byte) = self.full_text.get(self.pos) {
+            if byte == ARGUMENT_VALUE_TAG_BYTE {
+                self.pos += 1;
+                Some(self.parse_text()?)
+            } else {
+                None
+            }
+        } else {
+            None
+        };
+        match (name, value) {
+            (name, Some(value)) => Ok(Argument { name, value }),
+            (_, None) => self.err("Unable to parse required <argument_value>"),
         }
-
-        self.pos += 1;
-        self.parse_separator_terminated_text()
     }
 
     fn err<T>(&self, message: &str) -> Result<T, String> {
@@ -161,7 +182,7 @@ mod tests {
         let (label, args) = Event::parse_event_id(Cow::from("foo\x1emy_arg"));
 
         assert_eq!(label, "foo");
-        assert_eq!(args, vec![Cow::from("my_arg")]);
+        assert_eq!(args, vec![Argument::new("my_arg")]);
     }
 
     #[test]
@@ -171,7 +192,21 @@ mod tests {
         assert_eq!(label, "foo");
         assert_eq!(
             args,
-            vec![Cow::from("arg1"), Cow::from("arg2"), Cow::from("arg3")]
+            vec![Argument::new("arg1"), Argument::new("arg2"), Argument::new("arg3")]
+        );
+    }
+
+    #[test]
+    fn parse_event_id_n_named_args() {
+        let (label, args) = Event::parse_event_id(Cow::from("foo\x1darg1\x1eval1\x1darg2\x1eval2"));
+
+        assert_eq!(label, "foo");
+        assert_eq!(
+            args,
+            vec![
+                Argument::new_named("arg1", "val1"),
+                Argument::new_named("arg2", "val2"),
+            ]
         );
     }
 }
diff --git a/analyzeme/src/lib.rs b/analyzeme/src/lib.rs
index f60b827..4055915 100644
--- a/analyzeme/src/lib.rs
+++ b/analyzeme/src/lib.rs
@@ -21,7 +21,7 @@ mod stringtable;
 pub mod testing_common;
 mod timestamp;
 
-pub use crate::event::Event;
+pub use crate::event::{Event, Argument};
 pub use crate::lightweight_event::LightweightEvent;
 pub use crate::profiling_data::{ProfilingData, ProfilingDataBuilder};
 pub use crate::stack_collapse::collapse_stacks;
diff --git a/analyzeme/src/testing_common.rs b/analyzeme/src/testing_common.rs
index 216acfa..10c549a 100644
--- a/analyzeme/src/testing_common.rs
+++ b/analyzeme/src/testing_common.rs
@@ -1,5 +1,5 @@
 use crate::timestamp::Timestamp;
-use crate::{Event, ProfilingData};
+use crate::{Event, Argument, ProfilingData};
 use measureme::{EventId, EventIdBuilder, Profiler, SerializationSink, StringId};
 use rustc_hash::FxHashMap;
 use std::borrow::Cow;
@@ -22,15 +22,16 @@ fn mk_filestem(file_name_stem: &str) -> PathBuf {
 struct ExpectedEvent {
     kind: Cow<'static, str>,
     label: Cow<'static, str>,
-    args: Vec<Cow<'static, str>>,
+    args: Vec<Argument<'static>>,
 }
 
 impl ExpectedEvent {
-    fn new(kind: &'static str, label: &'static str, args: &[&'static str]) -> ExpectedEvent {
+    fn new(kind: &'static str, label: &'static str, args: &[Argument<'static>])
+    -> ExpectedEvent {
         ExpectedEvent {
             kind: Cow::from(kind),
             label: Cow::from(label),
-            args: args.iter().map(|&x| Cow::from(x)).collect(),
+            args: args.to_vec(),
         }
     }
 }
@@ -65,7 +66,7 @@ fn generate_profiling_data<S: SerializationSink>(
     let expected_events_templates = vec![
         ExpectedEvent::new("Generic", "SomeGenericActivity", &[]),
         ExpectedEvent::new("Query", "SomeQuery", &[]),
-        ExpectedEvent::new("QueryWithArg", "AQueryWithArg", &["some_arg"]),
+        ExpectedEvent::new("QueryWithArg", "AQueryWithArg", &[Argument::new("some_arg")]),
     ];
 
     let threads: Vec<_> = (0..num_threads)
diff --git a/crox/src/main.rs b/crox/src/main.rs
index f7ccb55..5ba8917 100644
--- a/crox/src/main.rs
+++ b/crox/src/main.rs
@@ -123,7 +123,11 @@ fn get_args(full_event: &analyzeme::Event) -> Option<FxHashMap<String, String>>
                 .additional_data
                 .iter()
                 .enumerate()
-                .map(|(i, arg)| (format!("arg{}", i).to_string(), arg.to_string()))
+                .map(|(i, arg)| (if let Some(name) = &arg.name {
+                    name.to_string()
+                } else {
+                    format!("arg{}", i).to_string()
+                }, arg.value.to_string()))
                 .collect(),
         )
     } else {
diff --git a/measureme/src/event_id.rs b/measureme/src/event_id.rs
index affbbf6..dad1239 100644
--- a/measureme/src/event_id.rs
+++ b/measureme/src/event_id.rs
@@ -1,11 +1,13 @@
 use crate::{Profiler, SerializationSink, StringComponent, StringId};
 
-/// Event IDs are strings conforming to the following grammar:
+/// Event IDs are strings conforming to the following (eBNF) grammar:
 ///
 /// ```ignore
 ///   <event_id> = <label> {<argument>}
 ///   <label> = <text>
-///   <argument> = '\x1E' <text>
+///   <argument> = ['\x1D' <argument_name>] '\x1E' <argument_value>
+///   <argument_name> = <text>
+///   <argument_value> = <text>
 ///   <text> = regex([^[[:cntrl:]]]+) // Anything but ASCII control characters
 ///  ```
 ///
@@ -13,8 +15,10 @@ use crate::{Profiler, SerializationSink, StringComponent, StringId};
 /// arguments. Future versions my support other optional suffixes (with a tag
 /// other than '\x11' after the '\x1E' separator), such as a "category".
 
-/// The byte used to separate arguments from the label and each other.
-pub const SEPARATOR_BYTE: &str = "\x1E";
+/// The byte used to denote the following text is an argument value.
+pub const ARGUMENT_VALUE_TAG_BYTE: &str = "\x1E";
+/// The byte used to denote the following text is an argument name.
+pub const ARGUMENT_NAME_TAG_BYTE: &str = "\x1D";
 
 /// An `EventId` is a `StringId` with the additional guarantee that the
 /// corresponding string conforms to the event_id grammar.
@@ -73,7 +77,7 @@ impl<'p, S: SerializationSink> EventIdBuilder<'p, S> {
             // Label
             StringComponent::Ref(label),
             // Seperator and start tag for arg
-            StringComponent::Value(SEPARATOR_BYTE),
+            StringComponent::Value(ARGUMENT_VALUE_TAG_BYTE),
             // Arg string id
             StringComponent::Ref(arg),
         ]))
diff --git a/mmview/src/main.rs b/mmview/src/main.rs
index 2df72a3..cfffe96 100644
--- a/mmview/src/main.rs
+++ b/mmview/src/main.rs
@@ -41,7 +41,13 @@ fn system_time_to_micros_since(t: SystemTime, since: SystemTime) -> u128 {
 }
 
 fn print_event(event: &Event<'_>, global_start_time: SystemTime) {
-    let additional_data = event.additional_data.join(",");
+    let additional_data = event.additional_data.iter().map(|arg| {
+        if let Some(name) = &arg.name {
+            format!("{} = {}", name, arg.value)
+        } else {
+            arg.value.to_string()
+        }
+    }).collect::<Vec<_>>().join(", ");
 
     let timestamp = match event.timestamp {
         Timestamp::Instant(t) => {