From 3a940f1124467d4197ce0dacef2faded62be3061 Mon Sep 17 00:00:00 2001 From: sharonrosenfeld Date: Sun, 9 Aug 2026 17:39:00 +0300 Subject: [PATCH] Allow storeNewElement to reuse an existing element id `storeNewElement` always allocated a fresh id by advancing `curElementCount` and appended the vector and the graph data to their containers. Add an optional `elementId` parameter: when a valid id is given it is used as-is, the index does not grow, and both the vector and the graph data are written in place over that slot (`RawDataContainer::updateElement` / `DataBlock::updateElement`), since the containers only support appending at `curElementCount`. Defaults to INVALID_ID, so all existing call sites keep allocating fresh ids. Co-Authored-By: Claude Opus 5 (1M context) --- src/VecSim/algorithms/hnsw/hnsw.h | 51 +++++++++++++++++++++++-------- 1 file changed, 39 insertions(+), 12 deletions(-) diff --git a/src/VecSim/algorithms/hnsw/hnsw.h b/src/VecSim/algorithms/hnsw/hnsw.h index 2c1fae87f..41756bd9b 100644 --- a/src/VecSim/algorithms/hnsw/hnsw.h +++ b/src/VecSim/algorithms/hnsw/hnsw.h @@ -277,7 +277,17 @@ class HNSWIndex : public VecSimIndexAbstract, bool isMarkedDeleted(idType internalId) const; bool isInProcess(idType internalId) const; void unmarkInProcess(idType internalId); - HNSWAddVectorState storeNewElement(labelType label, const void *vector_data); + // `elementId` is optional: + // - INVALID_ID (the default): a new id is allocated by advancing `curElementCount`, and the + // vector and the graph data are *appended* to their containers (growing the index by a block + // first if it is full). + // - a valid id: the id is used as-is, `curElementCount` is left untouched and the index does + // not grow. The vector and the graph data are written *in place* over that slot. + // The caller must guarantee that the slot is a legal, already-freed one - that is, + // `elementId < curElementCount` and the previous occupant's `ElementGraphData` has been + // destroyed (see `ElementGraphData::destroy`), otherwise its link lists are leaked. + HNSWAddVectorState storeNewElement(labelType label, const void *vector_data, + idType elementId = INVALID_ID); void removeAndSwapMarkDeletedElement(idType internalId); void repairNodeConnections(idType node_id, size_t level); // For prefetching only. @@ -1761,10 +1771,17 @@ void HNSWIndex::removeVectorInPlace(const idType element_int // Store the new element in the global data structures and keep the new state. In multithreaded // scenario, the index data guard should be held by the caller (exclusive lock). +// `elementId` is optional - see the declaration for its semantics. template HNSWAddVectorState HNSWIndex::storeNewElement(labelType label, - const void *vector_data) { - if (isCapacityFull()) { + const void *vector_data, + idType elementId) { + // When an id is given we are reusing an existing (already freed) slot, so the index doesn't + // grow and no capacity check is needed - the slot is within the current capacity by definition. + const bool reuseSlot = (elementId != INVALID_ID); + assert((!reuseSlot || elementId < curElementCount) && + "The given id must be an existing slot in the index"); + if (!reuseSlot && isCapacityFull()) { growByBlock(); } HNSWAddVectorState state{}; @@ -1773,36 +1790,46 @@ HNSWAddVectorState HNSWIndex::storeNewElement(labelType labe state.elementMaxLevel = getRandomLevel(mult); // Access and update the index global data structures with the new element meta-data. - state.newElementId = curElementCount++; + // Allocate a fresh id only if the caller didn't provide one. + state.newElementId = reuseSlot ? elementId : curElementCount++; // Create the new element's graph metadata. // We must assign manually enough memory on the stack and not just declare an `ElementGraphData` // variable, since it has a flexible array member. - auto tmpData = this->allocator->allocate_unique(this->elementGraphDataSize); - memset(tmpData.get(), 0, this->elementGraphDataSize); - ElementGraphData *cur_egd = (ElementGraphData *)(tmpData.get()); + auto newData = this->allocator->allocate_unique(this->elementGraphDataSize); + memset(newData.get(), 0, this->elementGraphDataSize); + ElementGraphData *new_egd = static_cast(newData.get()); // Allocate memory (inside `ElementGraphData` constructor) for the links in higher levels and // initialize this memory to zeros. The reason for doing it here is that we might mark this // vector as deleted BEFORE we finish its indexing. In that case, we will collect the incoming // edges to this element in every level, and try to access its link lists in higher levels. // Therefore, we allocate it here and initialize it with zeros, (otherwise we might crash...) try { - new (cur_egd) ElementGraphData(state.elementMaxLevel, levelDataSize, this->allocator); + new (new_egd) ElementGraphData(state.elementMaxLevel, levelDataSize, this->allocator); } catch (std::runtime_error &e) { this->log(VecSimCommonStrings::LOG_WARNING_STRING, "Error - allocating memory for new element failed due to low memory"); throw e; } - // Insert the new element to the data block - this->vectors->addElement(vector_data, state.newElementId); - this->graphDataBlocks.back().addElement(cur_egd); + // Insert the new element to the data block. The containers only support *appending* at + // `curElementCount`, so when reusing an existing slot we have to overwrite it in place + // instead. The previous occupant's graph data must have already been destroyed by the + // caller, otherwise its link lists are leaked by the overwrite below. + if (reuseSlot) { + this->vectors->updateElement(state.newElementId, vector_data); + this->graphDataBlocks[state.newElementId / this->blockSize].updateElement( + state.newElementId % this->blockSize, new_egd); + } else { + this->vectors->addElement(vector_data, state.newElementId); + this->graphDataBlocks.back().addElement(new_egd); + } // We mark id as in process *before* we set it in the label lookup, so that IN_PROCESS flag is // set when checking if label . this->idToMetaData[state.newElementId] = ElementMetaData(label); setVectorId(label, state.newElementId); - state.currMaxLevel = (int)maxLevel; + state.currMaxLevel = static_cast(maxLevel); state.currEntryPoint = entrypointNode; if (state.elementMaxLevel > state.currMaxLevel) { if (entrypointNode == INVALID_ID && maxLevel != HNSW_INVALID_LEVEL) {