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.

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

Optimize save_report_lines a bit #62

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
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
Optimize save_report_lines a bit
This avoids some intermediate allocations in particular for a `flag_map` case, as well as avoiding another allocation.

In other cases its unfortunately not possible to avoid more intermediate allocations.
  • Loading branch information
Swatinem committed Nov 19, 2024
commit 7b80699285399e05b95a2cfa1fcb23a40d3ee96c
43 changes: 20 additions & 23 deletions core/src/parsers/pyreport/utils.rs
Original file line number Diff line number Diff line change
@@ -78,7 +78,7 @@ fn create_model_sets_for_line_session<R: Report, B: ReportBuilder<R>>(
coverage_type: &models::CoverageType,
line_no: i64,
datapoint: Option<&CoverageDatapoint>,
ctx: &mut ParseCtx<R, B>,
ctx: &ParseCtx<R, B>,
) -> LineSessionModels {
let source_file_id = ctx.report_json_files[&ctx.chunk.index];
let (hits, hit_branches, total_branches) = separate_pyreport_coverage(&line_session.coverage);
@@ -184,32 +184,30 @@ fn create_model_sets_for_line_session<R: Report, B: ReportBuilder<R>>(
}
}

fn create_model_sets_for_report_line<R: Report, B: ReportBuilder<R>>(
report_line: &ReportLine,
ctx: &mut ParseCtx<R, B>,
) -> Vec<LineSessionModels> {
fn create_model_sets_for_report_line<'a, R: Report, B: ReportBuilder<R>>(
report_line: &'a ReportLine,
ctx: &'a ParseCtx<R, B>,
) -> impl Iterator<Item = LineSessionModels> + 'a {
// A `ReportLine` is a collection of `LineSession`s, and each `LineSession` has
// a set of models we need to insert for it. Build a list of those sets of
// models.
let mut line_session_models = vec![];
for line_session in &report_line.sessions {
report_line.sessions.iter().map(|session| {
// Datapoints are effectively `LineSession`-scoped, but they don't actually live
// in the `LineSession`. Get the `CoverageDatapoint` for this
// `LineSession` if there is one.
let datapoint = if let Some(Some(datapoints)) = &report_line.datapoints {
datapoints.get(&(line_session.session_id as u32))
datapoints.get(&(session.session_id as u32))
} else {
None
};
line_session_models.push(create_model_sets_for_line_session(
line_session,
create_model_sets_for_line_session(
session,
&report_line.coverage_type,
report_line.line_no,
datapoint,
ctx,
));
}
line_session_models
)
})
}

/// Each [`ReportLine`] from a chunks file is comprised of a number of
@@ -231,12 +229,9 @@ pub fn save_report_lines<R: Report, B: ReportBuilder<R>>(
// assigned as a side-effect of this insertion. That lets us populate the
// `local_sample_id` foreign key on all of the models associated with each
// `CoverageSample`.
ctx.db.report_builder.multi_insert_coverage_sample(
models
.iter_mut()
.map(|LineSessionModels { sample, .. }| sample)
.collect(),
)?;
ctx.db
.report_builder
.multi_insert_coverage_sample(models.iter_mut().map(|m| &mut m.sample))?;

// Populate `local_sample_id` and insert all of the context assocs for each
// `LineSession` (if there are any)
@@ -921,10 +916,11 @@ mod tests {
datapoints: None,
};

let model_sets = create_model_sets_for_report_line(&report_line, parse_ctx);
let model_sets: Vec<_> =
create_model_sets_for_report_line(&report_line, parse_ctx).collect();
assert_eq!(
model_sets,
vec![
&[
LineSessionModels {
sample: models::CoverageSample {
raw_upload_id: 123,
@@ -1018,10 +1014,11 @@ mod tests {
datapoints: Some(Some(datapoints)),
};

let model_sets = create_model_sets_for_report_line(&report_line, parse_ctx);
let model_sets: Vec<_> =
create_model_sets_for_report_line(&report_line, parse_ctx).collect();
assert_eq!(
model_sets,
vec![
&[
LineSessionModels {
sample: models::CoverageSample {
raw_upload_id: 123,
4 changes: 2 additions & 2 deletions core/src/report/mod.rs
Original file line number Diff line number Diff line change
@@ -63,9 +63,9 @@ pub trait ReportBuilder<R: Report> {
/// passed-in models' `local_sample_id` fields are ignored and overwritten
/// with values that are unique among all `CoverageSample`s with the same
/// `raw_upload_id`.
fn multi_insert_coverage_sample(
fn multi_insert_coverage_sample<'a>(
&mut self,
samples: Vec<&mut models::CoverageSample>,
samples: impl ExactSizeIterator<Item = &'a mut models::CoverageSample>,
) -> Result<()>;

/// Create a [`models::BranchesData`] record and return it. The passed-in
6 changes: 4 additions & 2 deletions core/src/report/sqlite/models.rs
Original file line number Diff line number Diff line change
@@ -102,9 +102,11 @@ pub trait Insertable {
Ok(())
}

fn multi_insert<'a, I>(mut models: I, conn: &rusqlite::Connection) -> Result<()>
fn multi_insert<'a>(
mut models: impl ExactSizeIterator<Item = &'a Self>,
conn: &rusqlite::Connection,
) -> Result<()>
where
I: Iterator<Item = &'a Self> + ExactSizeIterator,
Self: 'a,
{
let chunk_size = Self::maximum_chunk_size(conn);
23 changes: 13 additions & 10 deletions core/src/report/sqlite/report_builder.rs
Original file line number Diff line number Diff line change
@@ -95,9 +95,9 @@ impl ReportBuilder<SqliteReport> for SqliteReportBuilder {
self.transaction()?.insert_coverage_sample(sample)
}

fn multi_insert_coverage_sample(
fn multi_insert_coverage_sample<'a>(
&mut self,
samples: Vec<&mut models::CoverageSample>,
samples: impl ExactSizeIterator<Item = &'a mut models::CoverageSample>,
) -> Result<()> {
self.transaction()?.multi_insert_coverage_sample(samples)
}
@@ -237,14 +237,17 @@ impl ReportBuilder<SqliteReport> for SqliteReportBuilderTx<'_> {
Ok(sample)
}

fn multi_insert_coverage_sample(
fn multi_insert_coverage_sample<'a>(
&mut self,
mut samples: Vec<&mut models::CoverageSample>,
samples: impl ExactSizeIterator<Item = &'a mut models::CoverageSample>,
) -> Result<()> {
for sample in &mut samples {
sample.local_sample_id = self.id_sequence.next().unwrap();
}
models::CoverageSample::multi_insert(samples.iter().map(|v| &**v), &self.conn)?;
models::CoverageSample::multi_insert(
samples.map(|sample| {
sample.local_sample_id = self.id_sequence.next().unwrap();
&*sample
}),
&self.conn,
)?;
Ok(())
}

@@ -452,7 +455,7 @@ mod tests {
.insert_raw_upload(Default::default())
.unwrap();

let mut samples: Vec<models::CoverageSample> = vec![
let mut samples = vec![
models::CoverageSample {
source_file_id: file.id,
raw_upload_id: raw_upload.id,
@@ -461,7 +464,7 @@ mod tests {
5
];
report_builder
.multi_insert_coverage_sample(samples.iter_mut().collect())
.multi_insert_coverage_sample(samples.iter_mut())
.unwrap();

let report = report_builder.build().unwrap();
4 changes: 2 additions & 2 deletions core/src/test_utils/test_report.rs
Original file line number Diff line number Diff line change
@@ -93,9 +93,9 @@ impl ReportBuilder<TestReport> for TestReportBuilder {
Ok(sample)
}

fn multi_insert_coverage_sample(
fn multi_insert_coverage_sample<'a>(
&mut self,
samples: Vec<&mut CoverageSample>,
samples: impl ExactSizeIterator<Item = &'a mut CoverageSample>,
) -> error::Result<()> {
self.report
.samples
Loading
Oops, something went wrong.