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.

Sign up for GitHub

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

Jump to bottom

Space Optimization Fix #175

Merged
merged 19 commits into from
Jul 28, 2022
Merged

Space Optimization Fix #175

merged 19 commits into from
Jul 28, 2022

Conversation

GuyAv46
Copy link
Collaborator

@GuyAv46 GuyAv46 commented Jul 25, 2022 •
edited
Loading

TL;DR
This PR adds few missing functions for the optimizations (SIMD 4) and improves the logic of choosing the distance function
This PR also includes a benchmark for the distance functions.

This PR is revisiting the optimized distance functions and improve them

  1. Added missing functions on AVX512 and AVX implementations
  2. improved the choosing logic to take the best function for each dimension. chech L2_space.cpp or IP_space.cpp for full details
  3. reviewed all implementations, aligned operations (all implementations uses shifts on start and not division), made a few improvements on the summing of the results.
  4. compared alternative functions by the assembly files for the best flow
  5. added more tests for the functions to get a full coverage.
  6. added a benchmark for all functions to check future attempts of improvements and confirm the optimization choosing logic

@GuyAv46 GuyAv46 changed the title Guyav space optimization fix Space Optimization Fix Jul 25, 2022
@codecov
Copy link

codecov bot commented Jul 25, 2022 •
edited
Loading

Codecov Report

Merging #175 (7f06605) into main (4910a33) will increase coverage by 0.07%.
The diff coverage is 94.77%.

@@            Coverage Diff             @@
##             main     #175      +/-   ##
==========================================
+ Coverage   93.46%   93.54%   +0.07%     
==========================================
  Files          40       40              
  Lines        2218     2338     +120     
==========================================
+ Hits         2073     2187     +114     
- Misses        145      151       +6
Impacted Files Coverage Δ
src/VecSim/spaces/L2_space.cpp 51.35% <45.45%> (-0.51%) ⬇️
src/VecSim/spaces/IP_space.cpp 48.64% <71.42%> (+4.89%) ⬆️
src/VecSim/spaces/IP/IP_AVX.cpp 100.00% <100.00%> (ø)
src/VecSim/spaces/IP/IP_AVX512.cpp 100.00% <100.00%> (ø)
src/VecSim/spaces/IP/IP_SSE.cpp 100.00% <100.00%> (ø)
src/VecSim/spaces/L2/L2_AVX.cpp 100.00% <100.00%> (ø)
src/VecSim/spaces/L2/L2_AVX512.cpp 100.00% <100.00%> (ø)
src/VecSim/spaces/L2/L2_SSE.cpp 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4910a33...7f06605. Read the comment docs.

@GuyAv46 GuyAv46 requested a review from alonre24 July 26, 2022 12:35
@GuyAv46 GuyAv46 marked this pull request as ready for review July 26, 2022 12:37
alonre24
alonre24 reviewed Jul 26, 2022
View reviewed changes
Copy link
Collaborator

@alonre24 alonre24 left a comment

Choose a reason for hiding this comment

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

Looks good! small comments

src/VecSim/spaces/L2_space.cpp Show resolved Hide resolved
tests/benchmark/CMakeLists.txt Outdated Show resolved Hide resolved
tests/benchmark/bm_spaces.cpp Outdated Show resolved Hide resolved
tests/benchmark/bm_spaces.cpp Outdated Show resolved Hide resolved
tests/benchmark/bm_spaces.cpp Show resolved Hide resolved
alonre24
alonre24 previously approved these changes Jul 27, 2022
View reviewed changes
Copy link
Collaborator

@alonre24 alonre24 left a comment

Choose a reason for hiding this comment

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

Great job!

DvirDukhan
DvirDukhan reviewed Jul 27, 2022
View reviewed changes
src/VecSim/spaces/IP/IP_AVX512.cpp Show resolved Hide resolved
src/VecSim/spaces/IP/IP_AVX512.cpp Show resolved Hide resolved
tests/unit/test_spaces.cpp Show resolved Hide resolved
@GuyAv46 GuyAv46 force-pushed the guyav-space_optimization_fix branch from d833534 to 5bf131e Compare July 27, 2022 14:38
alonre24
alonre24 reviewed Jul 28, 2022
View reviewed changes
src/VecSim/spaces/IP/IP_AVX512.cpp Show resolved Hide resolved
src/VecSim/spaces/IP_space.cpp Show resolved Hide resolved
tests/benchmark/bm_spaces.cpp Outdated Show resolved Hide resolved
alonre24
alonre24 previously approved these changes Jul 28, 2022
View reviewed changes
DvirDukhan
DvirDukhan reviewed Jul 28, 2022
View reviewed changes
Copy link
Collaborator

@DvirDukhan DvirDukhan left a comment

Choose a reason for hiding this comment

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

small comment
also, please elaborate on all the modifications done in this PR in the PR description

tests/unit/test_spaces.cpp Outdated Show resolved Hide resolved
@GuyAv46 GuyAv46 requested a review from DvirDukhan July 28, 2022 16:25
@GuyAv46 GuyAv46 merged commit 4d6a240 into main Jul 28, 2022
@GuyAv46 GuyAv46 deleted the guyav-space_optimization_fix branch July 28, 2022 19:58
GuyAv46 added a commit that referenced this pull request Jul 31, 2022
@GuyAv46
Space Optimization Fix (#175)
54cc3fa 
* improving functions

* improving logic of optimization choosing

* first metrics benchmark

* improved benchmark

* added tests for all new functions

* improved benchmark and added comments

* added logic explanation

* fixing alignment

* added bm_spaces to "make benchmark" run

* added arch check in compile time

* refactor benchmark file

* comment fix
GuyAv46 added a commit that referenced this pull request Jul 31, 2022
@GuyAv46
Space Optimization Fix (#175)
c05d3e9 
* improving functions

* improving logic of optimization choosing

* first metrics benchmark

* improved benchmark

* added tests for all new functions

* improved benchmark and added comments

* added logic explanation

* fixing alignment

* added bm_spaces to "make benchmark" run

* added arch check in compile time

* refactor benchmark file

* comment fix
GuyAv46 added a commit that referenced this pull request Jul 31, 2022
@GuyAv46
Space Optimization Fix (#175) (#179)
9dde61f 
* improving functions

* improving logic of optimization choosing

* first metrics benchmark

* improved benchmark

* added tests for all new functions

* improved benchmark and added comments

* added logic explanation

* fixing alignment

* added bm_spaces to "make benchmark" run

* added arch check in compile time

* refactor benchmark file

* comment fix
GuyAv46 added a commit that referenced this pull request Jul 31, 2022
@GuyAv46
Space Optimization Fix (#175) (#180)
8e88725 
* improving functions

* improving logic of optimization choosing

* first metrics benchmark

* improved benchmark

* added tests for all new functions

* improved benchmark and added comments

* added logic explanation

* fixing alignment

* added bm_spaces to "make benchmark" run

* added arch check in compile time

* refactor benchmark file

* comment fix
GuyAv46 added a commit that referenced this pull request Aug 1, 2022
@GuyAv46 @DvirDukhan
Fixing benchmark run for #175 (#182)
ad6a57d 
* compile bm_saces class without optimizations

* changed names and moved functions

* improved benchmarks definitions

Co-authored-by: DvirDukhan <dvir@redis.com>
GuyAv46 added a commit that referenced this pull request Aug 1, 2022
@GuyAv46 @DvirDukhan
Fixing benchmark run for #175 (#182)
fed2f48 
* compile bm_saces class without optimizations

* changed names and moved functions

* improved benchmarks definitions

Co-authored-by: DvirDukhan <dvir@redis.com>
GuyAv46 added a commit that referenced this pull request Aug 1, 2022
@GuyAv46 @DvirDukhan
Fixing benchmark run for #175 (#182)
dfbbe31 
* compile bm_saces class without optimizations

* changed names and moved functions

* improved benchmarks definitions

Co-authored-by: DvirDukhan <dvir@redis.com>
GuyAv46 added a commit that referenced this pull request Aug 1, 2022
@GuyAv46 @DvirDukhan
Fixing benchmark run for #175 (#182) (#188)
b31fb91 
* compile bm_saces class without optimizations

* changed names and moved functions

* improved benchmarks definitions

Co-authored-by: DvirDukhan <dvir@redis.com>
GuyAv46 added a commit that referenced this pull request Aug 1, 2022
@GuyAv46 @DvirDukhan
Fixing benchmark run for #175 (#182) (#189)
7fae91f 
* compile bm_saces class without optimizations

* changed names and moved functions

* improved benchmarks definitions

Co-authored-by: DvirDukhan <dvir@redis.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DvirDukhan DvirDukhan DvirDukhan approved these changes

@alonre24 alonre24 alonre24 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@GuyAv46 @DvirDukhan @alonre24