From cea6543dd29f036a883da231e671b2484997f479 Mon Sep 17 00:00:00 2001 From: rh-jfuller Date: Sun, 16 Aug 2026 04:40:24 +0200 Subject: [PATCH 1/3] refactor(analysis): reduce allocations in graph filter and result construction - Skip JSON serialization in filter() for queries that don't use nested purl:/cpe: field access; the common path (full-text search, name filters) now uses only Value::Custom with zero JSON allocation per node - Share graph node data in BaseSummary via Arc instead of deep cloning: purl/cpe use Arc<[T]>, document_id/product_name/product_version use Arc - Fix Context::intern double hash lookup (contains_key + get) with single entry() call --- modules/analysis/src/model.rs | 13 +++-- modules/analysis/src/model/graph.rs | 65 +++++++++------------- modules/analysis/src/model/roots.rs | 19 ++++--- modules/analysis/src/service/load/mod.rs | 13 ++--- modules/analysis/src/service/mod.rs | 71 ++++++++++++------------ modules/analysis/src/service/test/mod.rs | 12 ++-- 6 files changed, 87 insertions(+), 106 deletions(-) diff --git a/modules/analysis/src/model.rs b/modules/analysis/src/model.rs index 9106d887d..d70d41f65 100644 --- a/modules/analysis/src/model.rs +++ b/modules/analysis/src/model.rs @@ -79,14 +79,17 @@ pub struct CacheStatusEntry { pub struct BaseSummary { pub sbom_id: String, pub node_id: String, - pub purl: Vec, - pub cpe: Vec, + pub purl: Arc<[Purl]>, + pub cpe: Arc<[Cpe]>, pub name: String, pub version: String, pub published: String, - pub document_id: String, - pub product_name: String, - pub product_version: String, + #[schema(value_type = String)] + pub document_id: Arc, + #[schema(value_type = String)] + pub product_name: Arc, + #[schema(value_type = String)] + pub product_version: Arc, } #[derive(Debug, Clone, PartialEq, Eq, Serialize, ToSchema)] diff --git a/modules/analysis/src/model/graph.rs b/modules/analysis/src/model/graph.rs index 7b6c537e5..97900d8f0 100644 --- a/modules/analysis/src/model/graph.rs +++ b/modules/analysis/src/model/graph.rs @@ -103,33 +103,30 @@ fn published_to_string(value: OffsetDateTime) -> String { value.format(&format).unwrap_or_else(|_| value.to_string()) } +static EMPTY_ARC_STRING: std::sync::LazyLock> = + std::sync::LazyLock::new(|| Arc::new(String::new())); + +fn arc_string_or_default(opt: &Option>) -> Arc { + opt.as_ref() + .cloned() + .unwrap_or_else(|| EMPTY_ARC_STRING.clone()) +} + impl From<&Node> for BaseSummary { fn from(value: &Node) -> Self { match value { Node::Package(value) => BaseSummary::from(value), _ => Self { sbom_id: value.sbom_id.to_string(), - node_id: value.node_id.to_string(), - purl: vec![], - cpe: vec![], - name: value.name.to_string(), - version: "".to_string(), + node_id: value.node_id.clone(), + purl: Arc::from([]), + cpe: Arc::from([]), + name: value.name.clone(), + version: String::new(), published: published_to_string(value.published), - document_id: value - .document_id - .as_ref() - .map(|s| s.to_string()) - .unwrap_or_default(), - product_name: value - .product_name - .as_ref() - .map(|s| s.to_string()) - .unwrap_or_default(), - product_version: value - .product_version - .as_ref() - .map(|s| s.to_string()) - .unwrap_or_default(), + document_id: arc_string_or_default(&value.document_id), + product_name: arc_string_or_default(&value.product_name), + product_version: arc_string_or_default(&value.product_version), }, } } @@ -139,27 +136,15 @@ impl From<&PackageNode> for BaseSummary { fn from(value: &PackageNode) -> Self { Self { sbom_id: value.sbom_id.to_string(), - node_id: value.node_id.to_string(), - purl: value.purl.to_vec(), - cpe: value.cpe.to_vec(), - name: value.name.to_string(), - version: value.version.to_string(), + node_id: value.node_id.clone(), + purl: Arc::clone(&value.purl), + cpe: Arc::clone(&value.cpe), + name: value.name.clone(), + version: value.version.clone(), published: published_to_string(value.published), - document_id: value - .document_id - .as_ref() - .map(|s| s.to_string()) - .unwrap_or_default(), - product_name: value - .product_name - .as_ref() - .map(|s| s.to_string()) - .unwrap_or_default(), - product_version: value - .product_version - .as_ref() - .map(|s| s.to_string()) - .unwrap_or_default(), + document_id: arc_string_or_default(&value.document_id), + product_name: arc_string_or_default(&value.product_name), + product_version: arc_string_or_default(&value.product_version), } } } diff --git a/modules/analysis/src/model/roots.rs b/modules/analysis/src/model/roots.rs index 5c985a5ac..9c6d3004d 100644 --- a/modules/analysis/src/model/roots.rs +++ b/modules/analysis/src/model/roots.rs @@ -92,20 +92,21 @@ impl<'a> RootTraces for &'a Vec { mod test { use super::*; use crate::model::BaseSummary; + use std::sync::Arc; use trustify_entity::relationship::Relationship; fn base(node_id: &str) -> BaseSummary { BaseSummary { - sbom_id: "".to_string(), + sbom_id: String::new(), node_id: node_id.to_string(), - purl: vec![], - cpe: vec![], - name: "".to_string(), - version: "".to_string(), - published: "".to_string(), - document_id: "".to_string(), - product_name: "".to_string(), - product_version: "".to_string(), + purl: Arc::from([]), + cpe: Arc::from([]), + name: String::new(), + version: String::new(), + published: String::new(), + document_id: Arc::new(String::new()), + product_name: Arc::new(String::new()), + product_version: Arc::new(String::new()), } } diff --git a/modules/analysis/src/service/load/mod.rs b/modules/analysis/src/service/load/mod.rs index 0112702f8..74b858232 100644 --- a/modules/analysis/src/service/load/mod.rs +++ b/modules/analysis/src/service/load/mod.rs @@ -82,15 +82,10 @@ impl Context { } pub fn intern(&mut self, s: String) -> Arc { - if self.strings.contains_key(&s) - && let Some(s) = self.strings.get(&s) - { - return s.clone(); - } - - let a = Arc::new(s.clone()); - self.strings.insert(s, a.clone()); - a + self.strings + .entry(s) + .or_insert_with_key(|k| Arc::new(k.clone())) + .clone() } } diff --git a/modules/analysis/src/service/mod.rs b/modules/analysis/src/service/mod.rs index ede4d9d49..4fec253e1 100644 --- a/modules/analysis/src/service/mod.rs +++ b/modules/analysis/src/service/mod.rs @@ -914,40 +914,9 @@ impl AnalysisService { }) } GraphQuery::Query(query) => graph.node_weight(i).is_some_and(|node| { - let purls: Vec<_> = match node { - graph::Node::Package(p) => { - p.purl - .iter() - .map(|p| { - let mut v: serde_json::Value = p.into(); - // if any translations are applied to - // the DB query, they must be added to - // this context as well - v["type"] = v["ty"].clone(); - Value::Json(v) - }) - .collect() - } - _ => vec![], - }; - let cpes: Vec<_> = match node { - graph::Node::Package(p) => p - .cpe - .iter() - .map(|cpe| { - Value::Json(json!({ - "part": cpe.part(), - "vendor": cpe.vendor(), - "product": cpe.product(), - "version": cpe.version(), - "update": cpe.update(), - "edition": cpe.edition(), - "language": cpe.language(), - })) - }) - .collect(), - _ => vec![], - }; + let q = &query.q; + let needs_nested_purl = q.contains("purl:"); + let needs_nested_cpe = q.contains("cpe:"); let sbom_id = node.sbom_id.to_string(); let mut context = ValueContext::from([ ("sbom_id", &*sbom_id), @@ -957,10 +926,38 @@ impl AnalysisService { match node { graph::Node::Package(package) => { context.put("version", &*package.version); - context.put_hidden("cpe", &package.cpe); - context.put_hidden("cpe", cpes); context.put_hidden("purl", &package.purl); - context.put_hidden("purl", purls); + context.put_hidden("cpe", &package.cpe); + if needs_nested_purl { + let purls: Vec<_> = package + .purl + .iter() + .map(|p| { + let mut v: serde_json::Value = p.into(); + v["type"] = v["ty"].clone(); + Value::Json(v) + }) + .collect(); + context.put_hidden("purl", purls); + } + if needs_nested_cpe { + let cpes: Vec<_> = package + .cpe + .iter() + .map(|cpe| { + Value::Json(json!({ + "part": cpe.part(), + "vendor": cpe.vendor(), + "product": cpe.product(), + "version": cpe.version(), + "update": cpe.update(), + "edition": cpe.edition(), + "language": cpe.language(), + })) + }) + .collect(); + context.put_hidden("cpe", cpes); + } } graph::Node::External(external) => { context.put( diff --git a/modules/analysis/src/service/test/mod.rs b/modules/analysis/src/service/test/mod.rs index e16ed004e..6f00195bd 100644 --- a/modules/analysis/src/service/test/mod.rs +++ b/modules/analysis/src/service/test/mod.rs @@ -511,12 +511,12 @@ async fn test_simple_by_name_deps_service(ctx: &TrustifyContext) -> Result<(), a assert_eq!(analysis_graph.total, Some(1)); assert_eq!( - analysis_graph.items[0].purl, - vec![Purl::from_str("pkg:rpm/redhat/A@0.0.0?arch=src")?] + &*analysis_graph.items[0].purl, + [Purl::from_str("pkg:rpm/redhat/A@0.0.0?arch=src")?] ); assert_eq!( - analysis_graph.items[0].cpe, - vec![Cpe::from_str("cpe:/a:redhat:simple:1::el9")?] + &*analysis_graph.items[0].cpe, + [Cpe::from_str("cpe:/a:redhat:simple:1::el9")?] ); Ok(()) @@ -545,8 +545,8 @@ async fn test_simple_by_purl_deps_service(ctx: &TrustifyContext) -> Result<(), a .await?; assert_eq!( - analysis_graph.items[0].purl, - vec![Purl::from_str("pkg:rpm/redhat/AA@0.0.0?arch=src")?] + &*analysis_graph.items[0].purl, + [Purl::from_str("pkg:rpm/redhat/AA@0.0.0?arch=src")?] ); assert_eq!(analysis_graph.total, Some(1)); From 5d5d7f020e580e7cbfddbc081c75b778a8d38d51 Mon Sep 17 00:00:00 2001 From: rh-jfuller Date: Sun, 16 Aug 2026 04:49:53 +0200 Subject: [PATCH 2/3] refactor(analysis): wrap relationships HashSet in Arc to avoid per-node clones Each matching node in run_graph_query cloned the HashSet into the async closure. Replace with Arc::new once, Arc::clone per node. --- modules/analysis/src/service/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/modules/analysis/src/service/mod.rs b/modules/analysis/src/service/mod.rs index 4fec253e1..198b7dd8e 100644 --- a/modules/analysis/src/service/mod.rs +++ b/modules/analysis/src/service/mod.rs @@ -725,7 +725,7 @@ impl AnalysisService { graphs: &[(Uuid, Arc)], connection: &C, ) -> Result, Error> { - let relationships = options.relationships; + let relationships = Arc::new(options.relationships); log::debug!("relations: {:?}", relationships); let loader = &GraphLoader::new(self.clone()); @@ -746,7 +746,7 @@ impl AnalysisService { self.concurrency, |graph, node_index, node| { let graph_cache = self.inner.graph_cache.clone(); - let relationships = relationships.clone(); + let relationships = Arc::clone(&relationships); let ancestor_cache = ancestor_cache.clone(); async move { log::trace!( From ef20a93accf4dbdee6d3fcb55048e0b85bdb1bd4 Mon Sep 17 00:00:00 2001 From: rh-jfuller Date: Sun, 16 Aug 2026 05:07:14 +0200 Subject: [PATCH 3/3] perf(analysis): reduce DB round-trips for cross-SBOM resolution - Collapse resolve_rh_external_sbom_descendants from two sequential queries into a single self-join on sbom_node_checksum, also adding checksum type matching - Add ExternalSbomCache to deduplicate resolve_external_sbom calls during descendant traversal; uses OnceCell coalescing so concurrent collectors for the same external reference share one DB query --- modules/analysis/src/service/collector.rs | 52 ++++++++++++++++++- modules/analysis/src/service/mod.rs | 62 ++++++++++++++--------- 2 files changed, 88 insertions(+), 26 deletions(-) diff --git a/modules/analysis/src/service/collector.rs b/modules/analysis/src/service/collector.rs index 18fa80d10..3451739d7 100644 --- a/modules/analysis/src/service/collector.rs +++ b/modules/analysis/src/service/collector.rs @@ -27,6 +27,48 @@ type AncestorResult = Arc>; type AncestorCell = Arc>; type AncestorMap = HashMap<(Uuid, String), AncestorCell>; +type ExternalSbomResult = Arc>; +type ExternalSbomCell = Arc>; +type ExternalSbomMap = HashMap; + +/// Request-scoped cache for [`resolve_external_sbom`] results. +/// +/// During descendant traversal, every `ExternalNode` triggers a +/// `resolve_external_sbom` DB query. The same external reference +/// can appear across multiple SBOMs in the result set, causing +/// redundant queries. This cache deduplicates them. +/// +/// Concurrent callers for the same `node_id` are coalesced via +/// `OnceCell` — only the first executes the query. +#[derive(Default, Clone)] +pub struct ExternalSbomCache { + cache: Arc>, +} + +impl ExternalSbomCache { + /// Resolve an external SBOM reference, returning a cached result + /// when available. + async fn resolve( + &self, + node_id: &str, + connection: &C, + ) -> Result, Error> { + let cell = { + let mut map = self.cache.lock(); + map.entry(node_id.to_string()).or_default().clone() + }; + + let result = cell + .get_or_try_init(|| async { + let resolved = resolve_external_sbom(node_id, connection).await?; + Ok::<_, Error>(Arc::new(resolved)) + }) + .await?; + + Ok((**result).clone()) + } +} + /// Coalescing barrier for [`AncestorCache::prefetch`]. /// /// When multiple concurrent tasks call `prefetch` for the same @@ -205,6 +247,7 @@ pub struct Collector<'a, C: ConnectionTrait> { discovered: DiscoveredTracker, loaded_graphs: Arc>>>, ancestor_cache: AncestorCache, + external_sbom_cache: ExternalSbomCache, relationships: &'a HashSet, connection: &'a C, concurrency: usize, @@ -224,6 +267,7 @@ impl<'a, C: ConnectionTrait> Collector<'a, C> { discovered: self.discovered.clone(), loaded_graphs: self.loaded_graphs.clone(), ancestor_cache: self.ancestor_cache.clone(), + external_sbom_cache: self.external_sbom_cache.clone(), relationships: self.relationships, connection: self.connection, concurrency: self.concurrency, @@ -246,6 +290,7 @@ impl<'a, C: ConnectionTrait> Collector<'a, C> { concurrency: usize, loader: &'a GraphLoader, ancestor_cache: AncestorCache, + external_sbom_cache: ExternalSbomCache, ) -> Self { Self { graph_cache, @@ -258,6 +303,7 @@ impl<'a, C: ConnectionTrait> Collector<'a, C> { discovered: Default::default(), loaded_graphs: Default::default(), ancestor_cache, + external_sbom_cache, relationships, connection, concurrency, @@ -293,6 +339,7 @@ impl<'a, C: ConnectionTrait> Collector<'a, C> { discovered: self.discovered.clone(), loaded_graphs: self.loaded_graphs.clone(), ancestor_cache: self.ancestor_cache.clone(), + external_sbom_cache: self.external_sbom_cache.clone(), relationships: self.relationships, connection: self.connection, concurrency: self.concurrency, @@ -365,7 +412,10 @@ impl<'a, C: ConnectionTrait> Collector<'a, C> { sbom_id: external_sbom_id, node_id: external_node_id, .. - }) = resolve_external_sbom(&external_node.node_id, self.connection).await? + }) = self + .external_sbom_cache + .resolve(&external_node.node_id, self.connection) + .await? else { return Ok(( None, diff --git a/modules/analysis/src/service/mod.rs b/modules/analysis/src/service/mod.rs index 198b7dd8e..04e438ba8 100644 --- a/modules/analysis/src/service/mod.rs +++ b/modules/analysis/src/service/mod.rs @@ -57,7 +57,7 @@ use trustify_entity::{ relationship::Relationship, sbom, sbom_external_node::{self, DiscriminatorType, ExternalType}, - sbom_node_checksum, source_document, + source_document, }; use uuid::Uuid; @@ -235,26 +235,31 @@ async fn resolve_rh_external_sbom_descendants( sbom_external_node_ref: String, connection: &C, ) -> Result, Error> { - // find checksum value for the node - - let Some(entity) = sbom_node_checksum::Entity::find() - .filter(sbom_node_checksum::Column::NodeId.eq(sbom_external_node_ref.clone())) - .filter(sbom_node_checksum::Column::SbomId.eq(sbom_external_sbom_id)) - .one(connection) - .await? - else { - log::debug!("Unable to find checksum"); - return Ok(None); - }; - - log::debug!("Checksum: {} / {}", entity.value, entity.sbom_id); + // Single self-join query: find nodes in other SBOMs that share + // the same checksum value as the given node. + #[derive(Debug, FromQueryResult)] + struct ChecksumMatch { + matched_sbom_id: Uuid, + matched_node_id: String, + } - // now find if there are any other nodes with the same checksums - let matches = sbom_node_checksum::Entity::find() - .filter(sbom_node_checksum::Column::Value.eq(entity.value.to_string())) - .filter(sbom_node_checksum::Column::SbomId.ne(entity.sbom_id)) - .all(connection) - .await?; + let matches = ChecksumMatch::find_by_statement(Statement::from_sql_and_values( + DatabaseBackend::Postgres, + r#" + SELECT matched.sbom_id AS matched_sbom_id, + matched.node_id AS matched_node_id + FROM sbom_node_checksum self_chk + JOIN sbom_node_checksum matched + ON matched.value = self_chk.value + AND matched.type = self_chk.type + AND matched.sbom_id != self_chk.sbom_id + WHERE self_chk.sbom_id = $1 + AND self_chk.node_id = $2 + "#, + [sbom_external_sbom_id.into(), sbom_external_node_ref.into()], + )) + .all(connection) + .await?; log::debug!("Found {} nodes by checksum", matches.len()); @@ -264,17 +269,20 @@ async fn resolve_rh_external_sbom_descendants( // which has not defined a bom-ref - we can 'sniff' this because such nodes always // are ingested with a uuid node-id. .find(|model| { - if Uuid::parse_str(&model.node_id).is_err() { + if Uuid::parse_str(&model.matched_node_id).is_err() { // failed to parse, we keep it true } else { - log::debug!("Dropping suspected top-level node ID: {}", model.node_id); + log::debug!( + "Dropping suspected top-level node ID: {}", + model.matched_node_id + ); false } }) - .map(|matched_model| ResolvedSbom { - sbom_id: matched_model.sbom_id, - node_id: matched_model.node_id, + .map(|matched| ResolvedSbom { + sbom_id: matched.matched_sbom_id, + node_id: matched.matched_node_id, cpe_ids: vec![], graph_node_id: None, })) @@ -730,6 +738,7 @@ impl AnalysisService { let loader = &GraphLoader::new(self.clone()); let ancestor_cache = AncestorCache::default(); + let external_sbom_cache = ExternalSbomCache::default(); // Batch-prefetch ancestor results for all PackageNodes in // the initial set of graphs. This replaces N individual @@ -748,6 +757,7 @@ impl AnalysisService { let graph_cache = self.inner.graph_cache.clone(); let relationships = Arc::clone(&relationships); let ancestor_cache = ancestor_cache.clone(); + let external_sbom_cache = external_sbom_cache.clone(); async move { log::trace!( "Discovered node - sbom: {}, node: {}", @@ -768,6 +778,7 @@ impl AnalysisService { self.concurrency, loader, ancestor_cache.clone(), + external_sbom_cache.clone(), ) .collect(); @@ -784,6 +795,7 @@ impl AnalysisService { self.concurrency, loader, ancestor_cache, + external_sbom_cache, ) .collect();