From 0fd0bafbf5d078cf7afa7346cc6e41991d0d0287 Mon Sep 17 00:00:00 2001 From: Arpad Borsos Date: Thu, 14 Nov 2024 17:55:03 +0100 Subject: [PATCH] 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. --- core/src/parsers/pyreport/utils.rs | 43 +++++++++++------------- core/src/report/mod.rs | 4 +-- core/src/report/sqlite/report_builder.rs | 23 +++++++------ core/src/test_utils/test_report.rs | 4 +-- 4 files changed, 37 insertions(+), 37 deletions(-) diff --git a/core/src/parsers/pyreport/utils.rs b/core/src/parsers/pyreport/utils.rs index fb6e4bd..82177d2 100644 --- a/core/src/parsers/pyreport/utils.rs +++ b/core/src/parsers/pyreport/utils.rs @@ -78,7 +78,7 @@ fn create_model_sets_for_line_session>( coverage_type: &models::CoverageType, line_no: i64, datapoint: Option<&CoverageDatapoint>, - ctx: &mut ParseCtx, + ctx: &ParseCtx, ) -> 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>( } } -fn create_model_sets_for_report_line>( - report_line: &ReportLine, - ctx: &mut ParseCtx, -) -> Vec { +fn create_model_sets_for_report_line<'a, R: Report, B: ReportBuilder>( + report_line: &'a ReportLine, + ctx: &'a ParseCtx, +) -> impl Iterator + '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>( // 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, diff --git a/core/src/report/mod.rs b/core/src/report/mod.rs index 9ecfce4..8135d2b 100644 --- a/core/src/report/mod.rs +++ b/core/src/report/mod.rs @@ -63,9 +63,9 @@ pub trait ReportBuilder { /// 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 Iterator + ExactSizeIterator, ) -> Result<()>; /// Create a [`models::BranchesData`] record and return it. The passed-in diff --git a/core/src/report/sqlite/report_builder.rs b/core/src/report/sqlite/report_builder.rs index 3ccbf2e..5000534 100644 --- a/core/src/report/sqlite/report_builder.rs +++ b/core/src/report/sqlite/report_builder.rs @@ -95,9 +95,9 @@ impl ReportBuilder 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 Iterator + ExactSizeIterator, ) -> Result<()> { self.transaction()?.multi_insert_coverage_sample(samples) } @@ -237,14 +237,17 @@ impl ReportBuilder 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 Iterator + ExactSizeIterator, ) -> 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 = 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(); diff --git a/core/src/test_utils/test_report.rs b/core/src/test_utils/test_report.rs index 5370b4b..1acb977 100644 --- a/core/src/test_utils/test_report.rs +++ b/core/src/test_utils/test_report.rs @@ -93,9 +93,9 @@ impl ReportBuilder for TestReportBuilder { Ok(sample) } - fn multi_insert_coverage_sample( + fn multi_insert_coverage_sample<'a>( &mut self, - samples: Vec<&mut CoverageSample>, + samples: impl Iterator + ExactSizeIterator, ) -> error::Result<()> { self.report .samples