From 9420f519fa9e18ec46e9e5eb79f474fc1586309d Mon Sep 17 00:00:00 2001 From: Cheick Keita Date: Tue, 15 Aug 2023 13:03:20 -0700 Subject: [PATCH] Deserialize the coverage files directly into the output files (#3410) * deserialize the coverage files directly into the output files * reduce the amount of data structure cloning individual save function for each formats * cleanup * unformatted json * bring back spawn * remove stray comment * build fix * fix build --- src/agent/cobertura/src/lib.rs | 71 ++++++----- src/agent/coverage/examples/cobertura.rs | 2 +- src/agent/coverage/examples/record.rs | 6 +- src/agent/coverage/src/cobertura.rs | 6 +- src/agent/coverage/src/source.rs | 3 +- src/agent/coverage/tests/snapshot.rs | 2 +- .../src/coverage/binary.rs | 4 +- .../src/coverage/binary/v1.rs | 4 +- .../src/coverage/source.rs | 4 +- .../src/coverage/source/v1.rs | 8 +- .../src/tasks/coverage/generic.rs | 119 ++++++++++++------ 11 files changed, 137 insertions(+), 92 deletions(-) diff --git a/src/agent/cobertura/src/lib.rs b/src/agent/cobertura/src/lib.rs index 9becf4a113..e919e08733 100644 --- a/src/agent/cobertura/src/lib.rs +++ b/src/agent/cobertura/src/lib.rs @@ -12,15 +12,20 @@ impl CoberturaCoverage { let mut writer = Writer::new_with_indent(cursor, b' ', 2); - self.write_xml(&mut writer)?; + self._write_xml(&mut writer)?; let text = String::from_utf8(data)?; Ok(text) } } -trait WriteXml { - fn write_xml(&self, writer: &mut Writer) -> Result<()>; +pub trait WriteXml { + fn _write_xml(&self, writer: &mut Writer) -> Result<()>; + + fn write_xml(&self, writer: W) -> Result<()> { + let mut writer = Writer::new(writer); + self._write_xml(&mut writer) + } } // Only write optional fields if present. @@ -28,9 +33,9 @@ impl WriteXml for Option where T: WriteXml, { - fn write_xml(&self, writer: &mut Writer) -> Result<()> { + fn _write_xml(&self, writer: &mut Writer) -> Result<()> { if let Some(value) = self { - value.write_xml(writer)?; + value._write_xml(writer)?; } Ok(()) @@ -41,9 +46,9 @@ impl WriteXml for Vec where T: WriteXml, { - fn write_xml(&self, writer: &mut Writer) -> Result<()> { + fn _write_xml(&self, writer: &mut Writer) -> Result<()> { for value in self { - value.write_xml(writer)?; + value._write_xml(writer)?; } Ok(()) @@ -101,7 +106,7 @@ pub struct CoberturaCoverage { } impl WriteXml for CoberturaCoverage { - fn write_xml(&self, writer: &mut Writer) -> Result<()> { + fn _write_xml(&self, writer: &mut Writer) -> Result<()> { writer .create_element("coverage") .with_attributes([ @@ -116,8 +121,8 @@ impl WriteXml for CoberturaCoverage { ("timestamp", uint!(self.timestamp)), ]) .write_inner_content(|w| { - self.sources.write_xml(w)?; - self.packages.write_xml(w)?; + self.sources._write_xml(w)?; + self.packages._write_xml(w)?; Ok(()) })?; @@ -133,10 +138,10 @@ pub struct Sources { } impl WriteXml for Sources { - fn write_xml(&self, writer: &mut Writer) -> Result<()> { + fn _write_xml(&self, writer: &mut Writer) -> Result<()> { writer .create_element("sources") - .write_inner_content(|w| self.sources.write_xml(w))?; + .write_inner_content(|w| self.sources._write_xml(w))?; Ok(()) } @@ -149,7 +154,7 @@ pub struct Source { } impl WriteXml for Source { - fn write_xml(&self, writer: &mut Writer) -> Result<()> { + fn _write_xml(&self, writer: &mut Writer) -> Result<()> { writer .create_element("source") .with_attributes([("path", string!(self.path))]) @@ -166,10 +171,10 @@ pub struct Packages { } impl WriteXml for Packages { - fn write_xml(&self, writer: &mut Writer) -> Result<()> { + fn _write_xml(&self, writer: &mut Writer) -> Result<()> { writer .create_element("packages") - .write_inner_content(|w| self.packages.write_xml(w))?; + .write_inner_content(|w| self.packages._write_xml(w))?; Ok(()) } @@ -191,7 +196,7 @@ pub struct Package { } impl WriteXml for Package { - fn write_xml(&self, writer: &mut Writer) -> Result<()> { + fn _write_xml(&self, writer: &mut Writer) -> Result<()> { writer .create_element("package") .with_attributes([ @@ -200,7 +205,7 @@ impl WriteXml for Package { ("branch-rate", float!(self.branch_rate)), ("complexity", uint!(self.complexity)), ]) - .write_inner_content(|w| self.classes.write_xml(w))?; + .write_inner_content(|w| self.classes._write_xml(w))?; Ok(()) } @@ -213,10 +218,10 @@ pub struct Classes { } impl WriteXml for Classes { - fn write_xml(&self, writer: &mut Writer) -> Result<()> { + fn _write_xml(&self, writer: &mut Writer) -> Result<()> { writer .create_element("classes") - .write_inner_content(|w| self.classes.write_xml(w))?; + .write_inner_content(|w| self.classes._write_xml(w))?; Ok(()) } @@ -241,7 +246,7 @@ pub struct Class { } impl WriteXml for Class { - fn write_xml(&self, writer: &mut Writer) -> Result<()> { + fn _write_xml(&self, writer: &mut Writer) -> Result<()> { writer .create_element("class") .with_attributes([ @@ -252,8 +257,8 @@ impl WriteXml for Class { ("complexity", uint!(self.complexity)), ]) .write_inner_content(|w| { - self.methods.write_xml(w)?; - self.lines.write_xml(w)?; + self.methods._write_xml(w)?; + self.lines._write_xml(w)?; Ok(()) })?; @@ -268,10 +273,10 @@ pub struct Methods { } impl WriteXml for Methods { - fn write_xml(&self, writer: &mut Writer) -> Result<()> { + fn _write_xml(&self, writer: &mut Writer) -> Result<()> { writer .create_element("methods") - .write_inner_content(|w| self.methods.write_xml(w))?; + .write_inner_content(|w| self.methods._write_xml(w))?; Ok(()) } @@ -293,7 +298,7 @@ pub struct Method { } impl WriteXml for Method { - fn write_xml(&self, writer: &mut Writer) -> Result<()> { + fn _write_xml(&self, writer: &mut Writer) -> Result<()> { writer .create_element("method") .with_attributes([ @@ -302,7 +307,7 @@ impl WriteXml for Method { ("line-rate", float!(self.line_rate)), ("branch-rate", float!(self.branch_rate)), ]) - .write_inner_content(|w| self.lines.write_xml(w))?; + .write_inner_content(|w| self.lines._write_xml(w))?; Ok(()) } @@ -315,10 +320,10 @@ pub struct Lines { } impl WriteXml for Lines { - fn write_xml(&self, writer: &mut Writer) -> Result<()> { + fn _write_xml(&self, writer: &mut Writer) -> Result<()> { writer .create_element("lines") - .write_inner_content(|w| self.lines.write_xml(w))?; + .write_inner_content(|w| self.lines._write_xml(w))?; Ok(()) } @@ -340,7 +345,7 @@ pub struct Line { } impl WriteXml for Line { - fn write_xml(&self, writer: &mut Writer) -> Result<()> { + fn _write_xml(&self, writer: &mut Writer) -> Result<()> { let condition_coverage = if let Some(s) = &self.condition_coverage { s.as_str() } else { @@ -355,7 +360,7 @@ impl WriteXml for Line { ("branch", boolean!(self.branch.unwrap_or_default())), ("condition-coverage", condition_coverage), ]) - .write_inner_content(|w| self.conditions.write_xml(w))?; + .write_inner_content(|w| self.conditions._write_xml(w))?; Ok(()) } @@ -368,10 +373,10 @@ pub struct Conditions { } impl WriteXml for Conditions { - fn write_xml(&self, writer: &mut Writer) -> Result<()> { + fn _write_xml(&self, writer: &mut Writer) -> Result<()> { writer .create_element("conditions") - .write_inner_content(|w| self.conditions.write_xml(w))?; + .write_inner_content(|w| self.conditions._write_xml(w))?; Ok(()) } @@ -389,7 +394,7 @@ pub struct Condition { } impl WriteXml for Condition { - fn write_xml(&self, writer: &mut Writer) -> Result<()> { + fn _write_xml(&self, writer: &mut Writer) -> Result<()> { writer .create_element("condition") .with_attributes([ diff --git a/src/agent/coverage/examples/cobertura.rs b/src/agent/coverage/examples/cobertura.rs index 8d999d6a0f..2ba5ec2f6a 100644 --- a/src/agent/coverage/examples/cobertura.rs +++ b/src/agent/coverage/examples/cobertura.rs @@ -40,7 +40,7 @@ fn generate_output() -> Result { coverage.files.insert(file_path, file); } - CoberturaCoverage::from(coverage).to_string() + CoberturaCoverage::from(&coverage).to_string() } #[cfg(test)] diff --git a/src/agent/coverage/examples/record.rs b/src/agent/coverage/examples/record.rs index 4d2ec46922..e2cc34dcd2 100644 --- a/src/agent/coverage/examples/record.rs +++ b/src/agent/coverage/examples/record.rs @@ -190,7 +190,7 @@ fn dump_modoff(coverage: &BinaryCoverage) -> Result<()> { } fn dump_source_line(binary: &BinaryCoverage, allowlist: AllowList) -> Result<()> { - let source = coverage::source::binary_to_source_coverage(binary, allowlist)?; + let source = coverage::source::binary_to_source_coverage(binary, &allowlist)?; for (path, file) in &source.files { for (line, count) in &file.lines { @@ -202,8 +202,8 @@ fn dump_source_line(binary: &BinaryCoverage, allowlist: AllowList) -> Result<()> } fn dump_cobertura(binary: &BinaryCoverage, allowlist: AllowList) -> Result<()> { - let source = coverage::source::binary_to_source_coverage(binary, allowlist)?; - let cobertura: CoberturaCoverage = source.into(); + let source = coverage::source::binary_to_source_coverage(binary, &allowlist)?; + let cobertura: CoberturaCoverage = (&source).into(); println!("{}", cobertura.to_string()?); diff --git a/src/agent/coverage/src/cobertura.rs b/src/agent/coverage/src/cobertura.rs index 669e787fb2..d9175c5e38 100644 --- a/src/agent/coverage/src/cobertura.rs +++ b/src/agent/coverage/src/cobertura.rs @@ -16,8 +16,8 @@ use crate::source::SourceCoverage; // Dir -> Set type FileMap<'a> = BTreeMap<&'a str, BTreeSet<&'a FilePath>>; -impl From for CoberturaCoverage { - fn from(source: SourceCoverage) -> Self { +impl From<&SourceCoverage> for CoberturaCoverage { + fn from(source: &SourceCoverage) -> Self { // The Cobertura data model is organized around `classes` and `methods` contained // in `packages`. Our source coverage has no language-level assumptions. // @@ -51,7 +51,7 @@ impl From for CoberturaCoverage { // Iterate through the grouped files, accumulating `` elements. let (packages, hit_counts): (Vec, Vec) = file_map .into_iter() - .map(|(directory, files)| directory_to_package(&source, directory, files)) + .map(|(directory, files)| directory_to_package(source, directory, files)) .unzip(); let hit_count: HitCounts = hit_counts.into_iter().sum(); diff --git a/src/agent/coverage/src/source.rs b/src/agent/coverage/src/source.rs index 735e7c6b75..b556fe447a 100644 --- a/src/agent/coverage/src/source.rs +++ b/src/agent/coverage/src/source.rs @@ -51,14 +51,13 @@ impl From for u32 { pub fn binary_to_source_coverage( binary: &BinaryCoverage, - source_allowlist: impl Into>, + source_allowlist: &AllowList, ) -> Result { use std::collections::btree_map::Entry; use symbolic::debuginfo::Object; use symbolic::symcache::{SymCache, SymCacheConverter}; - let source_allowlist = source_allowlist.into().unwrap_or_default(); let loader = Loader::new(); let mut source = SourceCoverage::default(); diff --git a/src/agent/coverage/tests/snapshot.rs b/src/agent/coverage/tests/snapshot.rs index 0b44bf1dd6..75d524e2da 100644 --- a/src/agent/coverage/tests/snapshot.rs +++ b/src/agent/coverage/tests/snapshot.rs @@ -54,7 +54,7 @@ fn windows_snapshot_tests() { // generate source-line coverage info let source = - coverage::source::binary_to_source_coverage(&recorded.coverage, source_allowlist) + coverage::source::binary_to_source_coverage(&recorded.coverage, &source_allowlist) .expect("binary_to_source_coverage"); let file_coverage = source diff --git a/src/agent/onefuzz-file-format/src/coverage/binary.rs b/src/agent/onefuzz-file-format/src/coverage/binary.rs index 5fc6346536..a0fc75cb02 100644 --- a/src/agent/onefuzz-file-format/src/coverage/binary.rs +++ b/src/agent/onefuzz-file-format/src/coverage/binary.rs @@ -32,8 +32,8 @@ impl BinaryCoverageJson { } // Convert into the latest format. -impl From for BinaryCoverageJson { - fn from(source: BinaryCoverage) -> Self { +impl From<&BinaryCoverage> for BinaryCoverageJson { + fn from(source: &BinaryCoverage) -> Self { v1::BinaryCoverageJson::from(source).into() } } diff --git a/src/agent/onefuzz-file-format/src/coverage/binary/v1.rs b/src/agent/onefuzz-file-format/src/coverage/binary/v1.rs index 154a056bcd..4f79780466 100644 --- a/src/agent/onefuzz-file-format/src/coverage/binary/v1.rs +++ b/src/agent/onefuzz-file-format/src/coverage/binary/v1.rs @@ -21,8 +21,8 @@ pub struct ModuleCoverageJson { pub blocks: BTreeMap, } -impl From for BinaryCoverageJson { - fn from(binary: BinaryCoverage) -> Self { +impl From<&BinaryCoverage> for BinaryCoverageJson { + fn from(binary: &BinaryCoverage) -> Self { let mut modules = BTreeMap::new(); for (path, offsets) in &binary.modules { diff --git a/src/agent/onefuzz-file-format/src/coverage/source.rs b/src/agent/onefuzz-file-format/src/coverage/source.rs index 74e7adcb10..779e572492 100644 --- a/src/agent/onefuzz-file-format/src/coverage/source.rs +++ b/src/agent/onefuzz-file-format/src/coverage/source.rs @@ -32,8 +32,8 @@ impl SourceCoverageJson { } // Convert into the latest format. -impl From for SourceCoverageJson { - fn from(source: SourceCoverage) -> Self { +impl From<&SourceCoverage> for SourceCoverageJson { + fn from(source: &SourceCoverage) -> Self { v1::SourceCoverageJson::from(source).into() } } diff --git a/src/agent/onefuzz-file-format/src/coverage/source/v1.rs b/src/agent/onefuzz-file-format/src/coverage/source/v1.rs index 0dc4ca6b1a..9b6e4fa477 100644 --- a/src/agent/onefuzz-file-format/src/coverage/source/v1.rs +++ b/src/agent/onefuzz-file-format/src/coverage/source/v1.rs @@ -23,14 +23,14 @@ pub struct FileCoverageJson { pub lines: BTreeMap, } -impl From for SourceCoverageJson { - fn from(source: SourceCoverage) -> Self { +impl From<&SourceCoverage> for SourceCoverageJson { + fn from(source: &SourceCoverage) -> Self { let mut json = SourceCoverageJson::default(); - for (path, file) in source.files { + for (path, file) in &source.files { let mut file_json = FileCoverageJson::default(); - for (line, count) in file.lines { + for (line, count) in &file.lines { let line_number = LineNumber(line.number()); let hit_count = count.0; file_json.lines.insert(line_number, hit_count); diff --git a/src/agent/onefuzz-task/src/tasks/coverage/generic.rs b/src/agent/onefuzz-task/src/tasks/coverage/generic.rs index 17ece8c017..b112cfefbe 100644 --- a/src/agent/onefuzz-task/src/tasks/coverage/generic.rs +++ b/src/agent/onefuzz-task/src/tasks/coverage/generic.rs @@ -10,7 +10,7 @@ use std::time::Duration; use anyhow::{bail, Context, Result}; use async_trait::async_trait; -use cobertura::CoberturaCoverage; +use cobertura::{CoberturaCoverage, WriteXml}; use coverage::allowlist::AllowList; use coverage::binary::{BinaryCoverage, DebugInfoCache}; use coverage::record::CoverageRecorder; @@ -29,6 +29,7 @@ use onefuzz_file_format::coverage::{ use onefuzz_telemetry::{event, warn, Event::coverage_data, Event::coverage_failed, EventData}; use storage_queue::{Message, QueueClient}; use tokio::fs; +use tokio::sync::RwLock; use tokio::task::spawn_blocking; use tokio_stream::wrappers::ReadDirStream; use url::Url; @@ -214,9 +215,9 @@ struct TargetAllowList { struct TaskContext<'a> { config: &'a Config, - coverage: BinaryCoverage, + coverage: RwLock, module_allowlist: AllowList, - source_allowlist: AllowList, + source_allowlist: Arc, heartbeat: Option, cache: Arc, } @@ -242,9 +243,9 @@ impl<'a> TaskContext<'a> { Ok(Self { config, - coverage, + coverage: RwLock::new(coverage), module_allowlist: allowlist.modules, - source_allowlist: allowlist.source_files, + source_allowlist: Arc::new(allowlist.source_files), heartbeat, cache: Arc::new(cache), }) @@ -287,8 +288,8 @@ impl<'a> TaskContext<'a> { async fn try_record_input(&mut self, input: &Path) -> Result<()> { let coverage = self.record_impl(input).await?; - self.coverage.merge(&coverage); - + let mut self_coverage = RwLock::write(&self.coverage).await; + self_coverage.merge(&coverage); Ok(()) } @@ -450,56 +451,96 @@ impl<'a> TaskContext<'a> { pub async fn report_coverage_stats(&self) -> Result<()> { use EventData::*; - let s = CoverageStats::new(&self.coverage); + let coverage = RwLock::read(&self.coverage).await; + let s = CoverageStats::new(&coverage); event!(coverage_data; Covered = s.covered, Features = s.features, Rate = s.rate); metric!(coverage_data; 1.0; Covered = s.covered, Features = s.features, Rate = s.rate); Ok(()) } - pub async fn save_and_sync_coverage(&self) -> Result<()> { - // JSON binary coverage. - let binary = self.coverage.clone(); - let json = BinaryCoverageJson::V1(BinaryCoverageJsonV1::from(binary)); - let text = serde_json::to_string(&json).context("serializing binary coverage")?; - let path = self.config.coverage.local_path.join(COVERAGE_FILE); - fs::write(&path, &text) - .await - .with_context(|| format!("writing coverage to {}", path.display()))?; - - // JSON source coverage. - let source = self.source_coverage().await?; - let json = SourceCoverageJson::V1(SourceCoverageJsonV1::from(source.clone())); - let text = serde_json::to_string(&json).context("serializing source coverage")?; - let path = self.config.coverage.local_path.join(SOURCE_COVERAGE_FILE); - fs::write(&path, &text) - .await - .with_context(|| format!("writing source coverage to {}", path.display()))?; + pub async fn save_coverage( + coverage: &RwLock, + source_allowlist: &Arc, + binary_coverage_path: &Path, + source_coverage_path: &Path, + copbertura_file_path: &Path, + ) -> Result<()> { + let source = Self::source_coverage(coverage, source_allowlist.clone()).await?; + let coverage = coverage.read().await; + + Self::save_binary_coverage(&coverage, binary_coverage_path)?; + Self::save_source_coverage(&source, source_coverage_path).await?; + Self::save_cobertura_xml(&source, copbertura_file_path).await?; + Ok(()) + } + + async fn source_coverage( + coverage: &RwLock, + source_allowlist: Arc, + ) -> Result { + // Must be owned due to `spawn_blocking()` lifetimes. + let allowlist = source_allowlist.clone(); + let binary = Arc::new(coverage.read().await.clone()); + // Conversion to source coverage heavy on blocking I/O. + spawn_blocking(move || binary_to_source_coverage(&binary, &allowlist)).await? + } - // Cobertura XML source coverage. - let cobertura = CoberturaCoverage::from(source.clone()); - let text = cobertura.to_string()?; - let path = self + pub async fn save_and_sync_coverage(&self) -> Result<()> { + let copbertura_file_path = self .config .coverage .local_path .join(COBERTURA_COVERAGE_FILE); - fs::write(&path, &text) - .await - .with_context(|| format!("writing cobertura source coverage to {}", path.display()))?; + let source_coverage_path = self.config.coverage.local_path.join(SOURCE_COVERAGE_FILE); + let binary_coverage_path = self.config.coverage.local_path.join(COVERAGE_FILE); + + Self::save_coverage( + &self.coverage, + &self.source_allowlist, + &binary_coverage_path, + &source_coverage_path, + &copbertura_file_path, + ) + .await?; self.config.coverage.sync_push().await?; + Ok(()) + } + async fn save_cobertura_xml(source: &SourceCoverage, path: &Path) -> Result<(), anyhow::Error> { + let cobertura = CoberturaCoverage::from(source); + let cobertura_coverage_file = std::fs::File::create(path) + .with_context(|| format!("creating cobertura coverage file {}", path.display()))?; + let cobertura_coverage_file_writer = std::io::BufWriter::new(cobertura_coverage_file); + cobertura + .write_xml(cobertura_coverage_file_writer) + .with_context(|| format!("serializing cobertura coverage to {}", path.display()))?; Ok(()) } - async fn source_coverage(&self) -> Result { - // Must be owned due to `spawn_blocking()` lifetimes. - let allowlist = self.source_allowlist.clone(); - let binary = self.coverage.clone(); + async fn save_source_coverage(source: &SourceCoverage, path: &Path) -> Result<()> { + let json = SourceCoverageJson::V1(SourceCoverageJsonV1::from(source)); + let source_coverage_file = std::fs::File::create(path) + .with_context(|| format!("creating source coverage file {}", path.display()))?; + let source_coverage_file_writer = std::io::BufWriter::new(source_coverage_file); + serde_json::to_writer_pretty(source_coverage_file_writer, &json) + .with_context(|| format!("serializing source coverage to {}", path.display()))?; + Ok(()) + } - // Conversion to source coverage heavy on blocking I/O. - spawn_blocking(move || binary_to_source_coverage(&binary, allowlist)).await? + fn save_binary_coverage( + binary_coverage: &BinaryCoverage, + path: &Path, + ) -> Result<(), anyhow::Error> { + let json = BinaryCoverageJson::V1(BinaryCoverageJsonV1::from(binary_coverage)); + + let coverage_file = std::fs::File::create(path) + .with_context(|| format!("creating coverage file {}", path.display()))?; + let coverage_file_writer = std::io::BufWriter::new(coverage_file); + serde_json::to_writer(coverage_file_writer, &json) + .with_context(|| format!("serializing binary coverage to {}", path.display()))?; + Ok(()) } }