From 3b278b8bd3c7a1b5dd033a900536636b68f3a609 Mon Sep 17 00:00:00 2001 From: Jon Gjengset Date: Tue, 28 May 2019 09:04:18 -0400 Subject: [PATCH 1/4] Hashbrown is now std::HashMap --- Cargo.toml | 1 - src/collapse/dtrace.rs | 3 +-- src/collapse/perf.rs | 3 +-- src/differential/mod.rs | 2 +- 4 files changed, 3 insertions(+), 6 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index c838cf35..bde30000 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -29,7 +29,6 @@ log = "0.4" quick-xml = { version = "0.14", default-features = false } num-format = { version = "0.4", default-features = false } str_stack = "0.1" -hashbrown = "0.2" rand = "0.6.5" structopt = { version = "0.2", optional = true } env_logger = { version = "0.6.0", optional = true } diff --git a/src/collapse/dtrace.rs b/src/collapse/dtrace.rs index 1b2783d0..6d79d246 100644 --- a/src/collapse/dtrace.rs +++ b/src/collapse/dtrace.rs @@ -1,7 +1,6 @@ use super::Collapse; -use hashbrown::HashMap; use log::warn; -use std::collections::VecDeque; +use std::collections::{VecDeque, HashMap}; use std::io; use std::io::prelude::*; diff --git a/src/collapse/perf.rs b/src/collapse/perf.rs index b1817bc0..d083478a 100644 --- a/src/collapse/perf.rs +++ b/src/collapse/perf.rs @@ -1,7 +1,6 @@ use super::Collapse; -use hashbrown::HashMap; use log::warn; -use std::collections::VecDeque; +use std::collections::{VecDeque, HashMap}; use std::io; use std::io::prelude::*; diff --git a/src/differential/mod.rs b/src/differential/mod.rs index b3fa05ab..48598b27 100644 --- a/src/differential/mod.rs +++ b/src/differential/mod.rs @@ -1,4 +1,4 @@ -use hashbrown::HashMap; +use std::collections::HashMap; use log::warn; use std::fs::File; use std::io; From edaff4f3725fd5b7fa19c2654d0f6a86e4f9ac04 Mon Sep 17 00:00:00 2001 From: Jon Gjengset Date: Tue, 28 May 2019 09:06:18 -0400 Subject: [PATCH 2/4] Ensure reliable nameattr ordering Hashbrown has a different ordering from HashMap, even with deterministic fnv hashing, so the tests were failing on beta. This changes nameattr to use an indexmap instead, which has a reliable ordering at all times (insertion order). This is probably a better behavior anyway, because it means that attributes will be added in the order they appear in the nameattr file. --- Cargo.toml | 1 + src/flamegraph/attrs.rs | 11 ++++++----- tests/data/flamegraph/nameattr/nameattr.svg | 14 +++++++------- .../nameattr/nameattr_duplicate_attributes.svg | 6 +++--- 4 files changed, 17 insertions(+), 15 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index bde30000..8ef3d2d2 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -36,6 +36,7 @@ rgb = "0.8.13" lazy_static = "1.3.0" fnv = "1.0.3" itoa = "0.4.3" +indexmap = "1.0" [dev-dependencies] pretty_assertions = "0.6" diff --git a/src/flamegraph/attrs.rs b/src/flamegraph/attrs.rs index ebbf4cb9..863d4bf6 100644 --- a/src/flamegraph/attrs.rs +++ b/src/flamegraph/attrs.rs @@ -1,11 +1,12 @@ -use fnv::FnvHashMap; +use indexmap::map::Entry; use log::warn; -use std::collections::hash_map::Entry; use std::collections::HashMap; use std::fs::File; use std::io::{self, BufRead, BufReader}; use std::path::PathBuf; +type AttrMap = indexmap::IndexMap; + macro_rules! unwrap_or_continue { ($e:expr) => {{ if let Some(x) = $e { @@ -101,7 +102,7 @@ pub(super) struct FrameAttrs { /// If set to None, the title is dynamically generated based on the function name. pub(super) title: Option, - pub(super) attrs: FnvHashMap, + pub(super) attrs: AttrMap, } impl FrameAttrs { @@ -212,7 +213,7 @@ mod test { let r = s.as_bytes(); let mut expected_inner = HashMap::new(); - let foo_attrs: FnvHashMap = convert_args!(hashmap!( + let foo_attrs: AttrMap = convert_args!(hashmap!( "class" => "foo class", "xlink:href" => "foo href", "target" => "foo target", @@ -232,7 +233,7 @@ mod test { }, ); - let bar_attrs: FnvHashMap = convert_args!(hashmap!( + let bar_attrs: AttrMap = convert_args!(hashmap!( "class" => "bar class", "xlink:href" => "bar href", "aextra1" => "foo", diff --git a/tests/data/flamegraph/nameattr/nameattr.svg b/tests/data/flamegraph/nameattr/nameattr.svg index db1587eb..78ebddda 100644 --- a/tests/data/flamegraph/nameattr/nameattr.svg +++ b/tests/data/flamegraph/nameattr/nameattr.svg @@ -43,17 +43,17 @@ var searchcolor = 'rgb(230,0,230)';]]> __libc_start_.. - + foo main - + bar cksum - + bar c.. @@ -98,17 +98,17 @@ var searchcolor = 'rgb(230,0,230)';]]> - + bar cksum - + foo main - + bar cksum @@ -128,7 +128,7 @@ var searchcolor = 'rgb(230,0,230)';]]> noploop - + foo main diff --git a/tests/data/flamegraph/nameattr/nameattr_duplicate_attributes.svg b/tests/data/flamegraph/nameattr/nameattr_duplicate_attributes.svg index e4e89c19..98009ac2 100644 --- a/tests/data/flamegraph/nameattr/nameattr_duplicate_attributes.svg +++ b/tests/data/flamegraph/nameattr/nameattr_duplicate_attributes.svg @@ -43,7 +43,7 @@ var searchcolor = 'rgb(230,0,230)';]]> __libc_start_.. - + foo main @@ -103,7 +103,7 @@ var searchcolor = 'rgb(230,0,230)';]]> cksum - + foo main @@ -128,7 +128,7 @@ var searchcolor = 'rgb(230,0,230)';]]> noploop - + foo main From 06338af20e8942d6694e9c6b574482211cd42689 Mon Sep 17 00:00:00 2001 From: Jon Gjengset Date: Tue, 28 May 2019 09:09:01 -0400 Subject: [PATCH 3/4] Use fnv hashing everywhere for the speedz --- src/collapse/dtrace.rs | 7 ++++--- src/collapse/perf.rs | 7 ++++--- src/differential/mod.rs | 4 ++-- src/flamegraph/attrs.rs | 6 +++--- 4 files changed, 13 insertions(+), 11 deletions(-) diff --git a/src/collapse/dtrace.rs b/src/collapse/dtrace.rs index 6d79d246..2a553ff7 100644 --- a/src/collapse/dtrace.rs +++ b/src/collapse/dtrace.rs @@ -1,6 +1,7 @@ use super::Collapse; +use fnv::FnvHashMap; use log::warn; -use std::collections::{VecDeque, HashMap}; +use std::collections::VecDeque; use std::io; use std::io::prelude::*; @@ -23,7 +24,7 @@ pub struct Folder { stack: VecDeque, /// Number of times each call stack has been seen. - occurrences: HashMap, + occurrences: FnvHashMap, /// Keep track of stack string size while we consume a stack stack_str_size: usize, @@ -114,7 +115,7 @@ impl From for Folder { fn from(opt: Options) -> Self { Self { stack: VecDeque::default(), - occurrences: HashMap::with_capacity(512), + occurrences: FnvHashMap::with_capacity_and_hasher(512, fnv::FnvBuildHasher::default()), cache_inlines: Vec::new(), opt, stack_str_size: 0, diff --git a/src/collapse/perf.rs b/src/collapse/perf.rs index d083478a..a8d73309 100644 --- a/src/collapse/perf.rs +++ b/src/collapse/perf.rs @@ -1,6 +1,7 @@ use super::Collapse; +use fnv::FnvHashMap; use log::warn; -use std::collections::{VecDeque, HashMap}; +use std::collections::VecDeque; use std::io; use std::io::prelude::*; @@ -70,7 +71,7 @@ pub struct Folder { cache_line: Vec, /// Number of times each call stack has been seen. - occurrences: HashMap, + occurrences: FnvHashMap, /// Current comm name. /// @@ -159,7 +160,7 @@ impl From for Folder { skip_stack: false, stack: VecDeque::default(), cache_line: Vec::default(), - occurrences: HashMap::with_capacity(512), + occurrences: FnvHashMap::with_capacity_and_hasher(512, fnv::FnvBuildHasher::default()), pname: String::new(), event_filtering: EventFilterState::None, opt, diff --git a/src/differential/mod.rs b/src/differential/mod.rs index 48598b27..3186a10e 100644 --- a/src/differential/mod.rs +++ b/src/differential/mod.rs @@ -1,4 +1,4 @@ -use std::collections::HashMap; +use fnv::FnvHashMap as HashMap; use log::warn; use std::fs::File; use std::io; @@ -46,7 +46,7 @@ where R2: BufRead, W: Write, { - let mut stack_counts = HashMap::new(); + let mut stack_counts = HashMap::default(); let total1 = parse_stack_counts(opt, &mut stack_counts, before, true)?; let total2 = parse_stack_counts(opt, &mut stack_counts, after, false)?; if opt.normalize && total1 != total2 { diff --git a/src/flamegraph/attrs.rs b/src/flamegraph/attrs.rs index 863d4bf6..b8fb9a6b 100644 --- a/src/flamegraph/attrs.rs +++ b/src/flamegraph/attrs.rs @@ -1,6 +1,6 @@ +use fnv::FnvHashMap; use indexmap::map::Entry; use log::warn; -use std::collections::HashMap; use std::fs::File; use std::io::{self, BufRead, BufReader}; use std::path::PathBuf; @@ -19,7 +19,7 @@ macro_rules! unwrap_or_continue { /// Provides a way to customize the attributes on the SVG elements for a frame. #[derive(PartialEq, Eq, Debug, Default)] -pub struct FuncFrameAttrsMap(HashMap); +pub struct FuncFrameAttrsMap(FnvHashMap); impl FuncFrameAttrsMap { /// Parse frame attributes from a file. @@ -212,7 +212,7 @@ mod test { let s = vec![foo, bar].join("\n"); let r = s.as_bytes(); - let mut expected_inner = HashMap::new(); + let mut expected_inner = FnvHashMap::default(); let foo_attrs: AttrMap = convert_args!(hashmap!( "class" => "foo class", "xlink:href" => "foo href", From 9ae3fd3014ce8432f56f06e24183a93089714bfe Mon Sep 17 00:00:00 2001 From: Jon Gjengset Date: Tue, 28 May 2019 10:41:35 -0400 Subject: [PATCH 4/4] Don't overload HashMap type --- src/differential/mod.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/differential/mod.rs b/src/differential/mod.rs index 3186a10e..68f47f3e 100644 --- a/src/differential/mod.rs +++ b/src/differential/mod.rs @@ -1,4 +1,4 @@ -use fnv::FnvHashMap as HashMap; +use fnv::FnvHashMap; use log::warn; use std::fs::File; use std::io; @@ -46,7 +46,7 @@ where R2: BufRead, W: Write, { - let mut stack_counts = HashMap::default(); + let mut stack_counts = FnvHashMap::default(); let total1 = parse_stack_counts(opt, &mut stack_counts, before, true)?; let total2 = parse_stack_counts(opt, &mut stack_counts, after, false)?; if opt.normalize && total1 != total2 { @@ -82,7 +82,7 @@ where // Populate stack_counts based on lines from the reader and returns the sum of the sample counts. fn parse_stack_counts( opt: Options, - stack_counts: &mut HashMap, + stack_counts: &mut FnvHashMap, mut reader: R, is_first: bool, ) -> io::Result @@ -119,7 +119,7 @@ where // Write three-column lines with the folded stack trace and two value columns, // one for each profile. -fn write_stacks(stack_counts: &HashMap, mut writer: W) -> io::Result<()> +fn write_stacks(stack_counts: &FnvHashMap, mut writer: W) -> io::Result<()> where W: Write, {