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

Unified search layer #310

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

op-hunter
Copy link

The function searchBaseLayer and searchBaseLayerST has almost the same code, this pull request unified the two functions.

cmli added 3 commits April 27, 2021 11:23
Signed-off-by: cmli <chengming.li@zilliz.com>
Signed-off-by: cmli <chengming.li@zilliz.com>
Signed-off-by: cmli <chengming.li@zilliz.com>
@op-hunter
Copy link
Author

@yurymalkov Can you help me with this pr? Error: "ImportError: cannot import name 'NoReturn'" occurs.

@yurymalkov
Copy link
Member

Thanks for the PR! It seems like a python 3.6 error. I can look into it.

From the point of the PR, I wonder if you've benchmarked the performance of the single-threaded version (e.g. check that performance is the same/better)?
The main intention to separate them is to avoid locks/branching in the search code (which isn't really need them unless there are updates at the same time).

@op-hunter
Copy link
Author

op-hunter commented May 7, 2021

I did a simple benchmark on my machine with 32G memory.
Index parametes: efConstruction = 200, M = 48, efSearch = 128. nq = 10000
Dataset: sift-1m, dimension = 128.
It seems that there is no much difference.
Test with multithread:

branch master unified search layer
build cost 293890 295734
query cost 2140 2053

Test with single thread:

branch master unified search layer
build cost 1854470 1849888
query cost 12939 12951

Note that the unit of time is millisecond.

@yurymalkov
Copy link
Member

Thank you for the test! I usually test it with low dim (say d=4), so to make evident the bottlenecks outside of the distance function computation.
I'll do my typical tests for the change the following week and will get back to you.

@yurymalkov
Copy link
Member

Hey @op-hunter ,
I've finally checked the performance for d=4 and it seems like there is a slowdown by more than 10% in case of large ef values (e.g. searching 1M vectors is 26.5k/s in single thread for ef=128 and only 20.8 k/s for the proposed change).

I think this can be solved by using templates. I wonder if there any way to make conditional lock creation?

@yurymalkov
Copy link
Member

Maybe even pointers are fine. Not sure if there is a clean alternative.

@op-hunter
Copy link
Author

@yurymalkov How about use std::defer_lock to delay the lock operation?
line 198:

std::unique_lock<std::mutex> lk(link_list_locks_[current_node_id], std::defer_lock);
if (!is_st) lk.lock();

@yurymalkov
Copy link
Member

Hm. I can check that.

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.

2 participants