Repository navigation
perf: reduce unnecessary index resizes during batch insertion - #158
reginaldalfret wants to merge 1 commit into
Conversation
Resolves spotify#121. Previously, TypedIndex::addItems resized the index to exactly `getNumElements() + rows` when capacity was exceeded. When adding elements incrementally or in small batches, this triggered frequent reallocations, deallocating and re-instantiating VisitedListPool and re-allocating mutex arrays under an exclusive lock. This commit introduces `calculateNewCapacity` using geometric growth: - Small indices (< 1,000 elements) double in capacity (or start with 16) to prevent frequent reallocations. - Larger indices grow by at least 10% or the batch size (whichever is larger) to minimize memory fragmentation. - Integer overflow is guarded using std::numeric_limits<size_t>::max(). - Adds a unit test in cpp/test/test_main.cpp verifying that incremental insertions beyond initial capacity resize correctly and return accurate nearest neighbor results. Signed-off-by: Reginald Alfret <reginaldalfret@gmail.com>
Manishram-ai
left a comment
There was a problem hiding this comment.
Thanks for picking up #121. I built the C++ tests from this branch (Release build on an M-series Mac) and the speedup is real. Adding vectors one at a time to a fresh index took 1.09s on main and 0.20s here for 5,000 vectors, and 3.65s vs 0.98s for 20,000. Batches of 10 got faster too, 1.58s to 0.96s for 20,000. The existing C++ tests pass.
A few things came up while testing.
-
The Java tests still expect an exact fit.
IndexTest.runTestWithcreates the index withmaxElements = 1, adds 2000 vectors and then checksassertEquals(numElements, index.getMaxElements()). Doing the same steps againstTypedIndexon this branch (the JNIgetMaxElementsreturns that value directly), I get 2001 after theaddItemsbatch and 2115 after adding one at a time, so the nine tests that callrunTestWithshould fail. I don't have Maven set up here, so I couldn't run the Java suite itself, and CI hasn't run on the PR yet. Changing that assert to something likeassertTrue(index.getMaxElements() >= numElements)would match the new behavior. -
A single large batch into a new index now ends up slightly bigger than it needs to be. 2000 vectors into an index created with
maxElements = 1gives 2001 here and 2000 on main. That comes fromcurrentMax + growthaftergrowthis raised torowsToAdd. Thestd::max(minCapacity, ...)on the return already makes room for the whole batch, so dropping that bump keeps the exact fit for big batches. I tried it locally and one-at-a-time inserts still grow the same way (2115 after 2000 inserts, same timings). Suggestion inline. -
The new test also passes on main, since main ends at exactly 10 and
>= 10holds. On this branch the first resize goes from 2 to 18, so asserting> 10would make it fail without the change.
Also, the TODO right above the first resize loop asks whether we should always double the size, which this PR answers now, so it could be updated or removed.
| if (growth < rowsToAdd) { | ||
| growth = rowsToAdd; | ||
| } |
There was a problem hiding this comment.
Dropping these three lines keeps one big batch at an exact fit (2000 rows into a fresh index gives 2000 instead of 2001), since the std::max(minCapacity, ...) below already covers the batch. Growth for one-at-a-time inserts doesn't change, I still get 2115 after 2000 inserts from 1.
| if (growth < rowsToAdd) { | |
| growth = rowsToAdd; | |
| } |
| } | ||
|
|
||
| REQUIRE(index.getNumElements() == 10); | ||
| REQUIRE(index.getMaxElements() >= 10); |
There was a problem hiding this comment.
This one passes on main as well, where the index ends at exactly 10. Here it ends at 18, so this would fail without the change:
| REQUIRE(index.getMaxElements() >= 10); | |
| REQUIRE(index.getMaxElements() > 10); |
Problem
As reported in #121, repeatedly calling
addItemor adding small batches of vectors to an index causes frequent implicit index resizes. InTypedIndex::addItems, whenevergetNumElements() + rows > getMaxElements(), the index was resized to exactlygetNumElements() + rows.Because
resizeIndexacquires an exclusive lock (resizeLock), deletes and re-instantiatesVisitedListPool, reallocateselement_levels_, allocates a new vector ofstd::mutexper element, and callsreallocon the base and upper layers, frequent resizing is expensive and severely degrades insertion throughput.Solution
Introduced
calculateNewCapacity(rowsToAdd)inTypedIndex.h:< 1,000elements), capacity doubles (with a minimum baseline of 16) to avoid reallocating on every insertion.>= 1,000elements), capacity grows by at least 10% or the batch size (rowsToAdd), whichever is larger.size_toverflow usingstd::numeric_limits<size_t>::max().cpp/test/test_main.cppverifying incremental item addition beyond initial capacity, validating thatmaxElementsexpands geometrically and nearest-neighbor query accuracy is preserved.Resolves #121