diff --git a/AGENT_CHAT.md b/AGENT_CHAT.md index 9c3562b1b..9ed5dada2 100644 --- a/AGENT_CHAT.md +++ b/AGENT_CHAT.md @@ -1,5 +1,10 @@ # RedlineDB Agent Chat +## 2026-09-25T20:32Z fix/simd-length-fma + +Claim: canonical checkout `/home/ubuntu/redlineDB`, branch `fix/simd-length-fma` from `origin/main` `a9c5e5297feee61d3887eeeecd4dfd1b4e897975`. Distance kernels enter the AVX2+FMA path only when both features are present, and they refuse unequal slice lengths before any vector load. No speed ratio. No worktree. + + ## 2026-09-25T20:05Z fix/txn-drop-row-locks Claim: canonical checkout `/home/ubuntu/redlineDB`, branch `fix/txn-drop-row-locks` from `origin/main` `a58bbb649f893b0ed00c6a4a0c46e44358d5bcb1`. Dropping an open transaction aborts it and releases the row locks it still holds. Commit and rollback already release those locks before the transaction closes, and that order stays. No speed ratio. No worktree. diff --git a/crates/kernel/src/vector/distance.rs b/crates/kernel/src/vector/distance.rs index b9f2949a7..4c1180319 100644 --- a/crates/kernel/src/vector/distance.rs +++ b/crates/kernel/src/vector/distance.rs @@ -4,13 +4,22 @@ //! `simd.rs`. They are deliberately written in the simplest, easiest-to-audit //! form. Performance-sensitive callers should go through [`super::simd`]. //! -//! All kernels assume the input slices have equal, non-zero length; the public -//! entry points in `super` already enforce that contract. +//! Release builds assert equal length before indexing so a shorter slice is +//! not read past. /// Alias matching the name used by the HNSW lane (V2). The canonical type is /// [`super::VectorMetric`]. pub type Metric = super::VectorMetric; +#[inline] +pub(crate) fn require_equal_len(a: &[f32], b: &[f32]) { + assert_eq!( + a.len(), + b.len(), + "distance slices must have the same length" + ); +} + /// Squared L2 distance: `Σ (a_i - b_i)^2`. /// /// We intentionally do **not** take the square root: top-K ordering is @@ -18,7 +27,7 @@ pub type Metric = super::VectorMetric; /// in tight loops (and matches pgvector's `<->` operator implementation). #[inline] pub fn l2_distance_scalar(a: &[f32], b: &[f32]) -> f32 { - debug_assert_eq!(a.len(), b.len()); + require_equal_len(a, b); let mut acc = 0.0_f32; for i in 0..a.len() { let d = a[i] - b[i]; @@ -34,7 +43,7 @@ pub fn l2_distance_scalar(a: &[f32], b: &[f32]) -> f32 { /// inspect the input themselves. #[inline] pub fn cosine_distance_scalar(a: &[f32], b: &[f32]) -> f32 { - debug_assert_eq!(a.len(), b.len()); + require_equal_len(a, b); let mut dot = 0.0_f32; let mut na = 0.0_f32; let mut nb = 0.0_f32; @@ -53,7 +62,7 @@ pub fn cosine_distance_scalar(a: &[f32], b: &[f32]) -> f32 { /// Negative inner product (smaller is "closer", matches pgvector `<#>`). #[inline] pub fn inner_product_scalar(a: &[f32], b: &[f32]) -> f32 { - debug_assert_eq!(a.len(), b.len()); + require_equal_len(a, b); let mut acc = 0.0_f32; for i in 0..a.len() { acc += a[i] * b[i]; diff --git a/crates/kernel/src/vector/mod.rs b/crates/kernel/src/vector/mod.rs index 902d916ab..4330b6a53 100644 --- a/crates/kernel/src/vector/mod.rs +++ b/crates/kernel/src/vector/mod.rs @@ -18,9 +18,9 @@ //! # SIMD dispatch //! //! At compile time, AVX2 and NEON impls are gated on `target_feature` / -//! `target_arch`. At run time, the AVX2 path additionally checks -//! `is_x86_feature_detected!("avx2")` so binaries built without `-Ctarget-cpu` -//! still benefit when the host actually supports AVX2. +//! `target_arch`. At run time, the AVX2+FMA path checks both +//! `avx2` and `fma`, and every kernel asserts equal slice length before it +//! loads. //! //! See `simd.rs` for the dispatcher and `distance.rs` for the scalar //! reference implementations used as a correctness oracle. diff --git a/crates/kernel/src/vector/simd.rs b/crates/kernel/src/vector/simd.rs index 79e94255a..563558f65 100644 --- a/crates/kernel/src/vector/simd.rs +++ b/crates/kernel/src/vector/simd.rs @@ -3,14 +3,24 @@ //! Public functions ([`l2_distance`], [`cosine_distance`], [`inner_product`]) //! pick the best implementation available for the host: //! -//! * `x86_64` with AVX2 at runtime -> 8-wide AVX2 + FMA path. +//! * `x86_64` with AVX2 and FMA at runtime -> 8-wide AVX2 + FMA path. //! * `aarch64` (NEON is mandatory in the AArch64 base ISA) -> 4-wide NEON path. //! * Everything else -> scalar implementation from [`super::distance`]. //! //! The AVX2 and NEON kernels live in `simd/avx2.rs` and `simd/neon.rs`; this //! module keeps dispatch and the stable public API in one place. -use super::distance::{cosine_distance_scalar, inner_product_scalar, l2_distance_scalar}; +use super::distance::{ + cosine_distance_scalar, inner_product_scalar, l2_distance_scalar, require_equal_len, +}; + +/// AVX2 kernels also execute FMA instructions. AVX2 without FMA stays on the +/// scalar path. +#[cfg(any(target_arch = "x86", target_arch = "x86_64"))] +#[inline] +fn host_avx2_fma() -> bool { + std::is_x86_feature_detected!("avx2") && std::is_x86_feature_detected!("fma") +} #[cfg(any(target_arch = "x86", target_arch = "x86_64"))] #[path = "simd/avx2.rs"] @@ -25,13 +35,11 @@ mod tests; /// Squared L2 distance with SIMD dispatch. #[inline] pub fn l2_distance(a: &[f32], b: &[f32]) -> f32 { - debug_assert_eq!(a.len(), b.len()); + require_equal_len(a, b); #[cfg(any(target_arch = "x86", target_arch = "x86_64"))] { - if std::is_x86_feature_detected!("avx2") { - // Runtime CPU dispatch upheld AVX2; bounds checked by inner loop; - // inputs are equal-length slices (debug_asserted above). - // SAFETY: `is_x86_feature_detected!("avx2")` returned true above, satisfying the `target_feature = "avx2,fma"` precondition of `l2_distance_avx2`. + if host_avx2_fma() { + // SAFETY: `host_avx2_fma` proved AVX2 and FMA. Lengths were asserted equal. unsafe { return avx2::l2_distance_avx2(a, b); } @@ -39,9 +47,7 @@ pub fn l2_distance(a: &[f32], b: &[f32]) -> f32 { } #[cfg(target_arch = "aarch64")] { - // NEON is mandatory in the AArch64 base ISA; bounds checked by inner - // loop; inputs are equal-length slices (debug_asserted above). - // SAFETY: AArch64 base ISA includes NEON, satisfying the `target_feature = "neon"` precondition of `neon::l2_distance_neon`. + // SAFETY: AArch64 base ISA includes NEON. Lengths were asserted equal. unsafe { return neon::l2_distance_neon(a, b); } @@ -53,13 +59,11 @@ pub fn l2_distance(a: &[f32], b: &[f32]) -> f32 { /// Cosine distance with SIMD dispatch. #[inline] pub fn cosine_distance(a: &[f32], b: &[f32]) -> f32 { - debug_assert_eq!(a.len(), b.len()); + require_equal_len(a, b); #[cfg(any(target_arch = "x86", target_arch = "x86_64"))] { - if std::is_x86_feature_detected!("avx2") { - // Runtime CPU dispatch upheld AVX2; bounds checked by inner loop; - // inputs are equal-length slices (debug_asserted above). - // SAFETY: `is_x86_feature_detected!("avx2")` returned true above, satisfying the `target_feature = "avx2,fma"` precondition of `cosine_distance_avx2`. + if host_avx2_fma() { + // SAFETY: `host_avx2_fma` proved AVX2 and FMA. Lengths were asserted equal. unsafe { return avx2::cosine_distance_avx2(a, b); } @@ -67,9 +71,7 @@ pub fn cosine_distance(a: &[f32], b: &[f32]) -> f32 { } #[cfg(target_arch = "aarch64")] { - // NEON is mandatory in the AArch64 base ISA; inputs are equal-length - // slices (debug_asserted above). - // SAFETY: AArch64 base ISA includes NEON, satisfying the `target_feature = "neon"` precondition of `neon::cosine_distance_neon`. + // SAFETY: AArch64 base ISA includes NEON. Lengths were asserted equal. unsafe { return neon::cosine_distance_neon(a, b); } @@ -81,13 +83,11 @@ pub fn cosine_distance(a: &[f32], b: &[f32]) -> f32 { /// Negative inner product with SIMD dispatch. #[inline] pub fn inner_product(a: &[f32], b: &[f32]) -> f32 { - debug_assert_eq!(a.len(), b.len()); + require_equal_len(a, b); #[cfg(any(target_arch = "x86", target_arch = "x86_64"))] { - if std::is_x86_feature_detected!("avx2") { - // Runtime CPU dispatch upheld AVX2; inputs are equal-length - // slices (debug_asserted above). - // SAFETY: `is_x86_feature_detected!("avx2")` returned true above, satisfying the `target_feature = "avx2,fma"` precondition of `inner_product_avx2`. + if host_avx2_fma() { + // SAFETY: `host_avx2_fma` proved AVX2 and FMA. Lengths were asserted equal. unsafe { return avx2::inner_product_avx2(a, b); } @@ -95,9 +95,7 @@ pub fn inner_product(a: &[f32], b: &[f32]) -> f32 { } #[cfg(target_arch = "aarch64")] { - // NEON is mandatory in the AArch64 base ISA; inputs are equal-length - // slices (debug_asserted above). - // SAFETY: AArch64 base ISA includes NEON, satisfying the `target_feature = "neon"` precondition of `neon::inner_product_neon`. + // SAFETY: AArch64 base ISA includes NEON. Lengths were asserted equal. unsafe { return neon::inner_product_neon(a, b); } diff --git a/crates/kernel/src/vector/simd/avx2.rs b/crates/kernel/src/vector/simd/avx2.rs index d17a8a00e..5d9e12efc 100644 --- a/crates/kernel/src/vector/simd/avx2.rs +++ b/crates/kernel/src/vector/simd/avx2.rs @@ -14,10 +14,10 @@ type Avx2Vec = std::arch::x86_64::__m256; /// # Safety /// /// The caller must only invoke this on a CPU where AVX2 and FMA are -/// available. The dispatcher in `simd.rs` enforces that by checking -/// `is_x86_feature_detected!("avx2")` first. +/// available. The dispatcher in `simd.rs` checks both +/// `avx2` and `fma` before calling. The loop walks `min(a.len(), b.len())`. pub(super) unsafe fn l2_distance_avx2(a: &[f32], b: &[f32]) -> f32 { - let len = a.len(); + let len = a.len().min(b.len()); let lanes = 8; let mut acc = _mm256_setzero_ps(); let mut i = 0; @@ -50,10 +50,10 @@ pub(super) unsafe fn l2_distance_avx2(a: &[f32], b: &[f32]) -> f32 { /// # Safety /// /// The caller must only invoke this on a CPU where AVX2 and FMA are -/// available. The dispatcher in `simd.rs` enforces that by checking -/// `is_x86_feature_detected!("avx2")` first. +/// available. The dispatcher in `simd.rs` checks both +/// `avx2` and `fma` before calling. The loop walks `min(a.len(), b.len())`. pub(super) unsafe fn cosine_distance_avx2(a: &[f32], b: &[f32]) -> f32 { - let len = a.len(); + let len = a.len().min(b.len()); let lanes = 8; let mut dot = _mm256_setzero_ps(); let mut na = _mm256_setzero_ps(); @@ -98,10 +98,10 @@ pub(super) unsafe fn cosine_distance_avx2(a: &[f32], b: &[f32]) -> f32 { /// # Safety /// /// The caller must only invoke this on a CPU where AVX2 and FMA are -/// available. The dispatcher in `simd.rs` enforces that by checking -/// `is_x86_feature_detected!("avx2")` first. +/// available. The dispatcher in `simd.rs` checks both +/// `avx2` and `fma` before calling. The loop walks `min(a.len(), b.len())`. pub(super) unsafe fn inner_product_avx2(a: &[f32], b: &[f32]) -> f32 { - let len = a.len(); + let len = a.len().min(b.len()); let lanes = 8; let mut acc = _mm256_setzero_ps(); let mut i = 0; diff --git a/crates/kernel/src/vector/simd/neon.rs b/crates/kernel/src/vector/simd/neon.rs index b78be4d47..4cd321963 100644 --- a/crates/kernel/src/vector/simd/neon.rs +++ b/crates/kernel/src/vector/simd/neon.rs @@ -9,7 +9,7 @@ use std::arch::aarch64::*; /// available. The dispatcher in `simd.rs` relies on the fact that NEON is /// part of the AArch64 base ISA. pub(super) unsafe fn l2_distance_neon(a: &[f32], b: &[f32]) -> f32 { - let len = a.len(); + let len = a.len().min(b.len()); let lanes = 4; let mut acc = vdupq_n_f32(0.0); let mut i = 0; @@ -44,7 +44,7 @@ pub(super) unsafe fn l2_distance_neon(a: &[f32], b: &[f32]) -> f32 { /// available. The dispatcher in `simd.rs` relies on the fact that NEON is /// part of the AArch64 base ISA. pub(super) unsafe fn cosine_distance_neon(a: &[f32], b: &[f32]) -> f32 { - let len = a.len(); + let len = a.len().min(b.len()); let lanes = 4; let mut dot = vdupq_n_f32(0.0); let mut na = vdupq_n_f32(0.0); @@ -87,7 +87,7 @@ pub(super) unsafe fn cosine_distance_neon(a: &[f32], b: &[f32]) -> f32 { /// available. The dispatcher in `simd.rs` relies on the fact that NEON is /// part of the AArch64 base ISA. pub(super) unsafe fn inner_product_neon(a: &[f32], b: &[f32]) -> f32 { - let len = a.len(); + let len = a.len().min(b.len()); let lanes = 4; let mut acc = vdupq_n_f32(0.0); let mut i = 0; diff --git a/crates/kernel/src/vector/simd/tests.rs b/crates/kernel/src/vector/simd/tests.rs index 443c3524c..56a5faa4a 100644 --- a/crates/kernel/src/vector/simd/tests.rs +++ b/crates/kernel/src/vector/simd/tests.rs @@ -28,6 +28,24 @@ fn rand_vec(n: usize, seed: u64) -> Vec { .collect() } +#[cfg(any(target_arch = "x86", target_arch = "x86_64"))] +#[test] +fn avx2_kernel_is_armed_only_when_fma_is_present() { + let avx2 = std::is_x86_feature_detected!("avx2"); + let fma = std::is_x86_feature_detected!("fma"); + assert_eq!(super::host_avx2_fma(), avx2 && fma); +} + +#[test] +fn unequal_lengths_panic_before_a_vector_load() { + let long = [1.0_f32; 9]; + let short = [1.0_f32; 2]; + assert!(std::panic::catch_unwind(|| l2_distance(&long, &short)).is_err()); + assert!(std::panic::catch_unwind(|| cosine_distance(&long, &short)).is_err()); + assert!(std::panic::catch_unwind(|| inner_product(&short, &long)).is_err()); + assert!(std::panic::catch_unwind(|| l2_distance_scalar(&long, &short)).is_err()); +} + #[test] fn simd_matches_scalar_l2_various_dims() { for &dim in &[8usize, 64, 128, 512, 1536] {