Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf(profiling): don't unnecessarily copy sample values #920

Merged
merged 1 commit into from
Mar 12, 2025
Merged

perf(profiling): don't unnecessarily copy sample values #920

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions profiling-ffi/src/profiles.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -382,7 +382,7 @@ impl<'a> TryFrom<Sample<'a>> for api::Sample<'a> {
locations.push(location.try_into()?)
}

let values: Vec<i64> = sample.values.into_slice().to_vec();
let values = sample.values.into_slice();

let mut labels: Vec<api::Label> = Vec::with_capacity(sample.labels.len());
for label in sample.labels.as_slice().iter() {
Expand All @@ -397,11 +397,11 @@ impl<'a> TryFrom<Sample<'a>> for api::Sample<'a> {
}
}

impl From<Sample<'_>> for api::StringIdSample {
fn from(sample: Sample<'_>) -> Self {
impl<'a> From<Sample<'a>> 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(),
}
}
Expand Down
2 changes: 1 addition & 1 deletion profiling-replayer/src/replayer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
},
));
Expand Down
2 changes: 1 addition & 1 deletion profiling/examples/profiles.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ fn main() {
..Default::default()
},
],
values: vec![1, 10000],
values: &[1, 10000],
labels: vec![],
};

Expand Down
8 changes: 4 additions & 4 deletions profiling/src/api.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<i64>,
pub values: &'a [i64],

/// label includes additional context for this sample. It can include
/// things like a thread id, allocation size, etc
Expand All @@ -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<StringIdLocation>,
pub values: Vec<i64>,
pub values: &'a [i64],
pub labels: Vec<StringIdLabel>,
}

Expand Down Expand Up @@ -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);
Expand Down
58 changes: 29 additions & 29 deletions profiling/src/internal/observation/observations.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ impl Observations {
&mut self,
sample: Sample,
timestamp: Option<Timestamp>,
values: Vec<i64>,
values: &[i64],
) -> anyhow::Result<()> {
anyhow::ensure!(
self.inner.is_some(),
Expand Down Expand Up @@ -102,7 +102,7 @@ impl AggregatedObservations {
}
}

fn add(&mut self, sample: Sample, values: Vec<i64>) -> 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",
Expand All @@ -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);
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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]
Expand All @@ -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]
Expand All @@ -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]
Expand All @@ -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]
Expand All @@ -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]
Expand All @@ -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]
Expand All @@ -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)| {
Expand Down Expand Up @@ -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);
Expand All @@ -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());
}
}

Expand Down
2 changes: 1 addition & 1 deletion profiling/src/internal/observation/timestamped_observations.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ impl TimestampedObservations {
}
}

pub fn add(&mut self, sample: Sample, ts: Timestamp, values: Vec<i64>) -> 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
Expand Down
29 changes: 13 additions & 16 deletions profiling/src/internal/observation/trimmed_observation.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<i64>, 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
Expand Down Expand Up @@ -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 {
Expand All @@ -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 {
Expand All @@ -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 {
Expand All @@ -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 {
Expand All @@ -193,19 +190,19 @@ mod tests {
bolero::check!().with_type::<Vec<i64>>().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);
}
Expand All @@ -227,9 +224,9 @@ mod tests {
.with_type::<(Vec<i64>, Vec<Operation>)>()
.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) };

Expand Down
2 changes: 1 addition & 1 deletion profiling/src/internal/profile/fuzz_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
}
}
Expand Down
Loading
Loading