Conversation
30cddd3 to
7e430f7
Compare
|
Updated PR to resolve merge conflicts since #16618 was merged. |
| } | ||
|
|
||
| /** | ||
| * Fixes disconnected nodes at a specific level by searching from each node's existing neighbors |
There was a problem hiding this comment.
Let's preserve the javadocs from the original implementation please, unless we (a human, not the AI, please) read them and think they need correcting. Honestly I'm finding it really tedious reviewing the AI's javadocs. Could you please take a pass over them all yourself and see if you think they are OK
There was a problem hiding this comment.
sorry if you did that, but I'm finding enough things that sounds like AI to me ...
There was a problem hiding this comment.
Sorry about that, I share the same opinion about these AI code comments. As I was trying to get it to improve the net-new comments it wrote it seems it messed with a bunch of others that wasn't my intention. I did this right at the end of my own self review and didn't think to look at the other comments it hadnt touched before. Will double check this in the future.
ba08a3a to
8ea4b7d
Compare
Repair ran serially inside createBuilder and dominated merge time; it now runs across the merge worker pool, one level at a time.
8ea4b7d to
0c4f49d
Compare
|
Please try to avoid force-push if possible, since it loses the commits on which the reviews are based and makes tracking the changes much more difficult |
3db8865 to
0c4f49d
Compare
Sorry! I was trying to revise and my agent ended up modifying the PR when I meant to just reconcile and push so I could review it myself first. My error, please do not review yet, I will add a more detailed update when I get a chance to fully review and finalize first. |
If this PR is not finalized, then you could mark this PR as draft. |
|
Thanks all for the feedback from previous reviewers, and sorry for the mess with the force pushing and draft PR stuff, I'm still getting the hang of this github thing for real prod-level development. Hopefully it is clear by my comment replies and javadocs what was changed and why, but happy to help clarify more if needed. I am also in the middle of running some more benchmarks on cohere to show the before and after results at some different deletion percents, so I'll add those in an additional comment when they are finished. Going to reopen this for review now. |
|
Some benchmarking results: Cohere-1024, 500k normalized float32 vectors indexed as 5x100k segments, M=32, beamWidth=200, forceMerge(1), eight merge workers, Corretto 25, and 16 pinned CPUs on a Graviton desktop. 1% deletes
5% deletes
20% deletes
35% deletes
|
3 participants
Overview
Currently, when
IncrementalHnswGraphMergerreuses an existing graph,InitializedHnswGraphBuildercopies it, repairs the nodes that lost too many neighbors, and rebalances the levels. This logic runs insideConcurrentHnswMerger.createBuilder. Which is called beforeHnswConcurrentMergeBuildereven exists, so the copy/repair/rebalance process runs on the single merge thread without utilizing the worker pool.This is expensive especially for repair as it does a search per flagged node. Benchmarking with a 500k cohere index at 35% deletes shows repair takes 97.7s of a 174.7s merge. This is slower than the full rebuild process which does use the paralellized worker pool, even though it reduces CPU utilization. (Full rebuild of the same index took 97.7s for ALL steps, while the repair step itself took the same amount of time)
Changes
The three phases used to be one unit where
initGraphwould copy, repair, and rebalance the graph. With this change, the copy now fires alone (keep this part serial, its difficult/not worth to parallelize) and the other work is deferred to the concurrent merge builder which finishes the process. In practice:InitializedHnswGraphBuilder.copyGraphexecutes the copy and returns the half-built graph and the list of nodes needing repair per level, thenConcurrentHnswMergerhands both toHnswConcurrentMergeBuilder, which repairs and rebalances insidebuild(), which utilizes the worker pool.Repair is now structured to be per node per level work items to allow this parallelization. On each level, the workers take batches of flagged nodes off a shared
AtomicIntegeruntil that level's list is drained, one task per worker, and each worker repairs its nodes through its own scorer and searcher.Levels are repaired top-down with
invokeAllserializing the levels (workers dont move on to next level until the current one is complete). This is necessary because nodes with no surviving neighbors have no entry point to search from, so it walks from the entry node to reach the current level, which looks at above levels (that must be completed before this happens). For this pr the rebalance work stays serial on the copy builder afterwards, as its time is negligible to the overall re-use process.Repairing concurrently needed one lock that a normal insert doesn't. With copied nodes that already inherit connections, its possible one node can be adding a reciprocal link into the same array that another is selecting. During repair, I hold
hnswLock.write(level, node)around the selection and snapshot the entry points underhnswLock.read(level, node)before searching. I then release the lock before the per-neighbor loop that takeshnswLock.write(level, nbr). This ensures that a worker holds at most one stripe at a time.Notes for reviewers
fixDisconnectedNodesandaddConnectionsontoHnswGraphBuilder, so it may be useful to look at inter-commit diff while reviewing.Benchmarking / Testing
In addition to the unit tests added in the PR, I ran before/after benchmarks on cohere to show the difference with/without parallelization.
cohere-1024, M=32, beamWidth=200, 500k vectors indexed as 5x100k segments, 35% delete, forceMerge(1), numMergeWorkers=8, m7g.4xlarge pinned to 8 cores
We see a dramatic speedup in copy+repair time from this fix compared to re-use with no parallelism. Note that in this run specifically, re-use and rebuild are nearly the same, but this is at 35% deletes (near upper limit of the amount of repair work the merge algo needs to do) This shows that we, at worst, restore the wall-time that was lost with the previous unparallel implementation and that with lower delete percents, the speedup will be more visible (at 10% deletes, I see a ~13% speedup with re-use vs rebuild).
Closes #16618