More perf work

CHANGELOG.md

@@ -4,7 +4,12 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
-## [Unreleased]
+## [0.11.0]
+### Changed
+- Start storing names as Arc<str> to reduce string allocations
+- Reduce md5 computations by taking the name hash from the on-disk hashtable.
+- Reserve capacity to reduce allocations
+
 ### Fixed
 - Fix site record merging by not adding in a duplicate entry.
 

Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "llvm_profparser"
-version = "0.10.0"
+version = "0.11.0"
 authors = ["xd009642 <danielmckenna93@gmail.com>"]
 description = "Parsing and interpretation of llvm coverage profiles and generated data"
 repository = "https://github.com/xd009642/llvm-profparser"

src/bin/profparser.rs

@@ -161,7 +161,7 @@ fn try_parse_weighted(input: &str) -> Result<(u64, String), String> {
     }
 }
 
-fn check_function(name: Option<&String>, pattern: Option<&String>) -> bool {
+fn check_function(name: Option<&str>, pattern: Option<&String>) -> bool {
     match pattern {
         Some(pat) => name.map(|x| x.contains(pat)).unwrap_or(false),
         None => false,
@@ -205,14 +205,16 @@ impl ShowCommand {
         let mut below_cutoff_funcs = 0;
         let topn = self.topn.unwrap_or_default();
         for func in profile.records() {
-            if func.name.is_none() || func.hash.is_none() {
+            let Some(name) = func.name() else {
+                continue;
+            };
+            if func.hash.is_none() {
                 continue;
             }
             if is_ir_instr && func.has_cs_flag() != self.showcs {
                 continue;
             }
-            let show =
-                self.all_functions || check_function(func.name.as_ref(), self.function.as_ref());
+            let show = self.all_functions || check_function(Some(name), self.function.as_ref());
 
             if show && self.text {
                 // TODO text format dump
@@ -226,12 +228,7 @@ impl ShowCommand {
             if func_max < self.value_cutoff {
                 below_cutoff_funcs += 1;
                 if self.only_list_below {
-                    println!(
-                        "  {}: (Max = {} Sum = {})",
-                        func.name.as_ref().unwrap(),
-                        func_max,
-                        func_sum
-                    );
+                    println!("  {}: (Max = {} Sum = {})", name, func_max, func_sum);
                     continue;
                 }
             } else if self.only_list_below {
@@ -243,13 +240,13 @@ impl ShowCommand {
                     if top.count < func_max {
                         hotties.pop();
                         hotties.push(HotFn {
-                            name: func.name.as_ref().unwrap().to_string(),
+                            name: name.to_string(),
                             count: func_max,
                         });
                     }
                 } else {
                     hotties.push(HotFn {
-                        name: func.name.as_ref().unwrap().to_string(),
+                        name: name.to_string(),
                         count: func_max,
                     });
                 }
@@ -259,7 +256,7 @@ impl ShowCommand {
                     println!("Counters:");
                 }
                 shown_funcs += 1;
-                println!("  {}:", func.name.as_ref().unwrap());
+                println!("  {}:", name);
                 println!("    Hash: {:#018x}", func.hash.unwrap());
                 println!("    Counters: {}", func.counts().len());
                 if !is_ir_instr {

src/hash_table.rs

@@ -6,6 +6,7 @@ use nom::{
 };
 use std::borrow::Cow;
 use std::mem::size_of;
+use std::sync::Arc;
 use tracing::debug;
 
 #[derive(Copy, Clone, Debug)]
@@ -15,7 +16,7 @@ struct KeyDataLen {
 }
 
 #[derive(Clone, Debug)]
-pub(crate) struct HashTable(pub IndexMap<(u64, String), InstrProfRecord>);
+pub(crate) struct HashTable(pub IndexMap<(u64, u64, Arc<str>), InstrProfRecord>);
 
 fn read_key_data_len(input: &[u8]) -> ParseResult<'_, KeyDataLen> {
     let (bytes, key_len) = le_u64(input)?;
@@ -179,14 +180,15 @@ impl HashTable {
         );
         let mut remaining = bytes;
         for _i in 0..num_items_in_bucket {
-            let (bytes, _hash) = le_u64(remaining)?;
-            debug!("Hash(?): {}", _hash);
+            let (bytes, name_hash) = le_u64(remaining)?;
+            debug!("Name hash: {}", name_hash);
             let (bytes, lens) = read_key_data_len(bytes)?;
             let (bytes, key) = read_key(bytes, lens.key_len as usize)?;
             debug!("lengths: {:?} and key: {}", lens, key);
             let (bytes, (hash, value)) = read_value(version, bytes, lens.data_len as usize)?;
             debug!("hash: {}, value: {:?}", hash, value);
-            self.0.insert((hash, key.to_string()), value);
+            self.0
+                .insert((hash, name_hash, Arc::from(key.as_ref())), value);
             assert!(num_entries > 0);
             num_entries -= 1;
 

src/instrumentation_profile/indexed_profile.rs

@@ -193,15 +193,13 @@ impl InstrProfReader for IndexedInstrProf {
         input = bytes;
         profile.reserve_records(table.0.len());
         profile.symtab.names.reserve(table.0.len());
-        for ((hash, name), v) in &table.0 {
-            let name = name.to_string();
-            let name_hash = compute_hash(&name);
+        for ((hash, name_hash, name), v) in &table.0 {
             profile
                 .symtab
-                .add_func_name_with_hash(name.clone(), name_hash);
+                .add_func_name_with_hash(name.clone(), *name_hash);
             let record = NamedInstrProfRecord {
-                name: Some(name),
-                name_hash: Some(name_hash),
+                name: Some(name.clone()),
+                name_hash: Some(*name_hash),
                 hash: Some(*hash),
                 record: v.clone(),
             };

src/instrumentation_profile/raw_profile.rs

@@ -15,6 +15,7 @@ use nom::{InputIter, InputLength, Slice};
 use std::convert::TryInto;
 use std::fmt::{Debug, Display};
 use std::mem::size_of;
+use std::sync::Arc;
 use tracing::{debug, error, trace};
 
 #[derive(Debug, Clone, Copy, Eq, PartialEq, Hash, Ord, PartialOrd)]
@@ -392,7 +393,7 @@ where
             let (bytes, _) = take(counters_end)(input)?;
             input = bytes;
             let end_length = input.len() - header.names_len as usize;
-            let mut symtab = Symtab::with_capacity(data_section.len());
+            let mut names_section = Vec::with_capacity(data_section.len());
             while input.len() > end_length {
                 let (new_bytes, names) = parse_string_ref(input)?;
                 debug!(
@@ -403,7 +404,19 @@ where
                 input = new_bytes;
                 for name in names.split(INSTR_PROF_NAME_SEP) {
                     debug!("Symbol name parsed: '{}'", name);
-                    symtab.add_func_name(name.to_string(), Some(header.endianness));
+                    names_section.push(Arc::from(name));
+                }
+            }
+            let mut symtab = Symtab::with_capacity(data_section.len());
+            if names_section.len() == data_section.len() {
+                // Raw profile records carry the name hash as `name_ref`, and LLVM emits the names
+                // section in record order. Reusing that key avoids hashing every function name.
+                for (data, name) in data_section.iter().zip(names_section) {
+                    symtab.add_func_name_with_hash(name, data.name_ref);
+                }
+            } else {
+                for name in names_section {
+                    symtab.add_func_name(name, Some(header.endianness));
                 }
             }
             let padding = get_num_padding_bytes(header.names_len);

src/instrumentation_profile/text_profile.rs

@@ -12,6 +12,7 @@ use nom::multi::*;
 use nom::sequence::*;
 use nom::*;
 use std::io::Read;
+use std::sync::Arc;
 
 const IR_TAG: &[u8] = b"ir";
 const FE_TAG: &[u8] = b"fe";
@@ -234,10 +235,10 @@ impl InstrProfReader for TextInstrProf {
                 counts: counters,
                 data,
             };
-            let name = std::str::from_utf8(name).map(|x| x.to_string()).ok();
+            let name = std::str::from_utf8(name).map(Arc::<str>::from).ok();
             result.push_record(NamedInstrProfRecord {
                 name: name.clone(),
-                name_hash: name.as_ref().map(compute_hash),
+                name_hash: name.as_ref().map(|name| compute_hash(name.as_bytes())),
                 hash: Some(hash),
                 record,
             });
@@ -374,11 +375,11 @@ mod tests {
         assert_eq!(report.get_level(), InstrumentationLevel::FrontEnd);
         assert_eq!(report.records().len(), 1);
         assert_eq!(report.symtab.len(), 1);
-        assert_eq!(report.symtab.names.get(&0).unwrap(), "main");
+        assert_eq!(report.symtab.get(0), Some("main"));
 
         let rec = &report.records()[0];
 
-        assert_eq!(rec.name, Some("main".to_string()));
+        assert_eq!(rec.name(), Some("main"));
         assert_eq!(rec.hash, Some(0));
         assert_eq!(rec.record.counts, vec![100]);
         assert_eq!(rec.record.data, None);

src/instrumentation_profile/types.rs

@@ -3,6 +3,7 @@ use rustc_hash::FxHashMap;
 use std::cmp::Ordering;
 use std::convert::TryInto;
 use std::fmt;
+use std::sync::Arc;
 
 /// ~VARIANT_MASKS_ALL & Header.version is the version number
 pub(crate) const VARIANT_MASKS_ALL: u64 = 0xff00_0000_0000_0000;
@@ -31,7 +32,7 @@ impl ValueKind {
 
 #[derive(Debug, Clone, Default, PartialEq, Eq)]
 pub struct Symtab {
-    pub names: FxHashMap<u64, String>,
+    pub names: FxHashMap<u64, Arc<str>>,
 }
 
 impl Symtab {
@@ -67,28 +68,28 @@ impl Symtab {
     /// require a matching endian hash. However, this doesn't seem to be represented in any of the
     /// llvm test files so is largely a mystery. Computes a little endian hash unless specified
     /// otherwise.
-    pub fn add_func_name(&mut self, name: String, endianness: Option<Endianness>) {
+    pub fn add_func_name(&mut self, name: Arc<str>, endianness: Option<Endianness>) {
         let hash = match endianness {
-            Some(Endianness::Big) => compute_be_hash(&name),
-            _ => compute_hash(&name),
+            Some(Endianness::Big) => compute_be_hash(name.as_bytes()),
+            _ => compute_hash(name.as_bytes()),
         };
         self.names.insert(hash, name);
     }
 
-    pub fn add_func_name_with_hash(&mut self, name: String, hash: u64) {
+    pub fn add_func_name_with_hash(&mut self, name: Arc<str>, hash: u64) {
         self.names.insert(hash, name);
     }
 
     pub fn contains(&self, hash: u64) -> bool {
         self.names.contains_key(&hash)
     }
 
-    pub fn get(&self, hash: u64) -> Option<&String> {
-        self.names.get(&hash)
+    pub fn get(&self, hash: u64) -> Option<&str> {
+        self.names.get(&hash).map(AsRef::as_ref)
     }
 
-    pub fn iter(&self) -> impl Iterator<Item = (&u64, &String)> {
-        self.names.iter()
+    pub fn iter(&self) -> impl Iterator<Item = (&u64, &str)> {
+        self.names.iter().map(|(hash, name)| (hash, name.as_ref()))
     }
 }
 
@@ -117,7 +118,7 @@ pub struct InstrumentationProfile {
     pub(crate) fn_entry_only: bool,
     pub(crate) memory_profiling: bool,
     records: Vec<NamedInstrProfRecord>,
-    record_name_lookup: FxHashMap<String, usize>,
+    record_name_lookup: FxHashMap<Arc<str>, usize>,
     pub symtab: Symtab,
 }
 
@@ -185,8 +186,9 @@ impl InstrumentationProfile {
     }
 
     pub fn push_record(&mut self, record: NamedInstrProfRecord) {
-        if let Some(name) = record.name.clone() {
-            self.record_name_lookup.insert(name, self.records.len());
+        if let Some(name) = record.name.as_ref() {
+            self.record_name_lookup
+                .insert(name.clone(), self.records.len());
         }
         self.records.push(record);
     }
@@ -233,23 +235,15 @@ impl InstrumentationProfile {
             let added = if self.symtab.contains(*hash) {
                 // Find the record and merge things. 0 hashed records should have no counters in the
                 // code and otherwise we'll ignore the change that truncated md5 hashes can collide
-                if let Some(rec) = record
-                    .name
-                    .as_ref()
-                    .and_then(|x| self.find_record_by_name_mut(x))
-                {
+                if let Some(rec) = record.name().and_then(|x| self.find_record_by_name_mut(x)) {
                     rec.record.merge(&record.record);
                     true
                 } else {
                     false
                 }
             } else if let Some(alt_hash) = record.hash {
                 if self.symtab.contains(alt_hash) {
-                    if let Some(rec) = record
-                        .name
-                        .as_ref()
-                        .and_then(|x| self.find_record_by_name_mut(x))
-                    {
+                    if let Some(rec) = record.name().and_then(|x| self.find_record_by_name_mut(x)) {
                         rec.record.merge(&record.record);
                         true
                     } else {
@@ -262,17 +256,15 @@ impl InstrumentationProfile {
                 false
             };
             if !added {
-                self.symtab.names.insert(*hash, record.name_unchecked());
+                self.symtab.names.insert(*hash, record.name_arc_unchecked());
                 self.push_record(record.clone());
             }
         }
     }
 
     /// Gets the instrumentation record for the give function
     pub fn get_record(&self, name: &str) -> Option<&NamedInstrProfRecord> {
-        self.records
-            .iter()
-            .find(|x| x.name.as_deref() == Some(name))
+        self.records.iter().find(|x| x.name() == Some(name))
     }
 
     /// Returns true if there are no instrumentation records associated with the profile
@@ -283,7 +275,7 @@ impl InstrumentationProfile {
 
 #[derive(Clone, Debug, Default, Eq, PartialEq, Hash, Ord, PartialOrd)]
 pub struct NamedInstrProfRecord {
-    pub name: Option<String>,
+    pub name: Option<Arc<str>>,
     pub name_hash: Option<u64>,
     pub hash: Option<u64>,
     pub record: InstrProfRecord,
@@ -317,13 +309,21 @@ impl NamedInstrProfRecord {
         &self.record.counts
     }
 
+    pub fn name(&self) -> Option<&str> {
+        self.name.as_deref()
+    }
+
     pub fn hash_unchecked(&self) -> u64 {
         self.hash.unwrap_or_default()
     }
 
-    pub fn name_unchecked(&self) -> String {
+    pub(crate) fn name_arc_unchecked(&self) -> Arc<str> {
         self.name.clone().unwrap_or_default()
     }
+
+    pub fn name_unchecked(&self) -> String {
+        self.name().unwrap_or_default().to_string()
+    }
 }
 
 #[derive(Clone, Debug, Default, Eq, PartialEq, Hash, Ord, PartialOrd)]

tests/cov.rs

@@ -242,7 +242,7 @@ fn check_mapping_consistency() {
             .unwrap();
         assert!(info.cov_map.contains_key(&fun.header.filenames_ref));
         let sym_name = instr.symtab.get(fun.header.name_hash);
-        assert_eq!(sym_name, record.name.as_ref());
+        assert_eq!(sym_name, record.name());
         // record.name record.hash record.counts() + more
 
         // mapping.mapping_info