remove alignedslice and replace with direct poly calls#994
remove alignedslice and replace with direct poly calls#994JordanMaples merged 4 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR completes the follow-up to #907 by removing the AlignedSlice/aligned_slice abstraction and migrating call sites to use Poly::broadcast directly, while also adding pre-defined alignment/power-of-two constants to simplify common allocations.
Changes:
- Remove
AlignedSlicetype alias andaligned_slice()constructor fromdiskann-quantization::allocand update all call sites to usePoly::broadcast. - Add
PowerOfTwoconsts (P0..P63+ value-mappedV*) andAlignedAllocatorconsts (A4..A4096) for common alignments. - Drop unused workspace dependency (
once_cell).
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| diskann-quantization/src/num.rs | Adds many PowerOfTwo constants (P*/V*) to avoid repeated runtime construction. |
| diskann-quantization/src/alloc/mod.rs | Stops exporting AlignedSlice/aligned_slice from the public alloc module API. |
| diskann-quantization/src/alloc/aligned_slice.rs | Deletes the alias + constructor and the associated tests. |
| diskann-quantization/src/alloc/aligned.rs | Adds AlignedAllocator::{A4..A4096} convenience constants. |
| diskann-disk/src/utils/aligned_file_reader/windows_aligned_file_reader.rs | Updates tests to allocate aligned buffers via Poly::broadcast + AlignedAllocator. |
| diskann-disk/src/utils/aligned_file_reader/storage_provider_aligned_file_reader.rs | Same as above for storage-provider aligned reader tests. |
| diskann-disk/src/utils/aligned_file_reader/linux_aligned_file_reader.rs | Same as above for Linux aligned reader tests. |
| diskann-disk/src/search/provider/disk_vertex_provider_factory.rs | Switches header read buffer allocation to Poly::broadcast. |
| diskann-disk/src/search/provider/disk_sector_graph.rs | Replaces AlignedSlice<u8> storage with Poly<[u8], AlignedAllocator> and allocates via Poly::broadcast. |
| diskann-disk/src/search/pq/pq_scratch.rs | Replaces aligned scratch buffers with Poly::broadcast allocations. |
| diskann-disk/benches/benchmarks_iai/aligned_file_reader_bench_iai.rs | Updates benchmark allocations to Poly::broadcast. |
| diskann-disk/benches/benchmarks/aligned_file_reader_bench.rs | Updates benchmark allocations to Poly::broadcast. |
| Cargo.toml | Removes once_cell from workspace dependencies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #994 +/- ##
==========================================
- Coverage 89.43% 89.42% -0.01%
==========================================
Files 449 448 -1
Lines 83779 83743 -36
==========================================
- Hits 74928 74890 -38
- Misses 8851 8853 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
@copilot could you review again? I believe that I've fully addressed your comments |
All five issues from the previous review have been addressed in commit
Clippy and tests pass cleanly. |
I synced up with @hildebrandmw after he returned from vacation and learned that I misunderstood the brief of #907 with my #965 pr. Rather than removing the struct and replace it with a type-alias for slightly less overhead, the ask was to just replace all calls. So, this follow-up PR does just that and removes AlignedSlice & aligned_slice and replaces all sites with Poly::broadcast calls.
This also introduces PowerOfTwo consts for 0..64 P and value mappings and AlignedAllocator consts for 4...4096