diff --git a/CHANGELOG.md b/CHANGELOG.md index 8700c1d..6010e70 100644 --- a/CHANGELOG.md +++ b/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 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. diff --git a/Cargo.toml b/Cargo.toml index 710b8c2..46839a7 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "llvm_profparser" -version = "0.10.0" +version = "0.11.0" authors = ["xd009642 "] description = "Parsing and interpretation of llvm coverage profiles and generated data" repository = "https://github.com/xd009642/llvm-profparser" diff --git a/src/bin/profparser.rs b/src/bin/profparser.rs index 132c439..cbce710 100644 --- a/src/bin/profparser.rs +++ b/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 { diff --git a/src/hash_table.rs b/src/hash_table.rs index ba5960b..8cfb4e9 100644 --- a/src/hash_table.rs +++ b/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), 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; diff --git a/src/instrumentation_profile/indexed_profile.rs b/src/instrumentation_profile/indexed_profile.rs index 70dcb18..add6b2c 100644 --- a/src/instrumentation_profile/indexed_profile.rs +++ b/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(), }; diff --git a/src/instrumentation_profile/raw_profile.rs b/src/instrumentation_profile/raw_profile.rs index 5ab7d38..2ae86fc 100644 --- a/src/instrumentation_profile/raw_profile.rs +++ b/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); diff --git a/src/instrumentation_profile/text_profile.rs b/src/instrumentation_profile/text_profile.rs index 4e89edf..3481867 100644 --- a/src/instrumentation_profile/text_profile.rs +++ b/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::::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); diff --git a/src/instrumentation_profile/types.rs b/src/instrumentation_profile/types.rs index 2bcd9d3..e3b7567 100644 --- a/src/instrumentation_profile/types.rs +++ b/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, + pub names: FxHashMap>, } impl Symtab { @@ -67,15 +68,15 @@ 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) { + pub fn add_func_name(&mut self, name: Arc, endianness: Option) { 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, hash: u64) { self.names.insert(hash, name); } @@ -83,12 +84,12 @@ impl Symtab { 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 { - self.names.iter() + pub fn iter(&self) -> impl Iterator { + 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, - record_name_lookup: FxHashMap, + record_name_lookup: FxHashMap, 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,11 +235,7 @@ 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 { @@ -245,11 +243,7 @@ impl InstrumentationProfile { } } 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,7 +256,7 @@ 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()); } } @@ -270,9 +264,7 @@ impl InstrumentationProfile { /// 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, + pub name: Option>, pub name_hash: Option, pub hash: Option, 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 { 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)] diff --git a/tests/cov.rs b/tests/cov.rs index 045919a..37f44c1 100644 --- a/tests/cov.rs +++ b/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