-
Notifications
You must be signed in to change notification settings - Fork 378
Add performance diagnostic to diskann-benchmark
#744
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
base: main
Are you sure you want to change the base?
Conversation
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.
Pull request overview
This PR adds a compile-time performance diagnostic to diskann-benchmark by introducing an orderable “capability level” abstraction in diskann-wide, then using that to warn when the benchmark binary appears to be compiled for an insufficient micro-architecture target (notably x86-64 instead of x86-64-v3+), plus an informational warning on aarch64.
Changes:
- Add
arch::LevelandArchitecture::level()to enable ordering micro-architecture capability without instantiating anArchitecture. - Implement
level()forScalar,x86_64::V3, andx86_64::V4, plus add ordering tests. - Extend
diskann-benchmarkCLI with--quietand emit target-related performance warnings.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| diskann-wide/src/helpers.rs | Removes dead commented-out cfg_attr block. |
| diskann-wide/src/arch/x86_64/v3/mod.rs | Adds ordering support and exposes V3 capability via level(). |
| diskann-wide/src/arch/x86_64/v4/mod.rs | Adds ordering support and exposes V4 capability via level(). |
| diskann-wide/src/arch/x86_64/mod.rs | Introduces internal level ordering and tests for the ordering. |
| diskann-wide/src/arch/mod.rs | Adds Level type + Architecture::level() API and related docs. |
| diskann-wide/src/arch/emulated/mod.rs | Implements level() for Scalar. |
| diskann-benchmark/src/main.rs | Adds top-level Cli wrapper with --quiet and emits compile-target warnings; updates tests accordingly. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (78.84%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #744 +/- ##
==========================================
- Coverage 89.01% 88.99% -0.03%
==========================================
Files 428 428
Lines 78294 78368 +74
==========================================
+ Hits 69692 69742 +50
- Misses 8602 8626 +24
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Add a warning to
diskann-benchmarkif it looks like the binary was compiled for thex86-64target CPU instead ofx86-64-v3,x86-64-v4, or some other at-least AVX2 compatible CPU.Also add a warning for
aarch64noting that full SIMD support is not available yet.With
diskann-benchmarkbeing available now oncrates.io, I just know that this will be compiled and run with the defaultx86-64target CPU, when the preferred target is strongly at leastx86-64-v3.Implementation
Our architecture detection/selection is performed by
diskann-wide. As such, we can detect if the whole application has been compiled for anx86-64-v3compatible target by checking if the compile time architecturediskann_wide::arch::Currentis at leastdiskann_wide::arch::x86_64::V3. To do this comparison, a light-weight, opaquediskann_wide::arch::Levelis introduced to allowArchitectures to be ordered without instantiating the associated architecture via theArchitecture::levelassociated function.Checks for compatibility can then be done using
I think it's reasonable to add this to
diskann-widesincediskann-benchmarkis not the only crate that may need this functionality anddiskann-wideis really the source of truth when it comes to micro-architecture specific functionality.