Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce locks for adhoc BF #419

Merged
merged 6 commits into from
Aug 29, 2023
Merged

Reduce locks for adhoc BF #419

merged 6 commits into from
Aug 29, 2023

Conversation

alonre24
Copy link
Collaborator

@alonre24 alonre24 commented Aug 24, 2023

Describe the changes in the pull request

In ad-hoc BF mode of the tiered index, we call internally to getDistanceFrom of the flat and HNSW indexes. These calls access the indexes' data, and it is not safe to run insert/delete operations in parallel. Until now we were acquiring the locks internally, but since this is usually called for every vector individually, we see that the overhead of acquiring and releasing the locks is significant in that case.
Hence we introduce the new API for acquiring and releasing the locks for shared ownership and assume that the caller acquires the locks while calling the tiered index getDistanceFrom.

Mark if applicable

  • This PR introduces API changes
  • This PR introduces serialization changes

@alonre24 alonre24 requested review from GuyAv46 and meiravgri and removed request for GuyAv46 August 24, 2023 14:39
@codecov
Copy link

codecov bot commented Aug 24, 2023

Codecov Report

Patch coverage: 92.30% and project coverage change: -0.12% ⚠️

Comparison is base (2d45add) 95.59% compared to head (9a7ff88) 95.48%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #419      +/-   ##
==========================================
- Coverage   95.59%   95.48%   -0.12%     
==========================================
  Files          65       65              
  Lines        4086     4093       +7     
==========================================
+ Hits         3906     3908       +2     
- Misses        180      185       +5     
Files Changed Coverage Δ
src/VecSim/vec_sim_index.h 96.73% <0.00%> (-2.15%) ⬇️
src/VecSim/vec_sim_interface.h 100.00% <ø> (ø)
.../VecSim/algorithms/brute_force/brute_force_multi.h 87.36% <100.00%> (ø)
...VecSim/algorithms/brute_force/brute_force_single.h 89.47% <100.00%> (ø)
src/VecSim/algorithms/hnsw/hnsw.h 98.33% <100.00%> (-0.10%) ⬇️
src/VecSim/algorithms/hnsw/hnsw_multi.h 89.01% <100.00%> (-1.90%) ⬇️
src/VecSim/algorithms/hnsw/hnsw_single.h 90.66% <100.00%> (-0.59%) ⬇️
src/VecSim/algorithms/hnsw/hnsw_tiered.h 98.92% <100.00%> (+0.01%) ⬆️
src/VecSim/vec_sim.cpp 96.26% <100.00%> (+0.17%) ⬆️

... and 1 file with indirect coverage changes

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

Copy link
Collaborator

@meiravgri meiravgri left a comment

Choose a reason for hiding this comment

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

See comments below :)
The main suggestion is to include UnSafe in the API function's name so if we have an alternative safe solution in the future, we won't have to break the API again.
Also, since this can cause a bottleneck in writing requests, I think it is worth examining the option to first accumulate results up to some limit (on Redisearch side) and then calculate the accumulated vectors distances.

src/VecSim/vec_sim.cpp Outdated Show resolved Hide resolved
src/VecSim/vec_sim_index.h Outdated Show resolved Hide resolved
src/VecSim/vec_sim.h Show resolved Hide resolved
src/VecSim/algorithms/hnsw/hnsw_tiered.h Show resolved Hide resolved
src/VecSim/algorithms/hnsw/hnsw_multi.h Show resolved Hide resolved
@alonre24 alonre24 merged commit 2295448 into main Aug 29, 2023
23 of 25 checks passed
@alonre24 alonre24 deleted the reduce_locks_for_adhoc_bf branch August 29, 2023 07:20
alonre24 added a commit that referenced this pull request Aug 29, 2023
alonre24 added a commit that referenced this pull request Aug 29, 2023
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.

3 participants