Skip to content

Conversation

pierd
Copy link

@pierd pierd commented Feb 22, 2023

Addresses #20

Changes

  • Adds the ability to benchmark instant-distance-py crate
  • Adds custom storage support
  • Introduces Metric trait
  • Introduces PointStorage to prevent extra dereference when getting to data
  • Makes Euclid metric work on variable length

Benchmark

I used building HNSW for the first 100k fasttext words as a benchmark (benchmarking script for reference).

The original implementation takes around 18.7s (min out of 20 runs). After changes it takes 19.5s (again min out of 20 runs). Roughly 4.2% slower, I tried profiling it but I'm not sure where it comes from.

As for the querying (10k queries per benchmark): original takes 4.8s and the new version 4.5s. So it actually looks like it's slightly faster.

Benchmarking directly in rust (added in 1st commit of this PR)

Original (almost, with benchmarking added):

test build    ... bench: 103,131,796 ns/iter (+/- 12,592,833)
test distance ... bench:          21 ns/iter (+/- 1)
test query    ... bench:      42,648 ns/iter (+/- 9,372)

After changes:

test build    ... bench: 120,487,512 ns/iter (+/- 13,330,122)
test distance ... bench:          21 ns/iter (+/- 5)
test query    ... bench:      43,210 ns/iter (+/- 10,657)

This time building seems to be 16.8% slower (less visible when using through Python). Querying is comparable.

@djc
Copy link
Owner

djc commented Feb 23, 2023

Is the regression for building or for querying? We should ideally benchmark both. I think we'd be willing to take a small regression on building but not regressing querying is actually more important. Also would be good to take a quick look at the impact on in-memory size.

@pierd
Copy link
Author

pierd commented Feb 23, 2023

I've just run some benchmarks for querying. There's a lot of variance in the results, I think it's coming from Python's overhead since I'm running search 10k times and it takes a few seconds. But even with that variance I can clearly see that making the distance calculation not bound to vector length regresses querying slightly and then actually using Vec in vectors regresses it further.

For original the shortest loop took 3.227s, after updating the metric it grew to 5.059s and after using AVec it went up to 5.596s. All those are the shortest times out of 40 runs (each run doing 10k searches).

@djc
Copy link
Owner

djc commented Feb 24, 2023

Right, those are definitely some substantial latency regressions -- so I don't think we want to go this route.

(Probably not necessary right now, but it might be interesting to test the low-level metric stuff with Rust benchmarks instead of the Python bindings? Would get rid of the Python noise at least.)

@pierd pierd force-pushed the n-len-vecs branch 2 times, most recently from 8b307d2 to afad778 Compare March 27, 2023 21:12
@pierd pierd force-pushed the n-len-vecs branch 2 times, most recently from 8c7374c to e4ce915 Compare March 28, 2023 17:36
@djc
Copy link
Owner

djc commented May 8, 2023

Should keep this issue in mind:

#35

@pierd
Copy link
Author

pierd commented May 8, 2023

I think I'd prefer to continue with this one and close/rework #35 once this PR is merged.

@djc
Copy link
Owner

djc commented May 8, 2023

Why is that? Do you think you'll still have time to work on this? Hope the new job is working out well for you!

@pierd
Copy link
Author

pierd commented May 11, 2023

The new job is great. I'm still hoping to finish this but the last few months are proving me wrong.

@djc
Copy link
Owner

djc commented May 12, 2023

Ambient looks cool, BTW. How about we just say that we're going to pick it up, and if you get to it before us that's nice, but we'll both assume that you have enough other things on your plate as it is? 🙂

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