Open
Conversation
…zation initializeRankers() previously used Walk() which deserialized the full protobuf serialized column for every row. With ~15M components in production, UnmarshalVTUnsafe zero-copy deserialization pinned pgx row buffers in memory (~6.7GB retained heap), causing Central OOM. Replace Walk() with RunSelectRequestForSchemaFn using a ComponentRiskView struct that fetches only the id and riskscore columns, following the established pattern from imagev2/datastore. Uses streaming callback to avoid intermediate slice allocation for 15M rows. Benchmark (500 components): 22% faster, 55% less memory per operation. Production impact is significantly larger due to multi-KB serialized blobs. This code was partially generated by AI. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughDatastore constructor now accepts a leading Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test/App
participant DS as Datastore
participant DB as PostgreSQL
Test->>DS: New(db, storage, risks, ranker)
DS->>DS: initializeRankers()
DS->>DB: SELECT component_id, component_risk_score<br/>FROM image_component_v2
DB-->>DS: ResultSet (ComponentRiskView rows)
DS->>DS: Iterate rows and update ranker
DS-->>Test: Datastore ready
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
central/imagecomponent/v2/datastore/datastore.go (1)
31-35: Avoid makingdbandstorageseparate sources of truth.
initializeRankers()reads throughdb, while the rest of the datastore reads throughstorage. That makes it possible to construct a datastore whose ranker is initialized from a different database than the one serving reads. Prefer collapsing this to one dependency—either buildstoragefromdbinsideNew, or move the lightweight select behind the store and drop the extradbparameter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@central/imagecomponent/v2/datastore/datastore.go` around lines 31 - 35, The constructor New currently accepts both db and storage, causing initializeRankers to read from db while other methods use storage; pick a single source of truth and unify them: either (A) remove the db parameter and update initializeRankers to use the provided storage (modify initializeRankers to call storage methods on datastoreImpl instead of direct db queries), or (B) construct the pgStore.Store from the db inside New (create storage via the store constructor using db and assign only storage to datastoreImpl) and stop passing storage in; update New, datastoreImpl, and initializeRankers accordingly so all reads use the same store symbol (storage) and drop the extra db dependency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@central/imagecomponent/v2/datastore/datastore.go`:
- Around line 31-35: The constructor New currently accepts both db and storage,
causing initializeRankers to read from db while other methods use storage; pick
a single source of truth and unify them: either (A) remove the db parameter and
update initializeRankers to use the provided storage (modify initializeRankers
to call storage methods on datastoreImpl instead of direct db queries), or (B)
construct the pgStore.Store from the db inside New (create storage via the store
constructor using db and assign only storage to datastoreImpl) and stop passing
storage in; update New, datastoreImpl, and initializeRankers accordingly so all
reads use the same store symbol (storage) and drop the extra db dependency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 418fd758-49cf-4031-8523-694b9edd7459
📒 Files selected for processing (6)
central/graphql/resolvers/test_setup_utils.gocentral/imagecomponent/v2/datastore/datastore.gocentral/imagecomponent/v2/datastore/datastore_bench_postgres_test.gocentral/imagecomponent/v2/datastore/datastore_impl.gocentral/imagecomponent/v2/datastore/singleton.gocentral/imagecomponent/v2/views/component_risk_view.go
Contributor
|
Images are ready for the commit at 620d9bb. To use with deploy scripts, first |
Two test files were not updated when the New() signature gained a postgres.DB parameter. This caused compilation failures in CI (style-check, golangci-lint, go-postgres, go-bench). Partially generated by AI. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #19739 +/- ##
==========================================
- Coverage 49.60% 49.60% -0.01%
==========================================
Files 2756 2756
Lines 208036 208050 +14
==========================================
+ Hits 103194 103195 +1
- Misses 97183 97194 +11
- Partials 7659 7661 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
The benchmark was discarding id/score values without adding them to a ranker, so it did not exercise the memory pinning behavior of UnmarshalVTUnsafe strings stored as map keys in idToScore. Partially generated by AI. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@dashrews78: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
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.
Description
Updating the initialization of the component rankers because the way it works with the walk was slow and essentially was holding a lot of memory unnecessarily.
imagecomponent/v2/datastore.initializeRankers() walks the entire image_components_v2 table (~15M rows in production), deserializing every full serialized protobuf blob via
UnmarshalVTUnsafe just to extract two fields: id and risk_score.
Because UnmarshalVTUnsafe performs zero-copy deserialization, the resulting protobuf string fields (including the component ID) hold direct references into the pgx-allocated
[]byte row buffers. When the ranker stores the ID as a map key in idToScore, it pins the entire serialized row buffer in memory for the lifetime of the process.
Benchmark Results
Benchmark compares the current
Walk-based ranker initialization (full protobuf deserialization) against the newSelectRiskViewapproach (fetches onlyidandriskscorecolumns).
BenchmarkInitializeRankers/Walk-12 210 5709767 ns/op 338654 B/op 1243 allocs/op
BenchmarkInitializeRankers/Walk-12 222 5441142 ns/op 338608 B/op 1243 allocs/op
BenchmarkInitializeRankers/Walk-12 202 5737878 ns/op 338808 B/op 1247 allocs/op
BenchmarkInitializeRankers/Walk-12 206 5612813 ns/op 338728 B/op 1250 allocs/op
BenchmarkInitializeRankers/Walk-12 216 5732239 ns/op 338735 B/op 1250 allocs/op
BenchmarkInitializeRankers/SelectRiskView-12 236 5096242 ns/op 245012 B/op 3829 allocs/op
BenchmarkInitializeRankers/SelectRiskView-12 236 5071273 ns/op 245004 B/op 3829 allocs/op
BenchmarkInitializeRankers/SelectRiskView-12 235 5057107 ns/op 244997 B/op 3829 allocs/op
BenchmarkInitializeRankers/SelectRiskView-12 232 5142105 ns/op 244969 B/op 3829 allocs/op
BenchmarkInitializeRankers/SelectRiskView-12 222 5292091 ns/op 245011 B/op 3829 allocs/op
Run with 500 test components using small serialized payloads (~200 bytes). In production (~15M components with multi-KB serialized blobs), the memory improvement is dramatically
larger — the
serializedcolumn is never fetched at all, eliminating the ~6.7GB of retained heap from pinned pgx row buffers.User-facing documentation
Testing and quality
Automated testing
How I validated my change
unit test and benchmarks