Add optional persistent locks to IndexHNSW for incremental adds (#5031)#5031
Open
ddrcoder wants to merge 1 commit intofacebookresearch:mainfrom
Open
Add optional persistent locks to IndexHNSW for incremental adds (#5031)#5031ddrcoder wants to merge 1 commit intofacebookresearch:mainfrom
ddrcoder wants to merge 1 commit intofacebookresearch:mainfrom
Conversation
Contributor
…bookresearch#5031) Summary: `IndexHNSW` allocates an initializes locks for `ntotal+n` nodes on every call to `add()`. This makes batched insertion very costly, and incremental insertion prohibitively so. This diff introduces optional persistent locks for `IndexHNSW` to improve incremental `add()` performance. Previously, `omp_lock_t` arrays of size `ntotal+n` were created/destroyed on each `add()` call. Now locks can be retained via a new `retain_locks` flag (default: false), using a new `HNSW::Lock` RAII wrapper with geometric growth. RFC: Instead of `retain_locks` being the only way to opt into this new behavior, this could be inferred on the first incremental add. That is, clear the locks after insertion iff `n0 == 0`. Workloads which call `add()` once would be unaffected, but workloads which call `add()` repeatedly would 1) forego the clearing of the lock vector after `add()` call facebookresearch#2, and reuse locks for all subsequent calls. The downside would be the lack of the ability to reclaim the locks after insertion without HNSW-specific behavior at the call site. Differential Revision: D98232750
meta-codesync
bot
changed the title
ddrcoder
force-pushed
the
export-D98232750
branch
from
cb6a017 to
f49d25f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary:
IndexHNSWallocates an initializes locks forntotal+nnodes on every call toadd(). This makes batched insertion very costly, and incremental insertion prohibitively so.This diff introduces optional persistent locks for
IndexHNSWto improve incrementaladd()performance. Previously,omp_lock_tarrays of sizentotal+nwere created/destroyed on eachadd()call. Now locks can be retained via a newretain_locksflag (default: false), using a newHNSW::LockRAII wrapper with geometric growth.RFC: Instead of
retain_locksbeing the only way to opt into this new behavior, this could be inferred on the first incremental add. That is, clear the locks after insertion iffn0 == 0. Workloads which calladd()once would be unaffected, but workloads which calladd()repeatedly would 1) forego the clearing of the lock vector afteradd()call #2, and reuse locks for all subsequent calls. The downside would be the lack of the ability to reclaim the locks after insertion without HNSW-specific behavior at the call site.Differential Revision: D98232750