From 8bdcfb725ea0a4f7665fb95313cd1264e259be67 Mon Sep 17 00:00:00 2001 From: Daniel Schwartz-Narbonne Date: Wed, 12 Mar 2025 13:02:54 -0400 Subject: [PATCH] [Profiler] Don't unnecessairily copy sample values --- profiling-ffi/src/profiles.rs | 8 +-- profiling-replayer/src/replayer.rs | 2 +- profiling/examples/profiles.rs | 2 +- profiling/src/api.rs | 8 +-- .../src/internal/observation/observations.rs | 58 ++++++++--------- .../observation/timestamped_observations.rs | 2 +- .../observation/trimmed_observation.rs | 29 ++++----- profiling/src/internal/profile/fuzz_tests.rs | 2 +- profiling/src/internal/profile/mod.rs | 64 +++++++++---------- profiling/tests/wordpress.rs | 4 +- 10 files changed, 88 insertions(+), 91 deletions(-) diff --git a/profiling-ffi/src/profiles.rs b/profiling-ffi/src/profiles.rs index dbbe61383..4a97b4da4 100644 --- a/profiling-ffi/src/profiles.rs +++ b/profiling-ffi/src/profiles.rs @@ -382,7 +382,7 @@ impl<'a> TryFrom> for api::Sample<'a> { locations.push(location.try_into()?) } - let values: Vec = sample.values.into_slice().to_vec(); + let values = sample.values.into_slice(); let mut labels: Vec = Vec::with_capacity(sample.labels.len()); for label in sample.labels.as_slice().iter() { @@ -397,11 +397,11 @@ impl<'a> TryFrom> for api::Sample<'a> { } } -impl From> for api::StringIdSample { - fn from(sample: Sample<'_>) -> Self { +impl<'a> From> for api::StringIdSample<'a> { + fn from(sample: Sample<'a>) -> Self { Self { locations: sample.locations.as_slice().iter().map(Into::into).collect(), - values: sample.values.into_slice().to_vec(), + values: sample.values.into_slice(), labels: sample.labels.as_slice().iter().map(Into::into).collect(), } } diff --git a/profiling-replayer/src/replayer.rs b/profiling-replayer/src/replayer.rs index 2d7cada21..878cc35ba 100644 --- a/profiling-replayer/src/replayer.rs +++ b/profiling-replayer/src/replayer.rs @@ -235,7 +235,7 @@ impl<'pprof> Replayer<'pprof> { timestamp, api::Sample { locations: Self::sample_locations(profile_index, sample)?, - values: sample.values.clone(), + values: &(sample.values), labels, }, )); diff --git a/profiling/examples/profiles.rs b/profiling/examples/profiles.rs index 955d46927..036a7bb84 100644 --- a/profiling/examples/profiles.rs +++ b/profiling/examples/profiles.rs @@ -43,7 +43,7 @@ fn main() { ..Default::default() }, ], - values: vec![1, 10000], + values: &[1, 10000], labels: vec![], }; diff --git a/profiling/src/api.rs b/profiling/src/api.rs index 17ea9481b..0fca7b5a7 100644 --- a/profiling/src/api.rs +++ b/profiling/src/api.rs @@ -172,7 +172,7 @@ pub struct Sample<'a> { /// When aggregating multiple samples into a single sample, the /// result has a list of values that is the element-wise sum of the /// lists of the originals. - pub values: Vec, + pub values: &'a [i64], /// label includes additional context for this sample. It can include /// things like a thread id, allocation size, etc @@ -181,9 +181,9 @@ pub struct Sample<'a> { #[derive(Clone, Debug, Eq, PartialEq)] // Same as Sample, but using StringIds -pub struct StringIdSample { +pub struct StringIdSample<'a> { pub locations: Vec, - pub values: Vec, + pub values: &'a [i64], pub labels: Vec, } @@ -425,7 +425,7 @@ impl<'a> TryFrom<&'a pprof::Profile> for Profile<'a> { } let sample = Sample { locations, - values: sample.values.clone(), + values: &sample.values, labels, }; samples.push(sample); diff --git a/profiling/src/internal/observation/observations.rs b/profiling/src/internal/observation/observations.rs index 95aff8ac9..7b3e0ea8a 100644 --- a/profiling/src/internal/observation/observations.rs +++ b/profiling/src/internal/observation/observations.rs @@ -41,7 +41,7 @@ impl Observations { &mut self, sample: Sample, timestamp: Option, - values: Vec, + values: &[i64], ) -> anyhow::Result<()> { anyhow::ensure!( self.inner.is_some(), @@ -102,7 +102,7 @@ impl AggregatedObservations { } } - fn add(&mut self, sample: Sample, values: Vec) -> anyhow::Result<()> { + fn add(&mut self, sample: Sample, values: &[i64]) -> anyhow::Result<()> { anyhow::ensure!( self.obs_len.eq(values.len()), "Observation length mismatch, expected {:?} values, got {} instead", @@ -116,7 +116,7 @@ impl AggregatedObservations { unsafe { v.as_mut_slice(self.obs_len) } .iter_mut() .zip(values) - .for_each(|(a, b)| *a = a.saturating_add(b)); + .for_each(|(a, b)| *a = a.saturating_add(*b)); } else { let trimmed = TrimmedObservation::new(values, self.obs_len); self.data.insert(sample, trimmed); @@ -217,11 +217,11 @@ mod tests { let t1 = Some(Timestamp::new(1).unwrap()); let t2 = Some(Timestamp::new(2).unwrap()); - o.add(s1, None, vec![1, 2, 3]).unwrap(); - o.add(s1, None, vec![4, 5, 6]).unwrap(); - o.add(s2, None, vec![7, 8, 9]).unwrap(); - o.add(s3, t1, vec![10, 11, 12]).unwrap(); - o.add(s2, t2, vec![13, 14, 15]).unwrap(); + o.add(s1, None, &[1, 2, 3]).unwrap(); + o.add(s1, None, &[4, 5, 6]).unwrap(); + o.add(s2, None, &[7, 8, 9]).unwrap(); + o.add(s3, t1, &[10, 11, 12]).unwrap(); + o.add(s2, t2, &[13, 14, 15]).unwrap(); // 2 because they aggregate together assert_eq!(2, o.aggregated_samples_count()); @@ -265,8 +265,8 @@ mod tests { }; let mut o = Observations::new(3); - o.add(s1, None, vec![1, 2, 3]).unwrap(); - o.add(s2, None, vec![4, 5]).unwrap_err(); + o.add(s1, None, &[1, 2, 3]).unwrap(); + o.add(s2, None, &[4, 5]).unwrap_err(); } #[test] @@ -277,8 +277,8 @@ mod tests { }; let mut o = Observations::new(3); - o.add(s1, None, vec![1, 2, 3]).unwrap(); - o.add(s1, None, vec![4, 5]).unwrap_err(); + o.add(s1, None, &[1, 2, 3]).unwrap(); + o.add(s1, None, &[4, 5]).unwrap_err(); } #[test] @@ -296,8 +296,8 @@ mod tests { let mut o = Observations::new(3); let ts = NonZeroI64::new(1).unwrap(); - o.add(s1, Some(ts), vec![1, 2, 3]).unwrap(); - o.add(s2, Some(ts), vec![4, 5]).unwrap_err(); + o.add(s1, Some(ts), &[1, 2, 3]).unwrap(); + o.add(s2, Some(ts), &[4, 5]).unwrap_err(); } #[test] @@ -309,8 +309,8 @@ mod tests { let mut o = Observations::new(3); let ts = NonZeroI64::new(1).unwrap(); - o.add(s1, Some(ts), vec![1, 2, 3]).unwrap(); - o.add(s1, Some(ts), vec![4, 5]).unwrap_err(); + o.add(s1, Some(ts), &[1, 2, 3]).unwrap(); + o.add(s1, Some(ts), &[4, 5]).unwrap_err(); } #[test] @@ -328,8 +328,8 @@ mod tests { let mut o = Observations::new(3); let ts = NonZeroI64::new(1).unwrap(); - o.add(s1, None, vec![1, 2, 3]).unwrap(); - o.add(s2, Some(ts), vec![4, 5]).unwrap_err(); + o.add(s1, None, &[1, 2, 3]).unwrap(); + o.add(s2, Some(ts), &[4, 5]).unwrap_err(); } #[test] @@ -342,9 +342,9 @@ mod tests { let mut o = Observations::new(3); let ts = NonZeroI64::new(1).unwrap(); - o.add(s1, Some(ts), vec![1, 2, 3]).unwrap(); + o.add(s1, Some(ts), &[1, 2, 3]).unwrap(); // This should panic - o.add(s1, None, vec![4, 5]).unwrap(); + o.add(s1, None, &[4, 5]).unwrap(); } #[test] @@ -366,10 +366,10 @@ mod tests { }; let t1 = Some(Timestamp::new(1).unwrap()); - o.add(s1, None, vec![1, 2, 3]).unwrap(); - o.add(s1, None, vec![4, 5, 6]).unwrap(); - o.add(s2, None, vec![7, 8, 9]).unwrap(); - o.add(s3, t1, vec![1, 1, 2]).unwrap(); + o.add(s1, None, &[1, 2, 3]).unwrap(); + o.add(s1, None, &[4, 5, 6]).unwrap(); + o.add(s2, None, &[7, 8, 9]).unwrap(); + o.add(s3, t1, &[1, 1, 2]).unwrap(); let mut count = 0; o.into_iter().for_each(|(k, ts, v)| { @@ -405,10 +405,10 @@ mod tests { for (s, ts, v) in ts_samples { if v.len() == *observations_len { - o.add(*s, Some(*ts), v.clone()).unwrap(); + o.add(*s, Some(*ts), v).unwrap(); ts_samples_added += 1; } else { - assert!(o.add(*s, Some(*ts), v.clone()).is_err()); + assert!(o.add(*s, Some(*ts), v).is_err()); } } assert_eq!(o.timestamped_samples_count(), ts_samples_added); @@ -417,10 +417,10 @@ mod tests { for (s, v) in no_ts_samples { if v.len() == *observations_len { - o.add(*s, None, v.clone()).unwrap(); - aggregated_observations.add(*s, v.clone()).unwrap(); + o.add(*s, None, v).unwrap(); + aggregated_observations.add(*s, v).unwrap(); } else { - assert!(o.add(*s, None, v.clone()).is_err()); + assert!(o.add(*s, None, v).is_err()); } } diff --git a/profiling/src/internal/observation/timestamped_observations.rs b/profiling/src/internal/observation/timestamped_observations.rs index c9be028b9..2288607b2 100644 --- a/profiling/src/internal/observation/timestamped_observations.rs +++ b/profiling/src/internal/observation/timestamped_observations.rs @@ -46,7 +46,7 @@ impl TimestampedObservations { } } - pub fn add(&mut self, sample: Sample, ts: Timestamp, values: Vec) -> anyhow::Result<()> { + pub fn add(&mut self, sample: Sample, ts: Timestamp, values: &[i64]) -> anyhow::Result<()> { // We explicitly turn the data into a stream of bytes, feeding it to the compressor. // @ivoanjo: I played with introducing a structure to serialize it all-at-once, but it seems // to be a lot of boilerplate (of which cost I'm not sure) to basically do the same diff --git a/profiling/src/internal/observation/trimmed_observation.rs b/profiling/src/internal/observation/trimmed_observation.rs index 822b0bbc9..53fb78ef2 100644 --- a/profiling/src/internal/observation/trimmed_observation.rs +++ b/profiling/src/internal/observation/trimmed_observation.rs @@ -65,14 +65,11 @@ impl TrimmedObservation { /// # Safety /// This panics if you attempt to create an Observation with a data vector /// of the wrong length. - pub fn new(v: Vec, len: ObservationLength) -> Self { + pub fn new(v: &[i64], len: ObservationLength) -> Self { len.assert_eq(v.len()); - // First, convert the vector into a boxed slice. - // This shrinks any excess capacity on the vec. - // At this point, the memory is now owned by the box. - // https://doc.rust-lang.org/std/vec/struct.Vec.html#method.into_boxed_slice - let b = v.into_boxed_slice(); + // First, convert the slice into a boxed slice so we can persist it. + let b: Box<[i64]> = Box::from(v); // Get the fat pointer representing the slice out of the box. // At this point, we now own the memory // https://doc.rust-lang.org/std/boxed/struct.Box.html#method.into_raw @@ -129,7 +126,7 @@ mod tests { #[test] fn as_mut_test() { - let v = vec![1, 2]; + let v = &[1, 2]; let o = ObservationLength::new(2); let mut t = TrimmedObservation::new(v, o); unsafe { @@ -142,7 +139,7 @@ mod tests { #[test] fn drop_after_emptying_test() { - let v = vec![1, 2]; + let v = &[1, 2]; let o = ObservationLength::new(2); let t = TrimmedObservation::new(v, o); unsafe { @@ -156,14 +153,14 @@ mod tests { // happens #[cfg_attr(miri, ignore)] fn drop_owned_data_panics_test() { - let v = vec![1, 2]; + let v = &[1, 2]; let o = ObservationLength::new(2); let _t = TrimmedObservation::new(v, o); } #[test] fn into_boxed_slice_test() { - let v = vec![1, 2]; + let v = &[1, 2]; let o = ObservationLength::new(2); let mut t = TrimmedObservation::new(v, o); unsafe { @@ -175,7 +172,7 @@ mod tests { #[test] fn into_vec_test() { - let v = vec![1, 2]; + let v = &[1, 2]; let o = ObservationLength::new(2); let mut t = TrimmedObservation::new(v, o); unsafe { @@ -193,19 +190,19 @@ mod tests { bolero::check!().with_type::>().for_each(|v| { let o = ObservationLength::new(v.len()); { - let t = TrimmedObservation::new(v.clone(), o); + let t = TrimmedObservation::new(v, o); unsafe { t.consume(o); } } { - let t = TrimmedObservation::new(v.clone(), o); + let t = TrimmedObservation::new(v, o); unsafe { assert_eq!(t.into_boxed_slice(o).as_ref(), v.as_slice()); } } { - let t = TrimmedObservation::new(v.clone(), o); + let t = TrimmedObservation::new(v, o); unsafe { assert_eq!(&t.into_vec(o), v); } @@ -227,9 +224,9 @@ mod tests { .with_type::<(Vec, Vec)>() .for_each(|(v, ops)| { let o = ObservationLength::new(v.len()); - let mut t = TrimmedObservation::new(v.clone(), o); + let mut t = TrimmedObservation::new(v, o); - let mut v = v.clone(); + let v = &mut v.clone(); for op in ops { let slice = unsafe { t.as_mut_slice(o) }; diff --git a/profiling/src/internal/profile/fuzz_tests.rs b/profiling/src/internal/profile/fuzz_tests.rs index 5c8c63105..b251913f1 100644 --- a/profiling/src/internal/profile/fuzz_tests.rs +++ b/profiling/src/internal/profile/fuzz_tests.rs @@ -212,7 +212,7 @@ impl<'a> From<&'a Sample> for api::Sample<'a> { fn from(value: &'a Sample) -> Self { Self { locations: value.locations.iter().map(api::Location::from).collect(), - values: value.values.clone(), + values: &value.values, labels: value.labels.iter().map(api::Label::from).collect(), } } diff --git a/profiling/src/internal/profile/mod.rs b/profiling/src/internal/profile/mod.rs index 312622a3e..9cd1f0f68 100644 --- a/profiling/src/internal/profile/mod.rs +++ b/profiling/src/internal/profile/mod.rs @@ -152,7 +152,7 @@ impl Profile { fn add_sample_internal( &mut self, - values: Vec, + values: &[i64], labels: Vec, locations: Vec, timestamp: Option, @@ -803,7 +803,7 @@ mod api_tests { .add_sample( api::Sample { locations, - values: vec![1, 10000], + values: &[1, 10000], labels: vec![], }, None, @@ -852,7 +852,7 @@ mod api_tests { ..Default::default() }]; - let values: Vec = vec![1]; + let values = &[1]; let labels = vec![api::Label { key: "pid", num: 101, @@ -861,13 +861,13 @@ mod api_tests { let main_sample = api::Sample { locations: main_locations, - values: values.clone(), + values, labels: labels.clone(), }; let test_sample = api::Sample { locations: test_locations, - values: values.clone(), + values, labels: labels.clone(), }; @@ -1086,7 +1086,7 @@ mod api_tests { let sample = api::Sample { locations: vec![], - values: vec![1, 10000], + values: &[1, 10000], labels: vec![id_label], }; @@ -1125,13 +1125,13 @@ mod api_tests { let sample1 = api::Sample { locations: vec![], - values: vec![1, 10000], + values: &[1, 10000], labels: vec![id_label, other_label], }; let sample2 = api::Sample { locations: vec![], - values: vec![1, 10000], + values: &[1, 10000], labels: vec![id2_label, other_label], }; @@ -1275,7 +1275,7 @@ mod api_tests { let sample = api::Sample { locations: vec![], - values: vec![10000], + values: &[10000], labels, }; @@ -1300,7 +1300,7 @@ mod api_tests { let sample1 = api::Sample { locations: vec![], - values: vec![1, 10000], + values: &[1, 10000], labels: vec![id_label], }; @@ -1340,7 +1340,7 @@ mod api_tests { let sample1 = api::Sample { locations: vec![], - values: vec![0, 10000, 42], + values: &[0, 10000, 42], labels: vec![], }; @@ -1368,7 +1368,7 @@ mod api_tests { let sample1 = api::Sample { locations: vec![], - values: vec![1, 10000, 42], + values: &[1, 10000, 42], labels: vec![], }; @@ -1396,7 +1396,7 @@ mod api_tests { let sample1 = api::Sample { locations: vec![], - values: vec![1, 16, 29], + values: &[1, 16, 29], labels: vec![], }; @@ -1428,7 +1428,7 @@ mod api_tests { let sample1 = api::Sample { locations: vec![], - values: vec![1, 16, 29], + values: &[1, 16, 29], labels: vec![], }; @@ -1460,7 +1460,7 @@ mod api_tests { let sample1 = api::Sample { locations: vec![], - values: vec![1, 16, 0], + values: &[1, 16, 0], labels: vec![], }; @@ -1492,7 +1492,7 @@ mod api_tests { let sample1 = api::Sample { locations: vec![], - values: vec![1, 16, 0], + values: &[1, 16, 0], labels: vec![], }; @@ -1539,7 +1539,7 @@ mod api_tests { let sample1 = api::Sample { locations: vec![], - values: vec![1, 10000, 21], + values: &[1, 10000, 21], labels: vec![], }; @@ -1562,7 +1562,7 @@ mod api_tests { let sample2 = api::Sample { locations: main_locations, - values: vec![5, 24, 99], + values: &[5, 24, 99], labels: vec![], }; @@ -1596,7 +1596,7 @@ mod api_tests { let sample1 = api::Sample { locations: vec![], - values: vec![1, 10000, 21], + values: &[1, 10000, 21], labels: vec![], }; @@ -1618,7 +1618,7 @@ mod api_tests { let sample2 = api::Sample { locations: main_locations, - values: vec![5, 24, 99], + values: &[5, 24, 99], labels: vec![], }; @@ -1663,7 +1663,7 @@ mod api_tests { let sample1 = api::Sample { locations: vec![], - values: vec![1, 10000, 42], + values: &[1, 10000, 42], labels: vec![id_label], }; @@ -1719,7 +1719,7 @@ mod api_tests { let sample1 = api::Sample { locations: vec![], - values: vec![1, 10000, 42], + values: &[1, 10000, 42], labels: vec![id_label], }; @@ -1754,7 +1754,7 @@ mod api_tests { let sample1 = api::Sample { locations: vec![], - values: vec![1, 10000, 42], + values: &[1, 10000, 42], labels: vec![id_label], }; @@ -1776,7 +1776,7 @@ mod api_tests { let sample2 = api::Sample { locations: main_locations, - values: vec![5, 24, 99], + values: &[5, 24, 99], labels: vec![], }; @@ -1818,7 +1818,7 @@ mod api_tests { let sample1 = api::Sample { locations: vec![], - values: vec![1, 10000, 42], + values: &[1, 10000, 42], labels: vec![id_label, id_no_match_label], }; @@ -1847,7 +1847,7 @@ mod api_tests { let sample2 = api::Sample { locations: main_locations, - values: vec![5, 24, 99], + values: &[5, 24, 99], labels: vec![id_no_match_label, id_label2], }; @@ -1900,7 +1900,7 @@ mod api_tests { let sample1 = api::Sample { locations: vec![], - values: vec![1, 10000, 42], + values: &[1, 10000, 42], labels: vec![id_label], }; @@ -1936,7 +1936,7 @@ mod api_tests { let sample1 = api::Sample { locations: vec![], - values: vec![1, 10000, 42], + values: &[1, 10000, 42], labels: vec![id_label], }; @@ -2208,7 +2208,7 @@ mod api_tests { let sample1 = api::Sample { locations: vec![], - values: vec![1, 10000, 42], + values: &[1, 10000, 42], labels: vec![id_label], }; @@ -2270,13 +2270,13 @@ mod api_tests { let sample1 = api::Sample { locations: vec![], - values: vec![1, 10000], + values: &[1, 10000], labels: vec![id_label], }; let sample2 = api::Sample { locations: vec![], - values: vec![1, 10000], + values: &[1, 10000], labels: vec![id2_label], }; @@ -2369,7 +2369,7 @@ mod api_tests { let sample = api::StringIdSample { locations: vec![location], - values: vec![1], + values: &[1], labels: vec![], }; diff --git a/profiling/tests/wordpress.rs b/profiling/tests/wordpress.rs index b1001c195..7381900e9 100644 --- a/profiling/tests/wordpress.rs +++ b/profiling/tests/wordpress.rs @@ -98,7 +98,7 @@ fn wordpress() { /* 8 */ php_location("