镜像站点 · 本页由第三方 GitHub 只读镜像提供,非 GitHub 官方站点,不接受任何登录或凭据输入。前往 github.com
Skip to content

[diskann] Add aarch64 NeighborPriortyQueue.get_lower_bound - #1512

Merged
Mark Hildebrand (hildebrandmw) merged 3 commits into
microsoft:mainfrom
pfoxARM:pfoxARM/get_lower_bound
Oct 30, 2026
Merged

Mark Hildebrand (hildebrandmw) merged 3 commits into
microsoft:mainfrom
pfoxARM:pfoxARM/get_lower_bound

Conversation

@pfoxARM

@pfoxARM pfoxARM commented Oct 26, 2026

Copy link
Copy Markdown
Contributor

Add an AArch64-specific implementation of get_lower_bound that uses a scalar linear search.

Benchmarks showed that scalar linear search gives the best overall QPS across different vector dimensions (dim128, dim256, dim768) and L sizes (10,20...,80).

We tested several alternatives:

  • NEON SIMD (vcgeq_f32)
  • Scalar unrolled search
  • Rust partition_point
  • Manual binary search

It was clear from testing that a scalar linear approach is best.

@pfoxARM
pfoxARM requested review from a team and a balanced review from Copilot October 26, 2026 15:23

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The architecture guards leave other targets without get_lower_bound, causing missing-method compilation errors.

1 open issue
What changed in this PR

This PR specializes the neighbor priority queue’s lower-bound search for AArch64, using scalar linear search based on the reported benchmark results.

Changes:

  • Adds an AArch64 scalar implementation of get_lower_bound.
  • Restricts the existing SIMD implementation and its imports to x86-64.
File Description
diskann/​src/​neighbor/​queue.rs Adds architecture-specific lower-bound implementations and gates SIMD imports.

Review effort: Balanced

Comment thread diskann/src/neighbor/queue.rs Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks! One very tiny suggestion from me.

Comment thread diskann/src/neighbor/queue.rs Outdated
@codecov-commenter

Codecov Comments Bot (codecov-commenter) commented Oct 28, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.07%. Comparing base (448e520) to head (dbb571a).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1512      +/-   ##
==========================================
+ Coverage   91.02%   92.07%   +1.04%     
==========================================
  Files         602      602              
  Lines      124080   124080              
==========================================
+ Hits       112948   114245    +1297     
+ Misses      11132     9835    -1297     
Flag Coverage Δ
miri 92.07% <ø> (+1.04%) ⬆️
unittests 92.01% <ø> (+1.28%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
diskann/src/neighbor/queue.rs 98.93% <ø> (ø)

... and 50 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks - looks good to me! CI failure should be taken care of by merging/rebasing with the latest main. Apologies for the flakiness.

pfoxARM and others added 3 commits October 29, 2026 22:25
@pfoxARM
pfoxARM force-pushed the pfoxARM/get_lower_bound branch from 2c73395 to dbb571a Compare October 29, 2026 22:25
@pfoxARM

pfoxARM commented Oct 29, 2026

Copy link
Copy Markdown
Contributor Author

No worries, hopefully CI passes now :)

@metajack

Copy link
Copy Markdown
Contributor

Note that openai v3 vectors at 3072 dimensions, and naive binary search will win there I think. At least it did last I benchmarked this. In the future I think we will have some way to dynamically select an NPQ algorithm based on dimension, but for now matching the existing thing is probably the right call.

@metajack

Copy link
Copy Markdown
Contributor

It seems quite surprising that a) our fallback is much less efficient and b) not using SIMD is as good as doing the work 8x as fast?

@hildebrandmw

Copy link
Copy Markdown
Contributor

Jack Moffitt (@metajack) - The big issue with the SIMD approach on aarch64 is that .first() is surprisingly expensive on aarch64, where-as it's much cheaper on x86. We could do some work on the masks types for aarch64 to maybe get it a bit faster, but it's still unfortunately not free.

@hildebrandmw
Mark Hildebrand (hildebrandmw) merged commit 65443a6 into microsoft:main Oct 30, 2026
31 checks passed
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.

5 participants