[jac0626]

Summary

Backports #2449 to the 0.18 branch.

• replace the per-node visitation generation array with generation-stamped bitmap words
• preserve lazy reset semantics while reducing visited-list scratch memory
• include boundary, wraparound, zero-capacity, and randomized differential coverage
• retain the 0.18 branch's existing Catch2 include and GetMemoryUsage() return type

Validation

• make fmt with clang-format 15
• git diff --check
• compiled src/utils/visited_list.cpp and src/utils/visited_list_test.cpp with the CMake-generated compilation commands
• make test CASE='[ut][VisitedList]' was blocked before project compilation by external dependency initialization/download failures; CI will run the targeted and repository checks with its dependency cache

Backport of #2449.

Follow-up validation (2026-10-13)

The backport remains unchanged. Compared with landed #2449 (2d587f4e), the only differences in the three affected files are the intended 0.18 Catch2 include and GetMemoryUsage() return type. All six review suggestions have been answered and resolved with source and contract reasoning.

• The unchanged VisitedList and VisitedListPool tests pass in a focused GCC 11.4 / Catch2 3.7.1 harness with ASan/UBSan: four test cases, 118,681 assertions, zero failures. git diff --check passes. This is focused validation, not a full local regression or coverage run.
• CircleCI job 19243 fails HGraph RangeSearch filters reranked distances with 0 == 1, seed 496730723; all four visited-list cases pass in that job.
• An isolated copy of the existing HGraph regression, with its generated build parameters inlined, was run against head 26b88d0c and its exact parent 7e64ea07. Both pass with normal CPU affinity and both fail the same assertion with one-CPU affinity. The controls use GCC 11.4, Catch2 3.7.1, Sanitize (-O2), ASan/MKL off, and the same OpenBLAS 0.3.34 dependency. All objects affected by the visited-list header were recompiled for the parent comparison.

These controls establish an inherited scheduling-sensitive regression-test failure, consistent with the fixture problem tracked in #3045. The original CircleCI insertion schedule was not preserved, so this does not claim inspection of that job's graph. No unrelated HGraph implementation or fixture change is included in this backport.

One failed-job-only rerun was requested for the existing CircleCI workflow; CircleCI returned HTTP 403 (Permission denied), so no new run was started. Eight of nine required check contexts pass; the CircleCI context remains failed. Two human approvals are still required. This PR is not merge-ready.

[vsag-bot]

/label status/waiting-for-review
/waiting-on reviewer
/request-review @jiaweizone
/request-review @wxyucs
/request-review @inabao
/request-review @ShawnShawnYou

[gemini-code-assist]

Code Review

This pull request refactors the VisitedList class to use a bitwise representation with WordType and TagType, which significantly optimizes memory usage. It also introduces comprehensive unit tests covering word boundaries, memory usage, tag overflow, and randomized differential testing. The reviewer's feedback suggests replacing the alternative operator and with the standard logical operator && in visited_list.h to align with standard C++ coding practices.

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.

[jac0626]

/retest

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

@@            Coverage Diff             @@
##             0.18    #2451      +/-   ##
==========================================
- Coverage   91.40%   91.38%   -0.03%     
==========================================
  Files         335      335              
  Lines       20112    20127      +15     
==========================================
+ Hits        18384    18393       +9     
- Misses       1728     1734       +6     

Flag	Coverage Δ
cpp	91.38% <100.00%> (-0.03%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
common	86.03% <ø> (ø)
datacell	92.89% <ø> (-0.16%)	⬇️
index	91.08% <ø> (-0.43%)	⬇️
simd	100.00% <ø> (ø)

---

Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7e64ea0...26b88d0. Read the comment docs.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[LHT129]

Code review summary for PR #2451 (backport of #2449 to 0.18 branch):

Overview: This PR replaces the per-node uint16_t visitation array with a generation-stamped bitmap approach using uint64_t words + uint16_t tags per word. This significantly reduces memory usage for the visited list (from 2 * max_size bytes to 10 * word_count bytes, where word_count = ceil(max_size / 64)).

Files changed: visited_list.h, visited_list.cpp, visited_list_test.cpp (+148 / -18)

Review findings:

1. Correctness: The generation-stamped bitmap logic in Set() and Get() is correct. The tag-overflow path in Reset() properly clears tags and resets tag_ to 1. The zero-capacity early-return path in the constructor correctly avoids allocation and the destructor guards against null words_.

2. Memory safety: The single-buffer allocation (words_bytes + tags_bytes) and the split into words_ / tags_ pointers is sound. The tags_ offset (word_count_ * 8) is naturally 8-byte aligned, so uint16_t accesses have no alignment issues.

3. Test coverage: Comprehensive tests cover word boundaries (0, 1, 63, 64, 65, size-1), repeated sets, tag overflow (full uint16_t range), zero-size edge case (verifies no allocation), memory usage calculation, and a randomized differential test against a vector<bool> reference across multiple sizes and 20k operations per size.

4. Performance: The Prefetch() method correctly prefetches tags_ (the first memory access in Get()). The bitmap approach reduces memory footprint and improves cache locality for large visited lists.

Conclusion: No blocking issues found. The backport is clean and the test coverage is thorough. The existing inline comment about and vs && is a minor style preference that does not affect correctness.

[jac0626]

/retest

[LHT129]

Thanks for this backport to the 0.18 branch. The change from per-element uint16_t to generation-stamped bitmap words is a clean memory optimization.

I have reviewed the three changed files (visited_list.h, visited_list.cpp, visited_list_test.cpp) and the implementation looks correct:

• Bitmap logic: The Set/Get methods correctly handle word boundaries, bit masking, and the lazy tag-reset semantics.
• Zero-capacity handling: The constructor correctly short-circuits when word_count_ == 0, and the destructor/Reset guard against null pointers.
• Tag overflow: The Reset method properly wraps around and clears all tags when the generation counter overflows.
• Test coverage: The new tests cover word boundaries, repeated sets, memory usage calculation, tag overflow, zero-size construction, and randomized differential testing against a vector<bool> reference — thorough coverage.

Two existing inline comments have been raised by other reviewers that are worth addressing:

1. The use of and instead of && in Get() (style consistency)
2. The words_ array not being explicitly zero-initialized in the constructor (defensive coding)

Otherwise, this backport looks clean and faithful to the original #2449. No additional issues found.

[LHT129]

Thank you for this backport. The changes look good overall — the bitmap-based visited list is a clean optimization that significantly reduces scratch memory while preserving the lazy reset semantics.

I have reviewed the three changed files and the existing inline comments from other reviewers already cover the points worth addressing:

1. words_ initialization in the constructor (by @LHT129)
2. Get() should be marked const (by @LHT129)
3. Prefetch() should also prefetch words_ (by @LHT129)
4. Use && instead of and (by gemini-code-assist)

The test coverage is thorough — the randomized differential test against a std::vector<bool> reference is a strong validation approach. The zero-size and tag-overflow edge cases are also well covered.

No additional blocking issues from my side. Please address the existing suggestions at your discretion.

[LHT129]

Overall this backport looks clean and well-tested. The existing inline comments from other reviewers have already covered the actionable items (uninitialized words_ array, and vs &&, Get const-correctness, and Prefetch coverage). No additional issues found from my review.

[LHT129]

This is a clean backport of #2449 to the 0.18 branch. The changes are well-contained and the test coverage is thorough (word boundaries, tag overflow, zero-size, randomized differential testing).

The existing inline comments from LHT129 cover the remaining suggestions (words_ initialization, Get() const-correctness, Prefetch coverage for words_). No additional issues found.

LGTM once the existing suggestions are addressed.

[LHT129]

This backport of #2449 to the 0.18 branch is clean and well-tested. The generation-stamped bitmap approach correctly reduces visited-list scratch memory while preserving lazy reset semantics.

I have reviewed all three changed files (visited_list.h, visited_list.cpp, visited_list_test.cpp) and the implementation is sound:

• Bitmap logic: Set/Get correctly handle word boundaries, bit masking, and lazy tag-reset.
• Zero-capacity handling: Constructor short-circuits when word_count_ == 0, and the destructor/Reset guard against null pointers.
• Tag overflow: Reset properly wraps around and clears all tags when the generation counter overflows.
• Test coverage: Thorough — word boundaries, repeated sets, memory usage, tag overflow, zero-size construction, and a randomized differential test against std::vector<bool>.

The existing inline comments from LHT129 already cover the remaining suggestions worth addressing:

1. words_ initialization in the constructor (defensive coding)
2. Get() should be marked const
3. Prefetch() should also prefetch words_
4. Allocate() return value null check
5. word_count_ == 0 guard in Set/Get

No additional issues found. LGTM once the existing suggestions are addressed.

[LHT129]

Review Summary

This is a clean backport of #2449 to the 0.18 branch. The core change replaces the per-node uint16_t visitation array with a generation-stamped bitmap approach using uint64_t words, which significantly reduces memory usage for large graphs.

What was checked

• Correctness: The bitmap logic (word_id, mask, tag-stamped lazy reset) is correct. The tag overflow handling properly resets to 1 (not 0) to avoid false matches against zero-initialized tags.
• Backport fidelity: Correctly retains the 0.18 branch differences — int64_t return type for GetMemoryUsage() and the Catch2 test framework include.
• Test coverage: Comprehensive — covers word boundaries, repeated sets, tag overflow, zero-size edge case, and randomized differential testing against a std::vector<bool> reference.
• Memory safety: The destructor correctly guards against null words_ before deallocation.

Existing inline comments

Several issues have already been flagged by other reviewers that should be addressed before merging:

1. words_ array is not zero-initialized after allocation (technically safe due to tag-stamped access pattern, but UB if access pattern changes)
2. Missing null check on allocator_->Allocate() return value
3. Get() should be marked const
4. Prefetch() should also prefetch words_ alongside tags_
5. Set()/Get() on zero-capacity lists would dereference null pointers
6. Use && instead of and keyword

No additional blocking issues found beyond those already raised.

[LHT129]

Review Summary

I have reviewed this backport of #2449 to the 0.18 branch. The three changed files (visited_list.h, visited_list.cpp, visited_list_test.cpp) have been thoroughly examined.

What was checked

• Correctness: The generation-stamped bitmap logic in Set()/Get() is correct. Word boundaries, bit masking, and lazy tag-reset semantics are properly implemented.
• Backport fidelity: Correctly retains 0.18 branch specifics — int64_t return type for GetMemoryUsage(), Catch2 test framework, and existing include structure.
• Test coverage: Comprehensive — word boundaries (0, 1, 63, 64, 65, size-1), repeated sets, memory usage calculation, tag overflow (full uint16_t range), zero-size edge case, and randomized differential testing against std::vector<bool> across multiple sizes with 20k operations each.
• Memory safety: Destructor guards against null words_ before deallocation. Zero-capacity constructor short-circuits correctly.

Existing inline comments

All actionable issues have already been raised by other reviewers:

1. Use && instead of and keyword in Get() (gemini-code-assist)
2. words_ array not zero-initialized in constructor (LHT129)
3. Get() should be marked const (LHT129)
4. Prefetch() should also prefetch words_ (LHT129)
5. Missing null check on Allocate() return value (LHT129)
6. word_count_ == 0 guard in Set()/Get() (LHT129)

Conclusion

No additional issues found beyond those already raised. The backport is clean and faithful to the original #2449. Please address the existing suggestions at your discretion.

[LHT129]

I have completed a review of this backport PR. The core change — replacing the per-node visitation generation array with generation-stamped bitmap words — is correctly implemented and well-tested.

What this PR does well:

• The bitmap word approach significantly reduces memory usage (from 2 * max_size bytes to ceil(max_size/64) * 10 bytes).
• The lazy reset semantics are correctly preserved via the tag-stamped word design.
• The test coverage is thorough: word boundaries (0, 1, 63, 64, 65), repeated sets, tag overflow/wraparound, zero-capacity edge case, memory usage verification, and a randomized differential test against a std::vector<bool> reference.
• The CountingAllocator in the zero-size test correctly verifies that no allocation occurs when max_size == 0.

Existing inline comments from other reviewers have already covered the notable findings:

• and vs && operator style in Get() (visited_list.h:54)
• words_ array left uninitialized in the constructor (visited_list.cpp:24)
• Get() should be marked const (visited_list.h:53)
• Prefetch() should also prefetch words_ (visited_list.h:60)
• Missing null check on Allocate() return value (visited_list.cpp:26)
• Zero-capacity edge case with potential null dereference in Set/Get (visited_list.h:40)

I did not find additional substantive issues beyond those already flagged. The backport is clean and the implementation matches the intent described in the PR summary.

[LHT129]

This is a clean backport of #2449 to the 0.18 branch. The changes are well-contained to visited_list.{h,cpp} and the test file.

The existing inline reviews from LHT129 and gemini-code-assist have already covered the key points:

1. and → && operator style (gemini-code-assist)
2. words_ array left uninitialized in constructor (LHT129)
3. Get() should be marked const (LHT129)
4. Prefetch should also prefetch words_ for cache efficiency (LHT129)
5. Null check on Allocate() return value (LHT129)
6. Set/Get null dereference when word_count_ == 0 (LHT129)

The test coverage is comprehensive — word boundaries, tag overflow, zero-capacity, memory usage, and randomized differential testing against a reference implementation. No additional issues found.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.