HNSW: reuse pooled id sets in the delete and repair paths

[dor-forer]

Draft. Numbers are in, but one design decision is open (see Open decision) and one unit test needs a call on it before this is ready for review.

Describe the changes in the pull request

Three places in the HNSW delete paths build a vecsim_stl::vector<bool> sized by the whole index, per call, to hold at most a node's links plus its neighbours' links (order tens of ids):

Location	Cost today
repairNodeConnections	two bitmaps sized maxElements, allocated + zero-filled per background repair job
repairConnectionsForDeletion	one bitmap sized curElementCount per call: once per bidirectional neighbour and once per incoming edge, per level
removeVectorInPlace	assign(curElementCount, false) once per level (buffer reused, but the zero fill is O(N) each time)

At 2M vectors that is roughly 250 KB of allocate-and-zero per repair job, for a job whose real work is a few hundred distance computations.

This PR replaces them with a pooled set that keeps O(1) lookup but clears in time proportional to its own size: in-place deletes get 35% faster at 2M vectors and the gain grows with index size; the async repair path saves CPU but not wall-clock time.

One hazard worth reviewer attention: removeVectorInPlace ends in removeAndSwap -> shrinkByBlock -> resizeIndexCommon, which resizes the pool. Holding a checked-out set across that trips the pool's "nothing in use" assertion, so the scratch is scoped to end before it.

Which issues this PR fixes

None. This came out of profiling the delete paths, not a planned task, so there is no MOD ticket. Happy to file one if the convention requires it.

Not MOD-9645, though it overlaps it: that task reuses internal element ids for new inserts to avoid swap jobs, which is a different mechanism (this PR changes no id allocation and removes no swap job). It will edit repairNodeConnections too, and its reported blocker is a repair-job race in this same code, so the two should be sequenced rather than merged blind. The pooled sets are id-indexed and cleared per call, so recycled ids do not break them.

Main objects this PR modified

1. IdFlagSet (new, src/VecSim/algorithms/hnsw/id_flag_set.h) - one bit per id plus the list of ids it set, so clear() is proportional to the set's size rather than the index capacity.
2. IdFlagSetPair / IdFlagSetPool (new) - hands out both sets a delete or repair job needs in a single pool round-trip, mirroring VisitedNodesHandlerPool. Releases its sets when index capacity drops to zero, so an emptied index returns to baseline memory.
3. PooledIdFlagSets (new) - RAII holder, so an early return cannot leak a set out of the pool.
4. HNSWIndex::repairNodeConnections, ::repairConnectionsForDeletion (signature now takes both sets from its caller), ::removeVectorInPlace - switched to the pooled sets.
5. HNSWIndex::resizeIndexCommon and both HNSWIndex constructors - pool lifecycle, next to the existing visitedNodesHandlerPool calls.

Mark if applicable

• This PR introduces API changes
• This PR introduces serialization changes

Neither box applies. hnsw_serializer_impl.h is touched, but only to initialize and size the new pool in the deserializing constructor; the on-disk format and the encoding version are unchanged.

Results

Xeon Platinum 8375C, 16 cores, gcc 13.3, RelWithDebInfo, tiered HNSW, dim 32, M 16, 3000 deletes per phase, 16 background threads. Two clones of the same commit (one patched), binaries run alternately, 3 reps each, mean reported.

In-place delete path (single-threaded: removeVectorInPlace / repairConnectionsForDeletion), us per delete:

N	baseline	patched	change
250K	214.4	199.0	-7.2%
1M	405.2	288.2	-28.9%
2M	533.1	342.7	-35.7%

Patched run-to-run spread also collapses (287.6 / 287.8 / 289.1 at 1M, against 377.6 / 414.0 / 424.0 baseline), which is what removing a size-dependent memset looks like.

Async repair path, us per delete:

N	metric	baseline	patched	change
1M	CPU (all threads)	3060.4	2906.3	-5.0%
2M	CPU (all threads)	3678.8	3227.2	-12.3%
1M	wall	514.7	529.9	+3.0%
2M	wall	584.0	597.4	+2.3%

The async path saves CPU (throughput headroom) but not latency: the zero fill it removes was already spread across the 16 background threads, and what remains is one pool mutex round-trip per job. Cutting the lock traffic (one scratch pair per job rather than per call, which is what this branch does) narrowed that but did not close it.

Tests

2606 / 2607 unit tests pass on the patched tree.

• HNSWTieredIndexTest.swapJobBasic (x2) failed at first and now passes: that is what drove the pool releasing its sets at zero capacity, so an emptied index still returns to its baseline memory.
• IndexAllocatorTest.test_hnsw_reclaim_memory fails: 849,620 actual vs 848,680 expected, i.e. exactly the 940 bytes of pooled scratch retained while the index is non-empty. test_allocator.cpp:606 is a whitebox model of every allocation, so any new accounted consumer breaks it by construction. Nothing leaks: the memory is accounted through VecSimAllocator and released when the index empties.

Open decision

1. Keep both paths and extend the accounting test's expectation (my preference): keeps the 35% in-place win and the 12% async CPU headroom. Cost: the host sees a small bounded reservation (one scratch pair per concurrent deleter) while an index is non-empty, exactly as the visited-nodes pool already behaves.
2. In-place path only: revert the async change, since that is where the wall-clock gain is absent. Smaller diff, less concurrency surface. Still needs the same test update, because the failing test exercises the in-place path.
3. Zero retained memory: drop the pool to empty whenever no delete is in flight. Leaves the accounting invariant untouched, at the cost of more lifecycle logic.

Notes

Unrelated papercuts found while measuring, not fixed here:

• make unit_test CTEST_ARGS='-R A|B' breaks: the Makefile passes it unquoted, so the shell splits on |.
• Running ctest directly needs ROOT=<repo>, or 14 serialization tests fail inside getenv("ROOT") with "basic_string: construction from null is not valid".
• -DUSE_SVS=OFF does not build on main: svs.h / svs_utils.h are included even with HAVE_SVS=0.

🤖 Generated with Claude Code