[WIP]fix(eval): correct peak memory measurement - #2522
Conversation
Signed-off-by: jc543239 <jc543239@antgroup.com> Assisted-by: Codex:gpt-5
|
/label status/blocked |
|
Automated pull request review failed. Review effort: git exited with 128: Cloning into '/tmp/vsag-pull-request-reviews/f4b26dd0-a029-46c5-8cd1-b1edc7bd7277/source'... 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.
Pull request overview
Fixes incorrect eval_performance peak memory reporting by replacing the underflow-prone RSS delta calculation with a phase-scoped background RSS sampler, while also tightening evaluation phase boundaries and enriching JSON diagnostics. This improves correctness of memory metrics (notably avoiding the multi-terabyte underflow artifact) without changing VSAG core APIs/ABI.
Changes:
- Reworked
MemoryPeakMonitorto sample process RSS in a restartable background thread, clamp negative deltas to zero, and emit detailed JSON diagnostics while preservingmemory_peak(build/search). - Refactored search evaluation passes to isolate monitored query phases from KNN statistics collection via
SearchPassRunner. - Added unit/regression coverage for the new monitor/pass scheduling and expanded CI/docs to reflect the updated metrics.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tools/eval/monitor/memory_peak_monitor.h | Redesigns the monitor API/state for a background RSS sampler and richer result fields. |
| tools/eval/monitor/memory_peak_monitor.cpp | Implements platform RSS readers (Linux/macOS), sampling loop, clamped deltas, and expanded JSON output. |
| tools/eval/monitor/memory_peak_monitor_test.cpp | Adds regression/unit tests for baseline/peak/delta behavior, restartability, and formatting. |
| tools/eval/CMakeLists.txt | Adds search_pass_runner.cpp to the eval tool build sources. |
| tools/eval/case/search_pass_runner.h | Introduces a helper to run monitored search passes with optional memory sampling coordination. |
| tools/eval/case/search_pass_runner.cpp | Implements pass scheduling to ensure memory sampling aligns with the intended query pass boundaries. |
| tools/eval/case/search_pass_runner_test.cpp | Adds tests validating pass order, memory-on/off behavior, and exception unwinding semantics. |
| tools/eval/case/search_eval_case.h | Extends constructor to optionally reuse an already-loaded dataset; adds helpers for pass separation. |
| tools/eval/case/search_eval_case.cpp | Uses SearchPassRunner to separate monitored query passes from statistics collection; adds index_memory(B) reporting. |
| tools/eval/case/eval_case.h | Extends factory/ctor to accept an optional shared dataset instance. |
| tools/eval/case/eval_case.cpp | Implements dataset reuse through MakeInstance and the base EvalCase constructor. |
| tools/eval/case/build_search_eval_case.h | Reuses one dataset across build+search and releases build index before creating/deserializing the search index. |
| tools/eval/case/build_eval_case.h | Extends build case constructor to accept an optional shared dataset. |
| tools/eval/case/build_eval_case.cpp | Tightens build measurement boundary around Index::Build() and adds index_memory(B) reporting. |
| tests/CMakeLists.txt | Adds eval monitor/pass sources to unittests (plus Threads/json linkage) to run new [eval] tests. |
| docs/docs/en/src/resources/eval.md | Documents the new RSS sampling semantics, phase boundaries, and additional JSON fields. |
| docs/docs/zh/src/resources/eval.md | Chinese documentation parity for the new memory metrics and boundaries. |
| .github/workflows/pr-ci.yml | Ensures tools/** triggers C++ CI and runs eval-tagged tests on macOS ASan builds. |
| const auto sample = read_memory(); | ||
| std::lock_guard<std::mutex> lock(state_mutex_); | ||
| if (sample.available) { | ||
| absolute_peak_bytes_ = std::max(absolute_peak_bytes_, sample.bytes); | ||
| ++sample_count_; | ||
| return; | ||
| } |
|
/version 1.1 |
Change Type
Linked Issue
eval_performanceReports Incorrect Memory Usage (16777216.00 TB) #1109What Changed
memory_peak(build/search)while adding raw baseline, absolute peak, delta, availability, sample-count, and error fields; clamp negative deltas to zero.Index::Build()during build and the latency/QPS query pass during search; run KNN statistics outside monitored intervals.build,searchmode and release the build index before creating/deserializing the search index.index_memory(B)in addition to process RSS and existing memory details.Root Cause
The previous monitor subtracted two
uint64_tRSS page counts without validating reads or ordering. If the final RSS was below the constructor-time baseline, subtraction wrapped nearUINT64_MAX, producing16777216.00 TB. Build also sampled only after completion, search used a later intrusive query pass, andbuild,searchretained duplicate datasets and index objects.Test Evidence
make fmtmake lintmake testmake cov, run tests, and collect coverageTest details:
The eval tests also passed 50 consecutive repetitions. A temporary 20,000-vector HDF5
build,searchend-to-end run completed successfully and produced sane byte/RSS fields without the oversized TB value.Compatibility Impact
memory_peak(build/search)keys remain available; additional diagnostics are emitted in JSON.Performance and Concurrency Impact
Index::KnnSearch()implementation. When memory and latency are enabled together, one 5 ms RSS sampling thread runs concurrently and may add small benchmark noise; KNN statistics no longer contaminate latency/QPS measurements.Documentation Impact
README.mdDEVELOPMENT.mdCONTRIBUTING.mddocs/docs/en/src/resources/eval.md,docs/docs/zh/src/resources/eval.mdRisk and Rollback
af9b6b1eto restore the previous eval monitor and pass lifecycle.Checklist