perf(analysis): paginate before hydration to fix component search timeout#2446
Open
mrrajan wants to merge 3 commits into
Open
perf(analysis): paginate before hydration to fix component search timeout#2446mrrajan wants to merge 3 commits into
mrrajan wants to merge 3 commits into
Conversation
…eout Split graph node processing into three phases: 1. Count matching nodes across cached graphs (cheap in-memory filter) 2. Apply offset/limit to the globally-flattened node index iterator 3. Run expensive Collector hydration only for the paged subset This reduces per-request work from ~39,280 Collector runs to ~25 (the default page limit), a ~1,500x improvement. Also: - Add deterministic graph ordering via ORDER BY sbom_id in load_graphs_subquery and sorted/deduped Vec in load_latest_graphs_query - Migrate log::debug to tracing::debug in retrieve_latest - Fix missing tokio feature on async-compression in common crate - Add 4 pagination tests: limits, offset, cross-graph, endpoint Implements TC-4365 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Assisted-by: Claude Code
Implements TC-4365 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Assisted-by: Claude Code
Replace Vec with manual sort()+dedup() with BTreeSet, which provides sorted unique elements directly via the standard library. Implements TC-4365 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Assisted-by: Claude Code
Contributor
Reviewer's GuideImplements offset/limit pagination at the graph index level before node hydration, adds a lightweight total-count path, and makes graph loading deterministic to prevent component search timeouts and ensure stable pagination across graphs and through the HTTP endpoint. Sequence diagram for paginated graph query before hydrationsequenceDiagram
actor Client
participant AnalysisService
participant PackageGraph
participant GraphLoader
participant Collector
Client->>AnalysisService: run_graph_query_paged(query, options, graphs, paginated, connection)
alt paginated.total()
AnalysisService->>AnalysisService: count_matching_nodes(&query, graphs)
loop each_graph
AnalysisService->>PackageGraph: node_indices()
end
end
AnalysisService->>AnalysisService: collect_graph_paged(query, graphs, concurrency, offset, limit, create)
loop each_graph
AnalysisService->>PackageGraph: node_indices()
end
AnalysisService->>GraphLoader: GraphLoader::new(self)
loop each_paged_node
AnalysisService->>Collector: Collector::new(..., Direction::Incoming)
AnalysisService->>Collector: Collector::new(..., Direction::Outgoing)
AnalysisService->>Collector: collect()
end
AnalysisService-->>Client: PaginatedResults<Node>
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- collect_graph_paged currently materializes and sorts all matching nodes across all graphs before applying offset/limit; if match sets are large this can still be expensive, so consider an approach that skips the first
offsetmatches on the fly and only stores up tolimititems (e.g., streaming iteration with a bounded buffer or a selection algorithm) to reduce memory and sort cost. - count_matching_nodes traverses all graphs a second time when total() is true, duplicating the filter work done in collect_graph_paged; if totals are requested frequently, it might be worth sharing the traversal (e.g., counting while collecting) or caching counts to avoid an extra full pass over all node indices.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- collect_graph_paged currently materializes and sorts all matching nodes across all graphs before applying offset/limit; if match sets are large this can still be expensive, so consider an approach that skips the first `offset` matches on the fly and only stores up to `limit` items (e.g., streaming iteration with a bounded buffer or a selection algorithm) to reduce memory and sort cost.
- count_matching_nodes traverses all graphs a second time when total() is true, duplicating the filter work done in collect_graph_paged; if totals are requested frequently, it might be worth sharing the traversal (e.g., counting while collecting) or caching counts to avoid an extra full pass over all node indices.
## Individual Comments
### Comment 1
<location path="modules/analysis/src/service/mod.rs" line_range="524-533" />
<code_context>
}
+ /// Count nodes matching the query across all graphs, without hydrating them.
+ fn count_matching_nodes(query: &GraphQuery, graphs: &[(Uuid, Arc<PackageGraph>)]) -> u64 {
+ graphs
+ .iter()
+ .map(|(_, graph)| {
+ graph
+ .node_indices()
+ .filter(|&i| Self::filter(graph, query, i))
+ .count() as u64
+ })
+ .sum()
+ }
+
</code_context>
<issue_to_address>
**suggestion (performance):** Avoid double iteration when computing totals for paginated queries to reduce cost on large graphs
When `paginated.total()` is true, this now walks all graphs twice: once in `count_matching_nodes`, and again in `collect_graph_paged` to build the page. On large graphs this doubles the dominant cost. Consider deriving the total from the same `matching` sequence used in `collect_graph_paged` (e.g., via a shared helper that returns both `items` and `total`, or by having `collect_graph_paged` optionally compute/return the total) so you only traverse and filter the nodes once per request.
Suggested implementation:
```rust
})
}
```
To fully adopt the "single traversal" approach and remove the double walk:
1. Locate the function that builds paginated results (likely named something like `collect_graph_paged` or similar) in `modules/analysis/src/service/mod.rs`.
2. Remove its dependency on `count_matching_nodes`:
- Delete any calls to `Self::count_matching_nodes(query, graphs)` (or similar).
3. In the body of `collect_graph_paged`, where you currently iterate over graphs/nodes to produce the `items` for the current page:
- Introduce a `total` counter initialized to `0u64`.
- For each node that passes `Self::filter(graph, query, i)`:
- Increment `total` by 1.
- Apply pagination (offset/limit) logic to decide whether to push that node into the `items` collection.
- This way, you build the page and the total in the same loop.
4. Update the return type or construction of the paginated response:
- If the API allows, have `collect_graph_paged` return both `items` and `total` (or construct a struct that contains both) so callers can access the total without an extra traversal.
- Only compute/attach `total` when `paginated.total()` (or equivalent flag) is true, to avoid unnecessary counting when totals are not requested.
5. Ensure there are no remaining references to `count_matching_nodes` anywhere in the codebase; if there are, update them to use the new "single traversal" logic described above.
These changes will ensure that totals for paginated queries are derived during the same pass that builds the page, avoiding the double iteration and associated cost on large graphs.
</issue_to_address>
### Comment 2
<location path="modules/analysis/src/service/mod.rs" line_range="575-580" />
<code_context>
+
+ let matching: Vec<_> = matching.into_iter().skip(offset).take(limit).collect();
+
+ stream::iter(matching)
+ .map(|(graph, node_index, package_node)| {
+ let create = create.clone();
+ create(graph, node_index, package_node)
+ })
+ .buffer_unordered(concurrency)
+ .try_collect::<Vec<_>>()
+ .await
</code_context>
<issue_to_address>
**issue (bug_risk):** Using `buffer_unordered` may break deterministic ordering of nodes within a page
`buffer_unordered` can change the order of results based on completion timing, so the final `Vec` may no longer be sorted by `(sbom_id, node_id)`. If callers depend on stable pagination (consistent page contents or resuming by offset), this non-determinism could cause subtle bugs. Consider using `buffered` or re-sorting after collection to retain deterministic ordering while still leveraging concurrency.
</issue_to_address>
### Comment 3
<location path="modules/analysis/src/service/load/mod.rs" line_range="457-459" />
<code_context>
) -> Result<Vec<(Uuid, Arc<PackageGraph>)>, Error> {
let distinct_sbom_ids = sbom::Entity::find()
.filter(sbom::Column::SbomId.in_subquery(subquery))
+ .order_by_asc(sbom::Column::SbomId)
.select()
.all(connection)
</code_context>
<issue_to_address>
**suggestion (performance):** Ordering by `SbomId` at the database layer may have indexing implications
This adds deterministic ordering but may introduce an extra sort on the DB side if there’s no index on `SbomId` (or if it doesn’t match the PK/index). Please check whether an appropriate index exists or whether this ordering can reuse an existing one. If not, and the number of `distinct_sbom_ids` is usually small, consider sorting client-side instead.
Suggested implementation:
```rust
) -> Result<Vec<(Uuid, Arc<PackageGraph>)>, Error> {
// Fetch distinct_sbom_ids without imposing a DB-side sort; we sort client-side below.
let mut distinct_sbom_ids = sbom::Entity::find()
.filter(sbom::Column::SbomId.in_subquery(subquery))
.select()
.all(connection)
.await?;
// Ensure deterministic ordering by sorting in memory by SbomId.
distinct_sbom_ids.sort_by_key(|model| model.sbom_id);
```
1. This change assumes the SeaORM model for the `sbom` table exposes the primary key/column as `sbom_id`. If the field name differs (e.g. `sbom_id` is nested or renamed), adjust the closure in `sort_by_key` accordingly.
2. If `distinct_sbom_ids` is later used as an iterator or converted into another structure (e.g. mapping to `(Uuid, Arc<PackageGraph>)`), no other changes should be needed, since the vector is now deterministically ordered before subsequent processing.
</issue_to_address>
### Comment 4
<location path="modules/analysis/src/service/test/mod.rs" line_range="976-985" />
<code_context>
+async fn test_pagination_cross_graph(ctx: &TrustifyContext) -> Result<(), anyhow::Error> {
</code_context>
<issue_to_address>
**issue (testing):** The uniqueness check only uses `sbom_id`, which may be too coarse and can make the test misleading or flaky if multiple matches exist in the same SBOM.
This test currently deduplicates only by `sbom_id`:
```rust
let unique: std::collections::HashSet<_> = collected_sbom_ids.iter().collect();
assert_eq!(unique.len(), total, "no duplicate items across pages");
```
If multiple nodes from the same SBOM match, this will incorrectly treat distinct items as duplicates and can both fail valid pagination and miss true `(sbom_id, node_id)` duplicates.
To better assert "no duplicate items, no gaps", collect and deduplicate on `(sbom_id, node_id)` instead, e.g.:
```rust
let mut collected = Vec::new();
...
collected.push((item.base.sbom_id.clone(), item.base.node_id.clone()));
...
let unique: HashSet<_> = collected.iter().cloned().collect();
assert_eq!(unique.len(), total);
```
This makes the test robust when multiple nodes per SBOM are returned.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+524
to
+533
| fn count_matching_nodes(query: &GraphQuery, graphs: &[(Uuid, Arc<PackageGraph>)]) -> u64 { | ||
| graphs | ||
| .iter() | ||
| .map(|(_, graph)| { | ||
| graph | ||
| .node_indices() | ||
| .filter(|&i| Self::filter(graph, query, i)) | ||
| .count() as u64 | ||
| }) | ||
| .sum() |
Contributor
There was a problem hiding this comment.
suggestion (performance): Avoid double iteration when computing totals for paginated queries to reduce cost on large graphs
When paginated.total() is true, this now walks all graphs twice: once in count_matching_nodes, and again in collect_graph_paged to build the page. On large graphs this doubles the dominant cost. Consider deriving the total from the same matching sequence used in collect_graph_paged (e.g., via a shared helper that returns both items and total, or by having collect_graph_paged optionally compute/return the total) so you only traverse and filter the nodes once per request.
Suggested implementation:
})
}To fully adopt the "single traversal" approach and remove the double walk:
- Locate the function that builds paginated results (likely named something like
collect_graph_pagedor similar) inmodules/analysis/src/service/mod.rs. - Remove its dependency on
count_matching_nodes:- Delete any calls to
Self::count_matching_nodes(query, graphs)(or similar).
- Delete any calls to
- In the body of
collect_graph_paged, where you currently iterate over graphs/nodes to produce theitemsfor the current page:- Introduce a
totalcounter initialized to0u64. - For each node that passes
Self::filter(graph, query, i):- Increment
totalby 1. - Apply pagination (offset/limit) logic to decide whether to push that node into the
itemscollection.
- Increment
- This way, you build the page and the total in the same loop.
- Introduce a
- Update the return type or construction of the paginated response:
- If the API allows, have
collect_graph_pagedreturn bothitemsandtotal(or construct a struct that contains both) so callers can access the total without an extra traversal. - Only compute/attach
totalwhenpaginated.total()(or equivalent flag) is true, to avoid unnecessary counting when totals are not requested.
- If the API allows, have
- Ensure there are no remaining references to
count_matching_nodesanywhere in the codebase; if there are, update them to use the new "single traversal" logic described above.
These changes will ensure that totals for paginated queries are derived during the same pass that builds the page, avoiding the double iteration and associated cost on large graphs.
Comment on lines
+575
to
+580
| stream::iter(matching) | ||
| .map(|(graph, node_index, package_node)| { | ||
| let create = create.clone(); | ||
| create(graph, node_index, package_node) | ||
| }) | ||
| .buffer_unordered(concurrency) |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Using buffer_unordered may break deterministic ordering of nodes within a page
buffer_unordered can change the order of results based on completion timing, so the final Vec may no longer be sorted by (sbom_id, node_id). If callers depend on stable pagination (consistent page contents or resuming by offset), this non-determinism could cause subtle bugs. Consider using buffered or re-sorting after collection to retain deterministic ordering while still leveraging concurrency.
Comment on lines
457
to
+459
| let distinct_sbom_ids = sbom::Entity::find() | ||
| .filter(sbom::Column::SbomId.in_subquery(subquery)) | ||
| .order_by_asc(sbom::Column::SbomId) |
Contributor
There was a problem hiding this comment.
suggestion (performance): Ordering by SbomId at the database layer may have indexing implications
This adds deterministic ordering but may introduce an extra sort on the DB side if there’s no index on SbomId (or if it doesn’t match the PK/index). Please check whether an appropriate index exists or whether this ordering can reuse an existing one. If not, and the number of distinct_sbom_ids is usually small, consider sorting client-side instead.
Suggested implementation:
) -> Result<Vec<(Uuid, Arc<PackageGraph>)>, Error> {
// Fetch distinct_sbom_ids without imposing a DB-side sort; we sort client-side below.
let mut distinct_sbom_ids = sbom::Entity::find()
.filter(sbom::Column::SbomId.in_subquery(subquery))
.select()
.all(connection)
.await?;
// Ensure deterministic ordering by sorting in memory by SbomId.
distinct_sbom_ids.sort_by_key(|model| model.sbom_id);- This change assumes the SeaORM model for the
sbomtable exposes the primary key/column assbom_id. If the field name differs (e.g.sbom_idis nested or renamed), adjust the closure insort_by_keyaccordingly. - If
distinct_sbom_idsis later used as an iterator or converted into another structure (e.g. mapping to(Uuid, Arc<PackageGraph>)), no other changes should be needed, since the vector is now deterministically ordered before subsequent processing.
Comment on lines
+976
to
+985
| async fn test_pagination_cross_graph(ctx: &TrustifyContext) -> Result<(), anyhow::Error> { | ||
| ctx.ingest_documents([ | ||
| "spdx/quarkus-bom-3.2.11.Final-redhat-00001.json", | ||
| "spdx/quarkus-bom-3.2.12.Final-redhat-00002.json", | ||
| ]) | ||
| .await?; | ||
|
|
||
| let service = AnalysisService::new(AnalysisConfig::default(), ReadOnly::new(ctx.db.clone())); | ||
|
|
||
| // Given: a query that matches nodes across both SBOMs |
Contributor
There was a problem hiding this comment.
issue (testing): The uniqueness check only uses sbom_id, which may be too coarse and can make the test misleading or flaky if multiple matches exist in the same SBOM.
This test currently deduplicates only by sbom_id:
let unique: std::collections::HashSet<_> = collected_sbom_ids.iter().collect();
assert_eq!(unique.len(), total, "no duplicate items across pages");If multiple nodes from the same SBOM match, this will incorrectly treat distinct items as duplicates and can both fail valid pagination and miss true (sbom_id, node_id) duplicates.
To better assert "no duplicate items, no gaps", collect and deduplicate on (sbom_id, node_id) instead, e.g.:
let mut collected = Vec::new();
...
collected.push((item.base.sbom_id.clone(), item.base.node_id.clone()));
...
let unique: HashSet<_> = collected.iter().cloned().collect();
assert_eq!(unique.len(), total);This makes the test robust when multiple nodes per SBOM are returned.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2446 +/- ##
==========================================
+ Coverage 71.52% 71.62% +0.10%
==========================================
Files 453 453
Lines 27363 27462 +99
Branches 27363 27462 +99
==========================================
+ Hits 19572 19671 +99
+ Misses 6668 6660 -8
- Partials 1123 1131 +8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
|
/scale-test |
|
🛠️ Scale test has started! Follow the progress here: Workflow Run |
Goose ReportGoose Attack ReportPlan Overview
Request Metrics
Response Time Metrics
Status Code Metrics
Transaction Metrics
Scenario Metrics
Error Metrics
📄 Full Report (Go to "Artifacts" and download report) |
Contributor
|
The scale test reports improvements for rest api endpoints but results are mixed eg. max time (95 percentile) increases but we also see a higher RPS which is positive - however the slow rest api performance is negatively impacted. At face value the impact is >25% improvement (that is prob biased by the perf tests, improvement might be better then that). I think this PR is definitely an improvement we want, though needs more analysis (I will run newer perf tests and look more at the code changes) ... lets pick it up next week - I do not think its a candidate yet for 0.5.z. |
Contributor
|
/perf-test |
|
🛠️ Perf test has started! Follow the progress here: Workflow Run |
Perf Test Report (Locust)
📄 Full Report (Go to "Artifacts" and download report) |
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
3 participants
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.
Summary
collect_graph_paged, eliminating the timeout caused by hydrating thousands of nodes only to discard most of them.BTreeSetfor ranked SBOM IDs and addsorder_by_asc(sbom_id)to subquery-based loading, ensuring stable pagination across requests.count_matching_nodesfor efficient total counting without hydrating any nodes.Test plan
test_pagination_limits_hydration— verifies only the requested number of nodes are hydrated whiletotalreflects all matchestest_pagination_offset— verifies different offsets produce different results and union matches combined querytest_pagination_cross_graph— verifies pagination spans multiple SBOM graphs without duplicates or gapstest_pagination_through_endpoint— verifies pagination via HTTP returns correct totals and distinct pagesSummary by Sourcery
Apply pagination at the graph index level before node hydration to improve performance and provide deterministic, cross-graph component retrieval with accurate totals.
New Features:
Enhancements:
Tests: