Conversation
sozercan
requested review from
ashnamehrotra,
jeremyrickard,
omercnet and
robert-cronin
as code owners
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1638 +/- ##
==========================================
+ Coverage 44.04% 44.85% +0.80%
==========================================
Files 58 58
Lines 10282 10557 +275
==========================================
+ Hits 4529 4735 +206
- Misses 5436 5466 +30
- Partials 317 356 +39
🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR optimizes CPU/allocation hot paths across Copa's report parsing, package-manager validation/manifest parsing, platform/BuildKit helpers, and bulk patch tag/report handling. It replaces regex- and strings.Split/bufio.Scanner-based code with lower-allocation equivalents (strings.Cut, hand-rolled scanners, precomputed lookup maps/sets, cached templates), while aiming to keep observable behavior identical. It also adds an optional ParseWithPackageTypes fast path so the Trivy parser can skip work for filtered pkg-types, and adds focused microbenchmarks and a couple of regression tests (signed patch-tag suffixes, escaped-slash ArtifactName).
Changes:
- Rewrote version selection/comparison, package-name validation, and OS manifest parsing with allocation-conscious implementations while preserving prior semantics.
- Added
ParseWithPackageTypes(via a new optional interface) to avoid computing OS/library updates that pkg-types filtering would discard. - Added benchmark suites for
pkg/report,pkg/pkgmgr,pkg/patch,pkg/buildkit, andpkg/bulk, plus regression tests inpkg/bulk/skip_test.go.
Show a summary per file
| File | Description |
|---|---|
| pkg/report/trivy.go | Replaces parseVersion/compareVersions/getHighestVersion/parseVersionParts with allocation-free candidate structs; adds ParseWithPackageTypes with includeOS/includeLibrary gating; hoists regexes/special-package map to package vars. |
| pkg/report/report.go | Adds optional packageTypeScanReportParser interface and dispatches to ParseWithPackageTypes when supported. |
| pkg/report/trivy_benchmark_test.go | New benchmarks for optimal fixed-version selection and large Trivy report parsing. |
| pkg/pkgmgr/pkgmgr.go | Replaces regex package-name validation with isValidOSPackageName; adds shared splitManifestLines; presizes maps/slices. |
| pkg/pkgmgr/rpm.go | Cut-based parseRPMTools/parseManifestFile, index-based version extraction in validateRPMPackageVersions, sort.Slice/sort.Strings. |
| pkg/pkgmgr/pacman.go | Uses splitManifestLines and index-based prefix/version extraction; non-stable sorts. |
| pkg/pkgmgr/dpkg.go | strings.Cut-based manifest field parsing with presized map. |
| pkg/pkgmgr/apk.go | Uses splitManifestLines and index-based prefix/version extraction; non-stable sorts. |
| pkg/pkgmgr/pkgmgr_bench_test.go | New benchmarks for validation/parse helpers. |
| pkg/patch/platform.go | Precomputes arch-tag suffixes and normalized platform specs; map-keyed filterPlatforms; string-concat archTag. |
| pkg/patch/platform_benchmark_test.go | New benchmarks for platform helpers. |
| pkg/buildkit/buildkit.go | Extracts descriptorFromRawManifest/platformImageReferenceFromManifest; typed manifest-list decoding with startsWithJSONArray guard and hex digest. |
| pkg/buildkit/buildkit_benchmark_test.go | New unit tests + benchmarks for manifest descriptor/reference helpers. |
| pkg/bulk/skip.go | Caches arch-tag suffix set; replaces regex tag matching with numeric parsing; adds a hand-rolled top-level JSON string extractor guarded by json.Valid; adds fast-path lookup. |
| pkg/bulk/discover.go | Replaces excludeTags/semver.Collection with a set + custom tagVersion parsing/compare and semver.NewVersion fallback. |
| pkg/bulk/engine.go | Caches tag templates via sync.Map; default-tag fast path; string-concat repository/summary building. |
| pkg/bulk/skip_test.go | Regression tests for signed patch-tag suffixes and escaped-slash ArtifactName. |
| pkg/bulk/bulk_benchmark_test.go | New benchmarks for discovery, report index build/lookup, and summary printing. |
Review details
- Files reviewed: 18/18 changed files
- Comments generated: 0
- Review effort level: Medium
Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
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.
Summary
ArtifactNamevalues.Impact
Representative benchmark improvements from the final runs include:
Full benchmark notes and raw progress tracking were kept in
/tmp/perf-copa.mdduring development.Validation
go test ./pkg/... -count=1git diff --checkpkg/reportpkg/pkgmgrpkg/patch/pkg/buildkitpkg/bulkpython3 /Users/sozercan/.codex/skills/autoreview/scripts/autoreview --mode localautoreview clean: no accepted/actionable findings reportedNotes
make lintwas run; it still reports pre-existing repository-wide lint issues unrelated to this change set (mostlygoconst, plus existinggosec,govet,gocritic, andstaticcheckfindings). New visible lint issues introduced while developing this change were fixed before commit.