[bug](hgraph): include reader caches in memory usage - #2498
Conversation
|
/label S-waiting-on-review |
|
Automated pull request review failed. Review effort: git exited with 128: fatal: destination path '/tmp/vsag-pull-request-reviews/6b56ae07-e6d0-48a1-99ac-78d37127008e/source' already exists and is not an empty directory. No GitHub review was submitted. |
Merge Protections🟢 All 3 merge protections satisfied — ready to merge. Show 3 satisfied protections🟢 Require kind label
🟢 Require version label
🟢 Require linked issue for feature/bug PRs
|
There was a problem hiding this comment.
Code Review
This pull request introduces the ability to track and report memory usage for custom Reader implementations within the HGraph index. It adds a GetMemoryUsage() method to the Reader interface, updates HGraph to include reader memory in its overall and detailed memory usage statistics, and updates the documentation and tests accordingly. The feedback suggests adding a defensive null check for high_precise_codes_ in SetPreciseCodesIO to prevent potential null pointer dereferences.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| void | ||
| HGraph::SetPreciseCodesIO(const std::shared_ptr<Reader>& reader) { | ||
| auto reader_param = std::make_shared<ReaderIOParameter>(); | ||
| reader_param->reader = reader; | ||
| high_precise_codes_->InitIO(reader_param); | ||
| this->precise_reader_ = reader; | ||
| } |
There was a problem hiding this comment.
To prevent a potential null pointer dereference, we should add a defensive null check for high_precise_codes_ before calling InitIO. Although the current serialization code path only calls SetPreciseCodesIO when has_precise_reorder() is true, adding this check makes the method robust against other callers or future refactorings.
| void | |
| HGraph::SetPreciseCodesIO(const std::shared_ptr<Reader>& reader) { | |
| auto reader_param = std::make_shared<ReaderIOParameter>(); | |
| reader_param->reader = reader; | |
| high_precise_codes_->InitIO(reader_param); | |
| this->precise_reader_ = reader; | |
| } | |
| void | |
| HGraph::SetPreciseCodesIO(const std::shared_ptr<Reader>& reader) { | |
| auto reader_param = std::make_shared<ReaderIOParameter>(); | |
| reader_param->reader = reader; | |
| if (this->high_precise_codes_ != nullptr) { | |
| this->high_precise_codes_->InitIO(reader_param); | |
| } | |
| this->precise_reader_ = reader; | |
| } |
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Updates HGraph memory reporting to include resident memory held by retained Reader implementations (e.g., caches), and keeps totals/detailed breakdowns accurate without double-counting shared readers.
Changes:
- Add
Reader::GetMemoryUsage()hook (default 0) and implement it in the test reader fixture. - Track shared and precise readers in HGraph and include them in total and detailed memory usage.
- Add a regression test and update English/Chinese memory documentation.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_hgraph.cpp | Adds a functional regression test validating reader memory inclusion and SetImmutable snapshot refresh. |
| tests/fixtures/framework/test_reader.h | Extends TestReader API to accept/return memory usage. |
| tests/fixtures/framework/test_reader.cpp | Implements TestReader::GetMemoryUsage() and stores configured usage. |
| src/algorithm/hgraph/hgraph_serialize.cpp | Adds reader(s) to HGraph detailed memory breakdown, avoiding double counts. |
| src/algorithm/hgraph/hgraph.h | Stores retained reader pointers and declares HGraph::GetMemoryUsage() override. |
| src/algorithm/hgraph/hgraph.cpp | Refreshes memory snapshot on SetImmutable() and includes reader/pool usage in totals. |
| include/vsag/readerset.h | Introduces Reader::GetMemoryUsage() virtual hook with thread-safety requirement. |
| docs/docs/zh/src/advanced/memory.md | Documents reader memory inclusion and new detail keys in Chinese. |
| docs/docs/en/src/advanced/memory.md | Documents reader memory inclusion and new detail keys in English. |
| * @brief Constructs a TestReader from a Binary object. | ||
| * @param binary The binary data to wrap for reading. | ||
| */ |
| constexpr int64_t dim = 16; | ||
| constexpr int64_t count = 200; | ||
| const auto build_param = R"({ | ||
| "dtype": "float32", | ||
| "metric_type": "l2", | ||
| "dim": 16, |
| REQUIRE(immutable_memory >= detail_sum); | ||
| REQUIRE(immutable_memory - detail_sum < 4096); |
| `label_table`, `high_precise_codes`, `extra_infos`, `raw_vector`, `reader`, and | ||
| `precise_reader`. Reader keys are present only when the corresponding retained reader exists. |
| `extra_infos`、`raw_vector`、`reader` 和 `precise_reader`。仅在索引持有对应 Reader 时才会 | ||
| 返回 Reader 相关项。SINDI 返回空 map,其他索引类型默认抛出异常。 |
vsag-bot
left a comment
There was a problem hiding this comment.
Automated inline review completed.
Review effort: medium (139 changed lines across 9 files).
Submitted 1 inline comment.
Reviewed commit 910d004.
| * @return Resident memory usage in bytes. The default implementation returns zero. | ||
| */ | ||
| [[nodiscard]] virtual uint64_t | ||
| GetMemoryUsage() const { |
There was a problem hiding this comment.
[P1] Preserve binary compatibility for existing Reader subclasses
Adding this virtual slot changes the public Reader vtable. An application can load the same major-version VSAG shared library while still constructing a custom Reader compiled against v1.0.0; when HGraph::GetMemoryUsage() dispatches this new slot, that object's old vtable has no corresponding entry, causing undefined behavior or a crash. The documented requirement to rebuild callers contradicts the repository's backward-compatible minor/patch version policy and is not enforced by the unchanged major-version SONAME. Expose optional accounting without extending this vtable (for example through a separate discoverable interface), or make this a major ABI transition.
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
/version 1.1 |
Signed-off-by: jc543239 <jc543239@antgroup.com> Assisted-by: Codex:gpt-5
910d004 to
186c1b0
Compare
Change Type
Linked Issue
What Changed
Reader::GetMemoryUsage()hook for resident reader-owned caches.double-counting a shared reader.
reflected immediately.
SetImmutable()releases neighbor mutex storage.Test Evidence
make fmtmake lintmake testmake cov, run tests, and collect coverageTest details:
Compatibility Impact
custom Reader implementations remain source-compatible, but should be rebuilt for binary
compatibility.
readers and tracks query-time pool growth.
Performance and Concurrency Impact
readers are added to
GetMemoryUsage(); search paths are unchanged.documented; the default implementation is stateless.
Documentation Impact
README.mdDEVELOPMENT.mdCONTRIBUTING.mddocs/docs/{en,zh}/src/advanced/memory.mdRisk and Rollback
Reader API.
Checklist
kind/bugandkind/feature; see "Linked Issue" above)[skip ci]prefix)