Skip to content

[MOD-19284] Reuse ids on async multi-value updateVectors - #1075

Merged
nonirosenfeldredis merged 5 commits into
mainfrom
MOD-19284-reuse-id-async-multi
Oct 6, 2026
Merged

nonirosenfeldredis merged 5 commits into
mainfrom
MOD-19284-reuse-id-async-multi

Conversation

@nonirosenfeldredis

@nonirosenfeldredis nonirosenfeldredis commented Oct 4, 2026 •

Copy link
Copy Markdown
Collaborator

Jira: MOD-19284

Follow-up to #1068 (async id reuse for single-value overwrites), extending it to multi-value labels.

Summary

  • In async write mode, updateVectors on a multi-value label deleted every old id and appended n fresh ones, leaving the old ids marked deleted until swap jobs compacted them - so repeated updates grew the index until GC ran.
  • Now the label's old ids are reserved, one per new vector, for the new vectors' insert jobs. Each such job is held until its id's repairs are done and then stores its vector there, exactly as in [MOD-19134] Reuse the replaced id on async single-value overwrites #1068.
    • n == m (old count): every old id is reused, nothing to compact.
    • n < m (shrink): n ids are reused, the remaining m - n go through normal compaction.
    • n > m (grow): m ids are reused, the remaining n - m vectors are appended as fresh ids.
  • New updateMultiValueAsync: drops the label's buffered copies, buffers the n new blobs in the flat buffer with their insert jobs (not yet submitted), then marks the label's HNSW vectors deleted before any new vector can be ingested (otherwise they'd be marked deleted too), pairing them with the jobs. Falls back to the previous delete-then-add path if the flat buffer can't hold all n blobs, if n == 0, or during SQ accumulation.
  • deleteLabelFromHNSW now takes a std::span<HNSWInsertJob *> of reuse jobs instead of a single job, and returns just the deleted count. Entries whose submission was delegated to the repair flow (their id has repairs pending) are set to nullptr, and the caller submits the rest. The single-value addVector call site is adapted to the same interface.
  • Only updateVectors reuses ids. A plain addVector on an existing multi-value label still just appends (it isn't a replacement).
  • Gated on the existing reuseIdOnUpdate flag (setReuseOnUpdate(false) restores the old behavior).
  • New debug counters for measuring defrag (runs and time, for tiered HNSW and tiered SVS), see the Defrag cost counters section below.

Locking

Unchanged from #1068: all the held-job, deferred-submission and reuse-store logic is reused as is. The only new concurrency consideration is the order in updateMultiValueAsync: all n insert jobs are created and registered in labelToInsertJobs under flatIndexGuard, but none is submitted until deleteLabelFromHNSW has marked the old ids deleted, so a worker can't ingest a new vector into the label before the old ones are marked.

The capacity check and the staging loop run under one scoped flatIndexGuard lock, and the label's labelToInsertJobs entry is looked up once before the loop. readySwapJobs is now atomic, since shouldDefrag() reads it on the writer thread while worker threads update it.

Defrag cost counters

Observability for how much GC costs, separate from the id-reuse change above.

  • HNSW: defrag() now also accounts for its own cost, so GC time can be read off an index: number of runs and wall time (including the wait for the main index guard). All callers (runGC, addVector, deleteVector, updateMultiValueAsync) already go through it. Reported in the tiered HNSW debug info as TIERED_HNSW_DEFRAG_RUNS and TIERED_HNSW_DEFRAG_TIME_NS. The number of swap jobs each run executed is logged at debug level instead of being counted; executeReadySwapJobs now returns it. Callers that bypass defrag() (benchmarks) are not counted.
  • SVS: The tiered SVS index counts its backend GC runs the same way (async GC job and write-in-place runGC(); a GC on an empty index returns early and is not counted), reported as TIERED_SVS_DEFRAG_RUNS and TIERED_SVS_DEFRAG_TIME_NS, with the time also logged at debug level.

Test plan

  • New: asyncUpdateVectorsMultiReusesIds (same count, shrink, and grow: checks which ids are reserved, that reused ids are a subset of the old ones, index size / marked-deleted / ready-swap-job counts before and after GC, and that other labels are untouched), asyncUpdateVectorsMultiReleasesIdsWhenUpdatedAgain, asyncUpdateVectorsMultiWithReuseDisabledAppends.
  • Defrag counters: invalidRepairJobOnSwap also checks the HNSW run count (at least 2) and time (> 0) and that the debug info matches; runGCAPI (1 run) and runGCInPlaceMode (threshold / 64 + 1 runs) check the SVS counters. The tiered info-field tests were updated for the new fields (expected field count HNSW 16 -> 18, SVS 18 -> 20, field order, per-field comparison). Tiered HNSW (434) and tiered SVS plus info-iterator tests (151) pass.
  • test_hnsw tiered tests: 434 pass; swept over VECSIM_SEED 1-12 with no failures. After the review fixes (scoped lock, atomic readySwapJobs): the *Tiered* filter in test_hnsw (298 tests, ASan debug build) passes for VECSIM_SEED 1-6.
  • clang-format 18.1.8 clean on all changed files.
  • Not yet run: tiered tests in test_int8 / test_uint8 / test_bf16 / test_fp16 / test_hnsw_sq8, the broader seed sweep, and a concurrency stress run - relying on CI for these for now.

🤖 Generated with Claude Code


Note

Medium Risk
Changes async deletion, repair, and insert job orchestration for multi-value updates; incorrect id reservation or submission could affect graph integrity until GC, though heavily tested.

Overview
Async multi-value updateVectors now reuses internal HNSW ids instead of delete-then-append, matching the existing single-value overwrite behavior when reuseIdOnUpdate is on.

deleteLabelFromHNSW takes a std::span<HNSWInsertJob *> so one deleted id can be reserved per pending insert job; jobs whose submission is deferred to the repair path are nulled in the span. New updateMultiValueAsync stages all n blobs and jobs under the flat lock, marks old vectors deleted before submit, then reuses ids pairwise (with fallback to delete-then-add if the flat buffer cannot hold n blobs).

Defrag/GC observability: tiered HNSW centralizes swap compaction in defrag() (timing + run count); readySwapJobs is atomic with shouldDefrag() for threshold checks. Tiered HNSW and tiered SVS expose TIERED_*_DEFRAG_RUNS and TIERED_*_DEFRAG_TIME_NS in debug info. Tests cover multi-value reuse, re-update invalidation, reuse-disabled behavior, and counter fields.

Reviewed by Cursor Bugbot for commit b0c52fc. Bugbot is set up for automated code reviews on this repo. Configure here.

An async updateVectors on a multi-value label deleted every old id and
appended n fresh ones, so the index grew until GC ran. The label's old ids
are now reserved, one per new vector, for the new vectors' insert jobs, which
are held back until their id's repairs are done and then store into it.
Surplus old ids (the label shrinks) go to normal compaction, and surplus new
vectors (it grows) append. deleteLabelFromHNSW takes a span of reuse jobs and
nulls the entries whose submission it delegated to the repair flow. Falls
back to delete-then-add when the flat buffer can't hold all n vectors.
Plain addVector on a multi-value label is unchanged. Gated on reuseIdOnUpdate.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@codecov

codecov Bot commented Oct 4, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.67442% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 97.53%. Comparing base (2c35d74) to head (b0c52fc).

Files with missing lines Patch % Lines
src/VecSim/algorithms/hnsw/hnsw_tiered.h 97.10% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1075      +/-   ##
==========================================
- Coverage   97.55%   97.53%   -0.03%     
==========================================
  Files         142      142              
  Lines        9336     9405      +69     
==========================================
+ Hits         9108     9173      +65     
- Misses        228      232       +4     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

nonirosenfeldredis and others added 3 commits October 5, 2026 09:35
Route every executeReadySwapJobs call through defrag() and record, per index,
the number of defrag runs, the swap jobs they executed and the wall time they
took (including the wait for the main index guard). The values are reported in
the tiered HNSW debug info as TIERED_HNSW_DEFRAG_RUNS, _SWAP_JOBS and _TIME_NS.

executeReadySwapJobs now returns how many jobs it ran.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…jobs

Drop the swap-jobs counter from the tiered HNSW info and log the number of
swap jobs each defrag executes at debug level instead. Count backend GC runs
and their time in the tiered SVS index as well, reported as
TIERED_SVS_DEFRAG_RUNS and TIERED_SVS_DEFRAG_TIME_NS. The time includes the
wait for the main index guard, as for HNSW.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
defrag() now logs a debug-level summary after each run, so the tiered index
log in this test has one more entry. Its elapsed time varies, so only the
fixed part of the message is compared.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

@dor-forer dor-forer left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very good

const char *blob = static_cast<const char *>(new_blobs);
vecsim_stl::vector<HNSWInsertJob *> insert_jobs(this->allocator);
insert_jobs.reserve(n);
this->flatIndexGuard.lock();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a scoped std::lock_guard be better here?

const void *new_blobs, size_t n) {
// Drop the label's buffered copies first, which also makes room for the new ones.
this->deleteFromFlatAndInsertJobs(label);
if (this->frontendIndex->indexSize() + n > this->flatBufferLimit) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the capacity check and staging loop should under a scoped std::lock_guard on flatIndexGuard?

Comment on lines +1533 to +1536
auto [it, inserted] = this->labelToInsertJobs.try_emplace(label, 1, job, this->allocator);
if (!inserted) {
it->second.push_back(job);
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do need try_emplace in every loop run (it's the same label).
We can, reserve it once before the loop and just push it every itertion:

auto it = this->labelToInsertJobs.try_emplace(label, this->allocator).first;
auto &label_jobs = it->second;
label_jobs.reserve(n);

And then in the loop

label_jobs.push_back(job);

}
template <typename DataType, typename DistType>
auto TieredHNSWIndex<DataType, DistType>::shouldDefrag() {
return readySwapJobs >= this->pendingSwapJobsThreshold;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to guard readySwapJobs here also, maybe we can make it atomic?

…ySwapJobs

- updateMultiValueAsync: scoped lock_guard on flatIndexGuard that also covers
  the flat buffer capacity check; look up the label's insert-job vector once
  before the staging loop.
- readySwapJobs is atomic: shouldDefrag() reads it on the writer thread while
  workers update it.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@nonirosenfeldredis
nonirosenfeldredis added this pull request to the merge queue Oct 6, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Oct 6, 2026
@nonirosenfeldredis
nonirosenfeldredis added this pull request to the merge queue Oct 6, 2026
Merged via the queue into main with commit ab6bb4f Oct 6, 2026
16 checks passed
@nonirosenfeldredis
nonirosenfeldredis deleted the MOD-19284-reuse-id-async-multi branch October 6, 2026 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants