perf: Improves a bit the recommend endpoint #2183
Open
+37
−10
Conversation
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
Contributor
Reviewer's GuideOptimizes the recommend endpoint by batching related entity loading (organizations and statuses) and reusing them via in-memory maps instead of issuing per-row queries, and adjusts VersionedPurlStatus construction to use these preloaded models. Sequence diagram for batched loading in recommend endpointsequenceDiagram
participant RecommendEndpoint
participant VersionedPurlAdvisory
participant StatusEntity
participant AdvisoryEntity
participant OrganizationEntity
participant StatusModelEntity
RecommendEndpoint->>VersionedPurlAdvisory: build_from_purl_statuses(vulns, statuses, tx)
Note over VersionedPurlAdvisory,StatusEntity: Previously: per row status lookup
VersionedPurlAdvisory->>StatusEntity: load_one(statuses)
StatusEntity-->>VersionedPurlAdvisory: status_models
Note over VersionedPurlAdvisory,AdvisoryEntity: Previously: per row organization lookup
VersionedPurlAdvisory->>AdvisoryEntity: load_one(advisories)
AdvisoryEntity-->>VersionedPurlAdvisory: advisory_models
VersionedPurlAdvisory->>OrganizationEntity: load_one(advisory_models)
OrganizationEntity-->>VersionedPurlAdvisory: organization_models
VersionedPurlAdvisory->>VersionedPurlAdvisory: build org_map by advisory id
VersionedPurlAdvisory->>VersionedPurlAdvisory: build status_map by status id
loop for each vuln, advisory, purl_status
VersionedPurlAdvisory->>VersionedPurlAdvisory: lookup status_model in status_map
VersionedPurlAdvisory->>VersionedPurlStatus: from_entity(vuln, purl_status, status_model, tx)
VersionedPurlStatus-->>VersionedPurlAdvisory: VersionedPurlStatus
VersionedPurlAdvisory->>VersionedPurlAdvisory: lookup organization in org_map
VersionedPurlAdvisory->>VersionedPurlAdvisory: push into results
end
VersionedPurlAdvisory-->>RecommendEndpoint: results
Updated class diagram for VersionedPurlAdvisory and VersionedPurlStatusclassDiagram
class VersionedPurlAdvisory {
+AdvisoryHead head
+Vec~VersionedPurlStatus~ status
+build_from_purl_statuses(vulns, statuses, tx) Result_Vec_VersionedPurlAdvisory_
}
class VersionedPurlStatus {
+VulnerabilityHead vulnerability
+String status
+from_entity(vuln, package_status, status_model, tx) Result_VersionedPurlStatus_
}
class AdvisoryHead
class VulnerabilityHead
class vulnerability_Model
class advisory_Model
class organization_Model
class status_Model
class purl_status_Model
VersionedPurlAdvisory o-- AdvisoryHead
VersionedPurlAdvisory o-- VersionedPurlStatus
VersionedPurlStatus o-- VulnerabilityHead
VersionedPurlStatus ..> status_Model : uses
VersionedPurlStatus ..> purl_status_Model : uses
VersionedPurlAdvisory ..> advisory_Model : batches
VersionedPurlAdvisory ..> organization_Model : batches
VersionedPurlAdvisory ..> status_Model : batches
VersionedPurlAdvisory ..> purl_status_Model : iterates
advisory_Model ..> organization_Model : related
purl_status_Model ..> status_Model : references
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 there - I've reviewed your changes - here's some feedback:
- In
from_entity, the_package_statusparameter is now unused; consider removing it from the signature (and all call sites) to simplify the API and avoid confusion about its purpose. - The construction of
org_mapandstatus_mapperforms a full clone of each model (org.clone(),status.clone()); if these structs are large, you may want to store references or lightweight identifiers instead to reduce allocation and copying overhead. - When creating
results, you can pre-allocate withVec::with_capacity(vulns.len())(or a tighter bound) to avoid repeated reallocations in this hot path.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `from_entity`, the `_package_status` parameter is now unused; consider removing it from the signature (and all call sites) to simplify the API and avoid confusion about its purpose.
- The construction of `org_map` and `status_map` performs a full clone of each model (`org.clone()`, `status.clone()`); if these structs are large, you may want to store references or lightweight identifiers instead to reduce allocation and copying overhead.
- When creating `results`, you can pre-allocate with `Vec::with_capacity(vulns.len())` (or a tighter bound) to avoid repeated reallocations in this hot path.
## Individual Comments
### Comment 1
<location> `modules/fundamental/src/purl/model/details/versioned_purl.rs:120-123` </location>
<code_context>
+ let status_models = statuses.load_one(status::Entity, tx).await?;
+
+ // Create a HashMap for fast status lookup by status ID
+ let status_map: HashMap<Uuid, Option<status::Model>> = statuses
+ .iter()
+ .zip(status_models.iter())
+ .map(|(purl_status, status)| (purl_status.status_id, status.clone()))
+ .collect();
+
</code_context>
<issue_to_address>
**suggestion (performance):** Status lookups can stay index-based instead of going through a HashMap.
`statuses` and `status_models` are already positionally aligned from `load_one`, and you iterate `statuses` in order below. Rather than building a `HashMap<Uuid, Option<status::Model>>`, you can keep a `Vec<Option<status::Model>>` and use an index or `zip` in the later loop to access the matching `status_model`, avoiding the extra hash lookups and clones.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
helio-frota
force-pushed
the
rec-perf
branch
from
a57b1f3 to
aba2b68
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2183 +/- ##
=======================================
Coverage ? 68.25%
=======================================
Files ? 376
Lines ? 21207
Branches ? 21207
=======================================
Hits ? 14474
Misses ? 5863
Partials ? 870 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
Author
|
/scale-test |
|
🛠️ Scale test has started! Follow the progress here: Workflow Run |
Assisted-by: Claude Code
helio-frota
force-pushed
the
rec-perf
branch
from
aba2b68 to
26e0503
Compare
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) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Assisted-by: Claude Code
Before:
After:
Before:
https://gist.github.com/helio-frota/25af13c01066bde8c506bee4a7b32b9e
After:
https://gist.github.com/helio-frota/0d17454a2ef37c5f3cbe41f64aec6073
Summary by Sourcery
Optimize advisory recommendation query by batching related entity loading and reusing pre-fetched status and organization data.
Enhancements: