Conversation
ToRelationalSchema parsed the raw report into a vuln.Report, then handed the raw string to toSchema, which parsed the whole document a second time. Both trees stayed reachable through the heaviest stretch of the conversion, where the CVE id set, the existing records loaded from the database, the item index and the new ORM records all pile up on top of them. Parsed structs run a few times the size of the JSON they came from, so on a report of a few thousand vulnerabilities the second tree is tens of MB that nothing needs. Pass the parsed report to toSchema instead. Peak live heap for a 5000 vulnerability report falls from 3.1x the raw report to 1.2x. toSchema builds its item index locally rather than through Report.GetVulnerabilityItemList, which caches the index on the report and would keep every item reachable after the caller drops Vulnerabilities. The new test measures peak live heap around a conversion with the database faked out, and fails on the two-parse version. Refs #478 Signed-off-by: Prasanth Baskar <prasanth@8gears.com>
|
Warning Review limit reachedNext included review available in 45 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Contributor
|
This change may need patch-release backports. Comment with one of these commands to open a cherry-pick PR:
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
|
Preview images for this PR are available in
Verify a preview image: Verify the SBOM attestation: |
bupd
added
prio/P1
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
2 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.
Refs #478.
ToRelationalSchemaparses the raw report into avuln.Report, then hands thesame raw string to
toSchema, which parses the whole document again into asecond complete tree:
Both trees stay reachable through the heaviest stretch of the conversion, where
the CVE id set, the existing records loaded from the database, the item index and
the new ORM records all pile up on top of them. Parsed structs run several times
the size of the JSON they came from, so on a report of a few thousand
vulnerabilities the second tree is tens of MB that nothing ever reads.
This passes the parsed report to
toSchemainstead.Effect
Peak live heap across one conversion, database faked out, measured at GOGC=1 so
HeapAlloctracks reachable bytes:For scale: one artifact on a production tenant returned 5,180 vulnerability
records, and the report in #478 was 10-40 MB.
toSchemaalso builds its item index locally rather than throughReport.GetVulnerabilityItemList, which caches the index on the report and wouldotherwise keep every item reachable after the caller clears
Vulnerabilities.Test
TestToRelationalSchemaDoesNotParseTheReportTwicemeasures peak live heap arounda conversion and fails on the unpatched code (3.1x against a 2.0x bound), so
it guards the regression rather than just passing. The fakes are hand-rolled
rather than mockery, whose argument capture would be most of what the numbers
measured. Runs in the pure lane — no database needed.
Rejected along the way
Decoding with
json.NewDecoderover astrings.Reader, to avoid the[]byte(reportData)copy, allocates more rather than less: on a singlemulti-MB document the decoder's internal buffer grows by reallocation and costs
more than the one copy it saves (27 MB to 31 MB total allocation, measured). Not
done, and the reason is in a code comment so it does not get retried.
Scope
This is item 2 of the five ranked in #478 and does not close it on its own. Items
1, 4 and 5 stand:
GOMEMLIMITat the chart level, batching the per-recordinserts, and streaming decode. Item 3, bounding concurrent conversions, is better
handled as part of #862, where the same
MaxCurrency() == 0is the root; see theassessment posted there so the two issues do not grow separate concurrency knobs.