Analysis module performance: reduce allocs, CPU, and DB round-trips - #2581
Open
rh-jfuller wants to merge 3 commits into
Open
Analysis module performance: reduce allocs, CPU, and DB round-trips#2581rh-jfuller wants to merge 3 commits into
rh-jfuller wants to merge 3 commits into
Conversation
…struction - 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<String> - Fix Context::intern double hash lookup (contains_key + get) with single entry() call
…de clones Each matching node in run_graph_query cloned the HashSet<Relationship> into the async closure. Replace with Arc::new once, Arc::clone per node.
- 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
Contributor
Reviewer's GuideThis PR optimizes the analysis module by reducing per-node allocations, CPU overhead in filtering and summarization, and duplicate database lookups, primarily via smarter data sharing, conditional JSON construction, and request-scoped caching for external SBOM resolution. Sequence diagram for cached external SBOM resolutionsequenceDiagram
participant Collector
participant ExternalSbomCache
participant OnceCell
participant Database as resolve_external_sbom
Collector->>ExternalSbomCache: resolve(node_id, connection)
ExternalSbomCache->>ExternalSbomCache: cache.lock()
ExternalSbomCache->>ExternalSbomCache: cache.entry(node_id).or_default()
ExternalSbomCache->>OnceCell: get_or_try_init(async { resolve_external_sbom(node_id, connection) })
alt first caller for node_id
OnceCell->>Database: resolve_external_sbom(node_id, connection)
Database-->>OnceCell: Option<ResolvedSbom>
OnceCell-->>ExternalSbomCache: Arc<Option<ResolvedSbom>>
else cached result
OnceCell-->>ExternalSbomCache: Arc<Option<ResolvedSbom>>
end
ExternalSbomCache-->>Collector: Option<ResolvedSbom>
note over Collector,ExternalSbomCache: Concurrent calls for the same node_id share a single DB query via OnceCell
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
GraphQuery::Queryoptimization that gates nested purl/cpe materialization onquery.q.contains("purl:")/"cpe:"is quite brittle; if the query syntax evolves (e.g., different casing, aliases, or structured fields), this detection will silently miss needed data, so consider delegating this decision to a parsed query structure instead of raw substring matching. - The new
ExternalSbomCacheis keyed solely bynode_id; ifresolve_external_sbombehavior ever depends on additional context (e.g., SBOM ID, discriminator, or external type), this cache could return incorrect cross-request results, so it would be safer to make the key reflect all inputs that affect resolution.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `GraphQuery::Query` optimization that gates nested purl/cpe materialization on `query.q.contains("purl:")` / `"cpe:"` is quite brittle; if the query syntax evolves (e.g., different casing, aliases, or structured fields), this detection will silently miss needed data, so consider delegating this decision to a parsed query structure instead of raw substring matching.
- The new `ExternalSbomCache` is keyed solely by `node_id`; if `resolve_external_sbom` behavior ever depends on additional context (e.g., SBOM ID, discriminator, or external type), this cache could return incorrect cross-request results, so it would be safer to make the key reflect all inputs that affect resolution.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
graph analysis module had several performance inefficiencies:
filter()— every node scanned during aGraphQuery::Querybuiltserde_json::Valuemaps for all purls and CPEs even if query only needed substring matchingBaseSummary— every node in the result tree cloned all purls, CPEs, and string fields from the graph into owned typesContext::intern—contains_key+getinstead of singleentry()call during graph constructionHashSet<Relationship>clone — the relationship filter set was cloned for every matching node in the async closureresolve_rh_external_sbom_descendants— first fetched the checksum value, then searched for matchesresolve_external_sbom— everyExternalNodehit during descendant traversal triggered a fresh DB query, even for duplicate referencesAddressing these positively impacts:
No API changes. No schema changes. No migration required.
Summary by Sourcery
Improve performance of the analysis module by reducing per-node allocations and database round-trips, primarily through caching, shared ownership of summary data, and more efficient query and filtering paths.
New Features:
Enhancements:
Tests: