-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Update sparse vector benchmarks #4163
Conversation
parking_lot = { version = "0.12.2", features = ["deadlock_detection", "serde"] } | ||
pprof = { version = "0.12", features = ["flamegraph", "prost-codec"] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean, that it is non-dev dependency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a workspace dependency, to be referred like this: pprof = { workspace = true }
.
In this PR, it's referred only inside of dev-dependencies
of collection
, dataset
, and segment
crates.
So, no.
lib/sparse/src/index/loaders.rs
Outdated
let (nrow, _ncol, nnz) = (*nrow as usize, *ncol as usize, *nnz as usize); | ||
|
||
let indptr = Vec::from(transmute_from_u8_to_slice::<u64>( | ||
&mmap.as_ref()[24..24 + 8 * (nrow + 1)], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what those numbers mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It recreates the format being used in https://github.com/qdrant/sparse-vectors-benchmark/blob/master/src/sparse_matrix.py.
Added an explanation table in a doc comment.
lib/sparse/benches/search.rs
Outdated
let query_vectors = | ||
loaders::load_csr_vecs(Dataset::AnnChallengeQueries.download().unwrap()).unwrap(); | ||
|
||
let index_1m = load_csr_index(Dataset::AnnChallenge1M.download().unwrap(), 1.0).unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are those msmarco?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the same benchmark used in https://github.com/qdrant/sparse-vectors-benchmark. It's embeddings based on MS MARCO. Renamed to NeurIps2023
as it would be accurate name.
&mut self, | ||
permit: Arc<CpuPermit>, | ||
stopped: &AtomicBool, | ||
tick_progress: impl FnMut(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it doesn't need to be Mut
. If we use Atomics to count (and it looks like we do) - impl Fn
should be enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to accept the broadest possible type of closure, which is FnMut
in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be that having FnMut can prevent some compiler-level optimizations
} | ||
} | ||
|
||
impl<'a> Profiler for FlamegraphProfiler<'a> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what alternative do we have to avoid copy pasting this file in the future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Put it in a separate crate.
- Low-effort hack solution: have it in one place and have multiple symlink to it.
- Get rid of it. Personally, I haven't found use for it yet as I'm using external profilers, e.g.
perf
+hotspot
orvtune
. I've just added it because benches in other crates have it.
1358afd
to
b582dbb
Compare
This PR brings a new set of benchmarks to
segment
andsparse
crates, based on downloadable datasets.These benchmarks would serve as a baseline to evaluate the performance of the new sparse index format (#4143).
There were a few problems with the current benchmark in the
segment
crate:We don't want to let CPU caches do a favor to less memory-efficient algorithms.
Changes
This PR contains the following changes:
lib/common/dataset
to download testing datasets during benchmarking.target/datasets
directory.Maybe
sh -c 'wget $URL | gunzip > $FILE'
would simplify everything, but I've tried to make it as hassle-free as possible (think of Windows users).I.e. ANN Challenge (based on MS MARCO), and Splade/Wiki Movie Plots.
lib/sparse/src/index/loaders.rs
sparse
crate.random-50k
,random-500k
,ann-1M
,ann-full-25pct
,movies
.The hottest vectors are short (4 or 5 elements) and contain the most frequent index in the dataset.
Hottest vectors are the best-case scenario for pruning optimization.
sparse
crate is tiny and lacks of dependencies, these benchmarks are fast to compile.sparse_index_search
in thesegment
crate.random-50k
,ann-1M
.random-500k
, but I've dropped them from this PR since they're not so interesting.tick_progress: impl FnMut()
tosegment::index::VectorIndex::build_index()
and related places.|| ()
get optimized out.Results
I've used these benchmarks to check whether the pruning optimization gives any performance benefits nowadays.
Turns out, it doesn't, for tested datasets.
I think it's safe to assume that it is safe to drop this optimization.
(Note: this PR doesn't disable pruning)
cargo bench -p segment
cargo bench -p segment; with pruning disabled
cargo bench -p sparse
cargo bench -p sparse; with pruning disabled