From 29ff8c4e20438c25ff35c30ffd4e4629d6eadc6d Mon Sep 17 00:00:00 2001 From: Dag Brattli Date: Fri, 10 Jul 2026 12:34:17 +0200 Subject: [PATCH 1/5] perf(python): tune release profile and fix array benchmark harness Add a [profile.release] to fable-library-core (fat LTO, codegen-units=1, strip) so maturin release builds don't leave compiler optimizations on the table. Fix the array benchmark harness, which was entirely broken: the module-level `Array = array.FSharpArray[_T]` alias now resolves (via __class_getitem__) to a concrete typed subclass (GenericArray) whose __new__ rejects the `array_type` kwarg, so every constructor call raised TypeError. Use the unsubscripted class for construction (subscripting is still fine in annotation position). Extend the benchmarks to cover the paths the following perf work targets: typed-vs-generic aggregates (sum/average/min/max/contains on Int32/Float64) and a generic-vs-typed map, so each optimization can be measured before/after. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/fable-library-py/.gitignore | 1 + src/fable-library-py/Cargo.toml | 9 ++ .../tests-benchmark/test_bench_array.py | 131 +++++++++++++++++- 3 files changed, 138 insertions(+), 3 deletions(-) diff --git a/src/fable-library-py/.gitignore b/src/fable-library-py/.gitignore index 9ebbd6c985..b1fa2b1a4c 100644 --- a/src/fable-library-py/.gitignore +++ b/src/fable-library-py/.gitignore @@ -1,2 +1,3 @@ # Ignore compiled shared object files *.so +.benchmarks/ diff --git a/src/fable-library-py/Cargo.toml b/src/fable-library-py/Cargo.toml index 350c1383e2..2901613ece 100644 --- a/src/fable-library-py/Cargo.toml +++ b/src/fable-library-py/Cargo.toml @@ -20,3 +20,12 @@ regex = "1.12.4" [features] # must be enabled when building with `cargo build`, maturin enables this automatically extension-module = ["pyo3/extension-module"] + +# Release profile tuned for the extension module. maturin builds with `--release`, +# so it honors these settings. opt-level defaults to 3 for release; do NOT set +# `panic = "abort"` — PyO3 relies on unwinding to convert boundary panics into a +# Python `PanicException` rather than aborting the interpreter process. +[profile.release] +lto = "fat" +codegen-units = 1 +strip = true diff --git a/src/fable-library-py/tests-benchmark/test_bench_array.py b/src/fable-library-py/tests-benchmark/test_bench_array.py index 6f330e8cb9..c3e4709d84 100644 --- a/src/fable-library-py/tests-benchmark/test_bench_array.py +++ b/src/fable-library-py/tests-benchmark/test_bench_array.py @@ -15,8 +15,11 @@ _T = TypeVar("_T") -# Use Array as an alias for FSharpArray -Array = array.FSharpArray[_T] +# Use Array as an alias for FSharpArray. Must stay unsubscripted: subscripting +# (FSharpArray[_T]) now resolves to a concrete typed subclass (e.g. GenericArray) +# whose __new__ rejects the array_type kwarg, so it can't be used as a constructor. +# Subscripting is still fine in annotation position (Array[Any]). +Array = array.FSharpArray # Creation benchmarks @@ -97,7 +100,7 @@ def python_map(): def test_fsharp_map_benchmark(benchmark: BenchmarkFixture, size: int) -> None: """Benchmark map operation on FSharpArray.""" # Use Generic array type for regular Python lists - fs_array: Array[Any] = Array[Any](array_type="Generic", elements=list(range(size))) + fs_array: Array[Any] = Array(array_type="Generic", elements=list(range(size))) def fsharp_map(): return fs_array.map(lambda x: x * 2) @@ -377,3 +380,125 @@ def create_fs_array() -> Array[int32]: assert len(fs_arr) == 1000 assert fs_arr[0] == 0 assert fs_arr[1] == 1 + + +# Aggregate benchmarks (sum / average / min / max / contains) +# +# These aggregates take an adder/averager/comparer object and today loop in Rust +# calling a Python method (GetZero/Add/DivideByInt/Compare) per element even on +# unboxed numeric storage. They gate the Phase 1 native fast path: the same +# benchmark runs before/after, and the typed variants should improve sharply once +# the numeric fast path lands while Generic stays on the callback path. + + +class _Adder: + """Minimal IGenericAdder for numeric elements (elements are plain int/float).""" + + def __init__(self, zero: Any) -> None: + self._zero = zero + + def GetZero(self) -> Any: + return self._zero + + def Add(self, a: Any, b: Any) -> Any: + return a + b + + +class _Averager(_Adder): + """Minimal IGenericAverager: adds DivideByInt to the adder protocol.""" + + def DivideByInt(self, a: Any, n: int) -> Any: + return a / n + + +class _Comparer: + """Minimal IComparer using Python's built-in ordering.""" + + def Compare(self, a: Any, b: Any) -> int: + return (a > b) - (a < b) + + +@pytest.mark.parametrize(("array_type", "zero"), [("Int32", 0), ("Float64", 0.0), ("Generic", 0)]) +def test_array_sum_benchmark(benchmark: BenchmarkFixture, array_type: str, zero: Any) -> None: + """Benchmark sum over typed vs generic arrays.""" + size = 1000 + arr: Array[Any] = Array(array_type=array_type, elements=list(range(size))) # type: ignore + adder = _Adder(zero) + + benchmark.group = "Array sum (size=1000)" + benchmark.name = array_type + result = benchmark(lambda: arr.sum(adder)) + assert result == sum(range(size)) + + +@pytest.mark.parametrize(("array_type", "zero"), [("Int32", 0), ("Float64", 0.0), ("Generic", 0)]) +def test_array_average_benchmark(benchmark: BenchmarkFixture, array_type: str, zero: Any) -> None: + """Benchmark average over typed vs generic arrays.""" + size = 1000 + arr: Array[Any] = Array(array_type=array_type, elements=list(range(size))) # type: ignore + averager = _Averager(zero) + + benchmark.group = "Array average (size=1000)" + benchmark.name = array_type + result = benchmark(lambda: arr.average(averager)) + total = sum(range(size)) + # Integer element types truncate the average (int32(a / n)); floats/generic don't. + expected = total // size if array_type == "Int32" else total / size + # result may be a numeric wrapper (Int32/Float64); compare as plain floats. + assert float(result) == pytest.approx(float(expected)) + + +@pytest.mark.parametrize("array_type", ["Int32", "Float64", "Generic"]) +def test_array_max_benchmark(benchmark: BenchmarkFixture, array_type: str) -> None: + """Benchmark max over typed vs generic arrays.""" + size = 1000 + arr: Array[Any] = Array(array_type=array_type, elements=list(range(size))) # type: ignore + comparer = _Comparer() + + benchmark.group = "Array max (size=1000)" + benchmark.name = array_type + result = benchmark(lambda: arr.max(comparer)) + assert result == size - 1 + + +@pytest.mark.parametrize("array_type", ["Int32", "Float64", "Generic"]) +def test_array_min_benchmark(benchmark: BenchmarkFixture, array_type: str) -> None: + """Benchmark min over typed vs generic arrays.""" + size = 1000 + arr: Array[Any] = Array(array_type=array_type, elements=list(range(size))) # type: ignore + comparer = _Comparer() + + benchmark.group = "Array min (size=1000)" + benchmark.name = array_type + result = benchmark(lambda: arr.min(comparer)) + assert result == 0 + + +@pytest.mark.parametrize("array_type", ["Int32", "Float64", "Generic"]) +def test_array_contains_benchmark(benchmark: BenchmarkFixture, array_type: str) -> None: + """Benchmark contains (worst case: element absent) over typed vs generic arrays.""" + size = 1000 + arr: Array[Any] = Array(array_type=array_type, elements=list(range(size))) # type: ignore + # Absent value forces a full scan. + missing = size + 1 + + benchmark.group = "Array contains (size=1000, absent)" + benchmark.name = array_type + result = benchmark(lambda: arr.contains(missing, None)) + assert result is False + + +# Generic-vs-typed map (gates the Phase 2 Arc removal on generic storage) + + +@pytest.mark.parametrize("array_type", ["Int32", "Generic"]) +def test_array_map_typed_vs_generic_benchmark(benchmark: BenchmarkFixture, array_type: str) -> None: + """Benchmark map on typed vs generic storage (same callback).""" + size = 1000 + arr: Array[Any] = Array(array_type=array_type, elements=list(range(size))) # type: ignore + + benchmark.group = "Array map typed-vs-generic (size=1000)" + benchmark.name = array_type + result = benchmark(lambda: arr.map(lambda x: x + 1)) + assert len(result) == size + assert result[0] == 1 From d46cae1fd7274ad0fddcfbd9f394c27b5191266f Mon Sep 17 00:00:00 2001 From: Dag Brattli Date: Fri, 10 Jul 2026 12:34:32 +0200 Subject: [PATCH 2/5] perf(python): speed up array aggregates and generic storage in the Rust core MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three related optimizations to fable-library-core, each measured against the release-build benchmark baseline (1000-element arrays): Native numeric aggregates. sum/average/min/max/contains looped in Rust calling a Python adder/comparer method per element even on unboxed numeric storage. Add ArrayType-keyed fast paths that compute the result in a tight native loop and box only the final value, falling back to the callback path for Generic storage (and for float min/max, to preserve the comparer's NaN ordering). Semantics are matched exactly: integer sum wraps at the element width, average delegates the final DivideByInt so integer averages still truncate, and the sum result keeps its wrapper type. Speedups: sum up to 314x (Int32) / 80x (Float64), average 208x/75x, min/max 293x (Int32), contains ~30-40x. Remove Arc from generic storage. NativeArray::PyObject was Arc>>, taking an uncontended lock on every element access (the GIL already serializes access) and — because clone did Arc::clone — sharing mutable storage, so sort()/copy() mutated their source array. Switch to a plain Vec: clone now deep-copies the element references (correct F# value semantics) and per-element locking is gone. ~2x on all generic-array combinators (map/filter/ fold/iterate) plus the aliasing-bug fix. Hot-loop cleanups. filter/choose dropped a dead per-element clone_ref; the sort/projection-comparer paths intern the "Compare" method name (invoked O(n log n) times). filter is 2.19x over baseline overall. Adds hypothesis property tests asserting the native aggregate paths match the callback path (value, wrapper type, empty/NaN edge cases) and regression tests that sort()/copy() no longer mutate a generic source array. Full crate suite (227) and transpiled F#->Python suite (2391) pass. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/fable-library-py/src/array.rs | 63 +++-- src/fable-library-py/src/native_array.rs | 285 +++++++++++++++++------ src/fable-library-py/src/util.rs | 7 +- src/fable-library-py/tests/test_array.py | 166 +++++++++++++ 4 files changed, 424 insertions(+), 97 deletions(-) diff --git a/src/fable-library-py/src/array.rs b/src/fable-library-py/src/array.rs index 26d8b1f824..7b9198a22a 100644 --- a/src/fable-library-py/src/array.rs +++ b/src/fable-library-py/src/array.rs @@ -14,7 +14,6 @@ use pyo3::{ prelude::*, types::{PyAnyMethods, PyList}, }; -use std::sync::{Arc, Mutex}; #[pyclass(module = "fable", subclass, from_py_object)] #[derive(Clone, Debug)] @@ -237,8 +236,7 @@ impl FSharpArray { } // Fallback to PyObject storage if type extraction fails. - // This allows for generic or mixed-type arrays, at the cost of dynamic dispatch and locking. - // Arc> is used for thread safety and Python interop. + // This allows for generic or mixed-type arrays, at the cost of dynamic dispatch. let len = elements.len().unwrap_or(0); let mut vec = Vec::with_capacity(len); @@ -248,7 +246,7 @@ impl FSharpArray { })?; Ok(FSharpArray { - storage: NativeArray::PyObject(Arc::new(Mutex::new(vec))), + storage: NativeArray::PyObject(vec), }) } @@ -369,7 +367,7 @@ impl FSharpArray { } Ok(FSharpArray { - storage: NativeArray::PyObject(Arc::new(Mutex::new(vec))), + storage: NativeArray::PyObject(vec), }) } @@ -946,8 +944,7 @@ impl FSharpArray { NativeArray::Bool(vec) => { vec[idx as usize] = value.extract()?; } - NativeArray::PyObject(arc_mutex_vec) => { - let mut vec = arc_mutex_vec.lock().unwrap(); // Acquire the lock + NativeArray::PyObject(vec) => { vec[idx as usize] = value.clone().into(); } } @@ -1162,12 +1159,10 @@ impl FSharpArray { let mut results = NativeArray::new(&original_type, None); // No initial capacity needed for i in 0..len { - // Avoid cloning item_obj if possible, only clone for predicate call - let item_obj = self.get_item_at_index(i as isize, py)?; - let keep = predicate.call1((item_obj.clone_ref(py),))?.is_truthy()?; - - if keep { - // Push the original item (by index) into the results collector + // The predicate consumes the fetched item; the kept value is copied + // straight from storage by index, so no extra clone is needed. + let item = self.storage.get(py, i)?; + if predicate.call1((item,))?.is_truthy()? { results.push_from_storage(&self.storage, i, py); } } @@ -1274,12 +1269,12 @@ impl FSharpArray { if len == 0 { // Return an empty array return Ok(FSharpArray { - storage: NativeArray::PyObject(Arc::new(Mutex::new(vec![]))), + storage: NativeArray::PyObject(vec![]), }); } // Create an array of arrays (chunks) - let mut chunks = NativeArray::PyObject(Arc::new(Mutex::new(vec![]))); + let mut chunks = NativeArray::PyObject(vec![]); // Create each chunk for x in 0..len.div_ceil(chunk_size) { @@ -1651,6 +1646,11 @@ impl FSharpArray { } pub fn sum(&self, py: Python<'_>, adder: &Bound<'_, PyAny>) -> PyResult> { + // Fast path: numeric storage sums natively without per-element Python calls. + if let Some(result) = self.storage.try_native_sum(py) { + return result; + } + let len = self.storage.len(); let mut acc = adder.call_method0("GetZero")?; @@ -1670,6 +1670,14 @@ impl FSharpArray { )); } + // Fast path: sum numeric storage natively, then delegate the final + // DivideByInt to the averager so division/truncation semantics match exactly. + if let Some(sum_res) = self.storage.try_native_sum(py) { + let acc = sum_res?; + let result = averager.call_method1("DivideByInt", (acc, len))?; + return Ok(result.into()); + } + let mut acc = averager.call_method0("GetZero")?; for i in 0..len { @@ -1931,7 +1939,7 @@ impl FSharpArray { let len = self.storage.len(); if len == 0 { return Ok(FSharpArray { - storage: NativeArray::PyObject(Arc::new(Mutex::new(vec![]))), + storage: NativeArray::PyObject(vec![]), }); } @@ -2852,6 +2860,14 @@ impl FSharpArray { value: &Bound<'_, PyAny>, eq: Option<&Bound<'_, PyAny>>, ) -> PyResult { + // Fast path: with no custom equality comparer, numeric/bool storage scans + // natively (falls back when `value` isn't representable as the element type). + if eq.is_none() { + if let Some(result) = self.storage.try_native_contains(value) { + return result; + } + } + let len = self.storage.len(); for i in 0..len { let item = self.get_item_at_index(i as isize, py)?; @@ -2877,6 +2893,12 @@ impl FSharpArray { // let max (xs: 'a[]) ([] comparer: IComparer<'a>) : 'a = pub fn max(&self, py: Python<'_>, comparer: &Bound<'_, PyAny>) -> PyResult> { + // Fast path: integer storage compares natively (floats fall back to the + // comparer path to preserve its NaN ordering; empty falls back to the + // canonical empty-array error in reduce_impl). + if let Some(result) = self.storage.try_native_max(py) { + return result; + } reduce_impl(self, py, |acc, item, py| { let comparison = comparer.call_method1("Compare", (acc.clone_ref(py), item.clone_ref(py)))?; @@ -2886,6 +2908,10 @@ impl FSharpArray { } pub fn min(&self, py: Python<'_>, comparer: &Bound<'_, PyAny>) -> PyResult> { + // Fast path: integer storage compares natively (see `max`). + if let Some(result) = self.storage.try_native_min(py) { + return result; + } reduce_impl(self, py, |acc, item, py| { let comparison = comparer.call_method1("Compare", (acc.clone_ref(py), item.clone_ref(py)))?; @@ -2947,7 +2973,7 @@ impl FSharpArray { let mut results = NativeArray::new(&target_type, None); for i in 0..len { let item = self.get_item_at_index(i as isize, py)?; - let chosen = chooser.call1((item.clone_ref(py),))?; + let chosen = chooser.call1((item,))?; if !chosen.is_none() { results.push_value(&chosen, py)?; } @@ -3282,8 +3308,7 @@ impl FSharpArray { NativeArray::Float32(vec) => vec.swap(i, j), NativeArray::Float64(vec) => vec.swap(i, j), NativeArray::Bool(vec) => vec.swap(i, j), - NativeArray::PyObject(arc) => { - let mut vec = arc.lock().unwrap(); + NativeArray::PyObject(vec) => { vec.swap(i, j); } } diff --git a/src/fable-library-py/src/native_array.rs b/src/fable-library-py/src/native_array.rs index 4039cf60f4..af20e96e8a 100644 --- a/src/fable-library-py/src/native_array.rs +++ b/src/fable-library-py/src/native_array.rs @@ -1,12 +1,12 @@ use crate::floats::{Float32, Float64}; use crate::ints::{Int16, Int32, Int64, Int8, UInt16, UInt32, UInt64, UInt8}; use pyo3::class::basic::CompareOp; +use pyo3::intern; use pyo3::prelude::*; use pyo3::types::PyAnyMethods; use pyo3::IntoPyObjectExt; use std::cell::RefCell; use std::cmp::Ordering; -use std::sync::{Arc, Mutex}; #[derive(Clone, Debug, PartialEq)] pub enum ArrayType { @@ -127,7 +127,12 @@ pub enum NativeArray { Float32(Vec), Float64(Vec), Bool(Vec), - PyObject(Arc>>>), + // Generic storage. A plain Vec is sufficient: the GIL serializes all access + // (every method that touches storage holds a `Python` token / uses `&mut self` + // through PyO3's borrow model), so no Mutex is needed. Cloning deep-copies the + // element references, giving correct F# value semantics (a clone never aliases + // the source's mutable storage). + PyObject(Vec>), } impl Clone for NativeArray { @@ -144,9 +149,12 @@ impl Clone for NativeArray { NativeArray::Float32(vec) => NativeArray::Float32(vec.clone()), NativeArray::Float64(vec) => NativeArray::Float64(vec.clone()), NativeArray::Bool(vec) => NativeArray::Bool(vec.clone()), - NativeArray::PyObject(arc) => { - // Clone the Arc, which increments the reference count - NativeArray::PyObject(Arc::clone(arc)) + NativeArray::PyObject(vec) => { + // Deep-copy: increment the refcount of each element so the clone + // owns independent references (value semantics, no aliasing). + Python::attach(|py| { + NativeArray::PyObject(vec.iter().map(|o| o.clone_ref(py)).collect()) + }) } } } @@ -240,7 +248,7 @@ impl NativeArray { NativeArray::Float32(vec) => vec.len(), NativeArray::Float64(vec) => vec.len(), NativeArray::Bool(vec) => vec.len(), - NativeArray::PyObject(vec) => vec.lock().unwrap().len(), + NativeArray::PyObject(vec) => vec.len(), } } @@ -252,7 +260,7 @@ impl NativeArray { pub fn equals(&self, other: &NativeArray, py: Python<'_>) -> bool { // Helper for comparing PyObject with other types fn compare_pyobject_with pyo3::FromPyObject<'a, 'py>>( - py_vec: &std::sync::MutexGuard>>, + py_vec: &[Py], other_vec: &[T], py: Python<'_>, ) -> bool { @@ -277,8 +285,8 @@ impl NativeArray { (NativeArray::Float64(vec), NativeArray::Float64(other_vec)) => vec == other_vec, (NativeArray::Bool(vec), NativeArray::Bool(other_vec)) => vec == other_vec, (NativeArray::PyObject(vec), NativeArray::PyObject(other_vec)) => { - let xs = vec.lock().unwrap(); - let ys = other_vec.lock().unwrap(); + let xs = vec; + let ys = other_vec; if xs.len() != ys.len() { return false; } @@ -290,36 +298,36 @@ impl NativeArray { }) } (NativeArray::PyObject(vec), other) => { - let xs = vec.lock().unwrap(); + let xs = vec; match other { - NativeArray::Int8(vec) => compare_pyobject_with(&xs, vec, py), - NativeArray::UInt8(vec) => compare_pyobject_with(&xs, vec, py), - NativeArray::Int16(vec) => compare_pyobject_with(&xs, vec, py), - NativeArray::UInt16(vec) => compare_pyobject_with(&xs, vec, py), - NativeArray::Int32(vec) => compare_pyobject_with(&xs, vec, py), - NativeArray::UInt32(vec) => compare_pyobject_with(&xs, vec, py), - NativeArray::Int64(vec) => compare_pyobject_with(&xs, vec, py), - NativeArray::UInt64(vec) => compare_pyobject_with(&xs, vec, py), - NativeArray::Float32(vec) => compare_pyobject_with(&xs, vec, py), - NativeArray::Float64(vec) => compare_pyobject_with(&xs, vec, py), - NativeArray::Bool(vec) => compare_pyobject_with(&xs, vec, py), + NativeArray::Int8(vec) => compare_pyobject_with(xs, vec, py), + NativeArray::UInt8(vec) => compare_pyobject_with(xs, vec, py), + NativeArray::Int16(vec) => compare_pyobject_with(xs, vec, py), + NativeArray::UInt16(vec) => compare_pyobject_with(xs, vec, py), + NativeArray::Int32(vec) => compare_pyobject_with(xs, vec, py), + NativeArray::UInt32(vec) => compare_pyobject_with(xs, vec, py), + NativeArray::Int64(vec) => compare_pyobject_with(xs, vec, py), + NativeArray::UInt64(vec) => compare_pyobject_with(xs, vec, py), + NativeArray::Float32(vec) => compare_pyobject_with(xs, vec, py), + NativeArray::Float64(vec) => compare_pyobject_with(xs, vec, py), + NativeArray::Bool(vec) => compare_pyobject_with(xs, vec, py), _ => false, } } (other, NativeArray::PyObject(vec)) => { - let ys = vec.lock().unwrap(); + let ys = vec; match other { - NativeArray::Int8(vec) => compare_pyobject_with(&ys, vec, py), - NativeArray::UInt8(vec) => compare_pyobject_with(&ys, vec, py), - NativeArray::Int16(vec) => compare_pyobject_with(&ys, vec, py), - NativeArray::UInt16(vec) => compare_pyobject_with(&ys, vec, py), - NativeArray::Int32(vec) => compare_pyobject_with(&ys, vec, py), - NativeArray::UInt32(vec) => compare_pyobject_with(&ys, vec, py), - NativeArray::Int64(vec) => compare_pyobject_with(&ys, vec, py), - NativeArray::UInt64(vec) => compare_pyobject_with(&ys, vec, py), - NativeArray::Float32(vec) => compare_pyobject_with(&ys, vec, py), - NativeArray::Float64(vec) => compare_pyobject_with(&ys, vec, py), - NativeArray::Bool(vec) => compare_pyobject_with(&ys, vec, py), + NativeArray::Int8(vec) => compare_pyobject_with(ys, vec, py), + NativeArray::UInt8(vec) => compare_pyobject_with(ys, vec, py), + NativeArray::Int16(vec) => compare_pyobject_with(ys, vec, py), + NativeArray::UInt16(vec) => compare_pyobject_with(ys, vec, py), + NativeArray::Int32(vec) => compare_pyobject_with(ys, vec, py), + NativeArray::UInt32(vec) => compare_pyobject_with(ys, vec, py), + NativeArray::Int64(vec) => compare_pyobject_with(ys, vec, py), + NativeArray::UInt64(vec) => compare_pyobject_with(ys, vec, py), + NativeArray::Float32(vec) => compare_pyobject_with(ys, vec, py), + NativeArray::Float64(vec) => compare_pyobject_with(ys, vec, py), + NativeArray::Bool(vec) => compare_pyobject_with(ys, vec, py), _ => false, } } @@ -403,8 +411,6 @@ impl NativeArray { Ok(()) } (NativeArray::PyObject(src), NativeArray::PyObject(dst)) => { - let src = src.lock().unwrap(); - let mut dst = dst.lock().unwrap(); for i in 0..count { dst[target_index + i] = src[source_index + i].clone_ref(py); } @@ -436,9 +442,9 @@ impl NativeArray { ArrayType::Float32 => NativeArray::Float32(Vec::with_capacity(capacity.unwrap_or(0))), ArrayType::Float64 => NativeArray::Float64(Vec::with_capacity(capacity.unwrap_or(0))), ArrayType::Bool => NativeArray::Bool(Vec::with_capacity(capacity.unwrap_or(0))), - ArrayType::Generic => NativeArray::PyObject(Arc::new(Mutex::new(Vec::with_capacity( - capacity.unwrap_or(0), - )))), + ArrayType::Generic => { + NativeArray::PyObject(Vec::with_capacity(capacity.unwrap_or(0))) + } } } @@ -496,7 +502,7 @@ impl NativeArray { ArrayType::Float32 => NativeArray::Float32(Vec::new()), ArrayType::Float64 => NativeArray::Float64(Vec::new()), ArrayType::Bool => NativeArray::Bool(Vec::new()), - ArrayType::Generic => NativeArray::PyObject(Arc::new(Mutex::new(Vec::new()))), + ArrayType::Generic => NativeArray::PyObject(Vec::new()), } } @@ -528,9 +534,7 @@ impl NativeArray { (NativeArray::Float64(dst), NativeArray::Float64(src)) => dst.push(src[index]), (NativeArray::Bool(dst), NativeArray::Bool(src)) => dst.push(src[index]), (NativeArray::PyObject(dst), NativeArray::PyObject(src)) => { - let src_guard = src.lock().unwrap(); - let mut dst_guard = dst.lock().unwrap(); - dst_guard.push(src_guard[index].clone_ref(py)); + dst.push(src[index].clone_ref(py)); } _ => panic!("Cannot push between different array types"), } @@ -556,8 +560,7 @@ impl NativeArray { NativeArray::Float64(vec) => push_typed_value!(vec, value), NativeArray::Bool(vec) => push_typed_value!(vec, value), NativeArray::PyObject(vec) => { - let mut guard = vec.lock().unwrap(); - guard.push(value.clone().unbind()); + vec.push(value.clone().unbind()); } } Ok(()) @@ -597,10 +600,9 @@ impl NativeArray { } } NativeArray::PyObject(vec) => { - let mut guard = vec.lock().unwrap(); let py_value = value.into_py_any(value.py())?; for i in 0..count { - guard[target_index + i] = py_value.clone_ref(value.py()); + vec[target_index + i] = py_value.clone_ref(value.py()); } } } @@ -623,15 +625,12 @@ impl NativeArray { NativeArray::Float32(vec) => NativeArray::Float32(reverse_vec!(vec)), NativeArray::Float64(vec) => NativeArray::Float64(reverse_vec!(vec)), NativeArray::Bool(vec) => NativeArray::Bool(reverse_vec!(vec)), - NativeArray::PyObject(arc) => { - let mut new_vec = Vec::with_capacity(arc.lock().unwrap().len()); - { - let guard = arc.lock().unwrap(); - for i in (0..guard.len()).rev() { - new_vec.push(guard[i].clone_ref(py)); - } + NativeArray::PyObject(vec) => { + let mut new_vec = Vec::with_capacity(vec.len()); + for i in (0..vec.len()).rev() { + new_vec.push(vec[i].clone_ref(py)); } - NativeArray::PyObject(Arc::new(Mutex::new(new_vec))) + NativeArray::PyObject(new_vec) } } } @@ -653,8 +652,8 @@ impl NativeArray { NativeArray::Float32(vec) => simple_vec_operation!(vec, remove, index), NativeArray::Float64(vec) => simple_vec_operation!(vec, remove, index), NativeArray::Bool(vec) => simple_vec_operation!(vec, remove, index), - NativeArray::PyObject(arc_vec) => { - arc_vec.lock().unwrap().remove(index); + NativeArray::PyObject(vec) => { + vec.remove(index); } } } @@ -692,8 +691,7 @@ impl NativeArray { let bool_value: bool = value.extract()?; vec.insert(index, bool_value); } - NativeArray::PyObject(arc_mutex_vec) => { - let mut vec = arc_mutex_vec.lock().unwrap(); + NativeArray::PyObject(vec) => { vec.insert(index, value.clone().into()); } } @@ -717,9 +715,7 @@ impl NativeArray { NativeArray::Float64(vec) => simple_vec_operation!(vec, truncate, new_size), NativeArray::Bool(vec) => simple_vec_operation!(vec, truncate, new_size), NativeArray::PyObject(vec) => { - if let Ok(mut vec) = vec.lock() { - vec.truncate(new_size); - } + vec.truncate(new_size); } } } @@ -827,8 +823,7 @@ impl NativeArray { } NativeArray::PyObject(vec) => { let error: RefCell> = RefCell::new(None); - let mut guard = vec.lock().unwrap(); - guard.sort_by(|a, b| { + vec.sort_by(|a, b| { if error.borrow().is_some() { return Ordering::Equal; } @@ -899,7 +894,7 @@ impl NativeArray { let proj_a = projection.call1((py_a,))?; let proj_b = projection.call1((py_b,))?; // Call the Compare method on the IComparer object - let result = comparer.call_method1("Compare", (proj_a, proj_b))?; + let result = comparer.call_method1(intern!(py, "Compare"), (proj_a, proj_b))?; Ok(result.extract::()?.cmp(&0)) } @@ -941,12 +936,7 @@ impl NativeArray { NativeArray::Bool(vec) => sort_projection_with_error!(vec, Bool), NativeArray::PyObject(vec) => { let error: RefCell> = RefCell::new(None); - let mut new_vec = vec - .lock() - .unwrap() - .iter() - .map(|x| x.clone_ref(py)) - .collect::>(); + let mut new_vec = vec.iter().map(|x| x.clone_ref(py)).collect::>(); new_vec.sort_by(|a, b| { if error.borrow().is_some() { return Ordering::Equal; @@ -956,7 +946,7 @@ impl NativeArray { let py_b = b.bind(py); let proj_a = projection.call1((py_a,))?; let proj_b = projection.call1((py_b,))?; - let cmp_result = comparer.call_method1("Compare", (proj_a, proj_b))?; + let cmp_result = comparer.call_method1(intern!(py, "Compare"), (proj_a, proj_b))?; Ok(cmp_result.extract::()?.cmp(&0)) })(); match result { @@ -970,7 +960,7 @@ impl NativeArray { if let Some(e) = error.into_inner() { return Err(e); } - NativeArray::PyObject(Arc::new(Mutex::new(new_vec))) + NativeArray::PyObject(new_vec) } }; Ok(result) @@ -994,10 +984,153 @@ impl NativeArray { NativeArray::Float32(vec) => Ok(vec[index].into_py_any(py)?), NativeArray::Float64(vec) => Ok(vec[index].into_py_any(py)?), NativeArray::Bool(vec) => Ok(vec[index].into_py_any(py)?), - NativeArray::PyObject(vec) => { - let guard = vec.lock().unwrap(); - Ok(guard[index].clone_ref(py)) - } + NativeArray::PyObject(vec) => Ok(vec[index].clone_ref(py)), + } + } + + /// Native `sum` fast path for numeric storage. + /// + /// Returns `Some(result)` for the numeric variants, computing the sum in a + /// tight unboxed loop and boxing only the final value; returns `None` for + /// `Bool`/`Generic` storage so the caller falls back to the Python-callback + /// path. + /// + /// Semantics match the Python `IGenericAdder` path: integer types use + /// **wrapping** arithmetic at the element width and the result is boxed as the + /// corresponding wrapper type (`Int32`, …) — the same type `GetZero`/`Add` + /// would produce — while floats use IEEE addition. + pub fn try_native_sum(&self, py: Python<'_>) -> Option>> { + macro_rules! sum_int { + ($vec:expr, $w:ident, $t:ty) => {{ + let mut acc: $t = 0; + for &x in $vec.iter() { + acc = acc.wrapping_add(x); + } + Some($w(acc).into_py_any(py)) + }}; + } + macro_rules! sum_float { + ($vec:expr, $w:ident, $t:ty) => {{ + let mut acc: $t = 0.0; + for &x in $vec.iter() { + acc += x; + } + Some($w(acc).into_py_any(py)) + }}; + } + match self { + NativeArray::Int8(v) => sum_int!(v, Int8, i8), + NativeArray::UInt8(v) => sum_int!(v, UInt8, u8), + NativeArray::Int16(v) => sum_int!(v, Int16, i16), + NativeArray::UInt16(v) => sum_int!(v, UInt16, u16), + NativeArray::Int32(v) => sum_int!(v, Int32, i32), + NativeArray::UInt32(v) => sum_int!(v, UInt32, u32), + NativeArray::Int64(v) => sum_int!(v, Int64, i64), + NativeArray::UInt64(v) => sum_int!(v, UInt64, u64), + NativeArray::Float32(v) => sum_float!(v, Float32, f32), + NativeArray::Float64(v) => sum_float!(v, Float64, f64), + NativeArray::Bool(_) | NativeArray::PyObject(_) => None, + } + } + + /// Native `max` fast path for **integer** storage. + /// + /// Returns `Some(result)` for non-empty integer variants, boxing the largest + /// element as a plain Python int (matching `get`, i.e. the value the existing + /// `reduce_impl`/comparer path returns). Returns `None` for empty arrays (so + /// the caller's `reduce_impl` produces the canonical empty-array error) and for + /// float/bool/generic storage (floats fall back to the comparer path to + /// preserve its NaN ordering semantics). + pub fn try_native_max(&self, py: Python<'_>) -> Option>> { + macro_rules! max_int { + ($vec:expr) => {{ + if $vec.is_empty() { + None + } else { + let mut acc = $vec[0]; + for &x in &$vec[1..] { + if x > acc { + acc = x; + } + } + Some(acc.into_py_any(py)) + } + }}; + } + match self { + NativeArray::Int8(v) => max_int!(v), + NativeArray::UInt8(v) => max_int!(v), + NativeArray::Int16(v) => max_int!(v), + NativeArray::UInt16(v) => max_int!(v), + NativeArray::Int32(v) => max_int!(v), + NativeArray::UInt32(v) => max_int!(v), + NativeArray::Int64(v) => max_int!(v), + NativeArray::UInt64(v) => max_int!(v), + _ => None, + } + } + + /// Native `min` fast path for **integer** storage. See [`try_native_max`]. + pub fn try_native_min(&self, py: Python<'_>) -> Option>> { + macro_rules! min_int { + ($vec:expr) => {{ + if $vec.is_empty() { + None + } else { + let mut acc = $vec[0]; + for &x in &$vec[1..] { + if x < acc { + acc = x; + } + } + Some(acc.into_py_any(py)) + } + }}; + } + match self { + NativeArray::Int8(v) => min_int!(v), + NativeArray::UInt8(v) => min_int!(v), + NativeArray::Int16(v) => min_int!(v), + NativeArray::UInt16(v) => min_int!(v), + NativeArray::Int32(v) => min_int!(v), + NativeArray::UInt32(v) => min_int!(v), + NativeArray::Int64(v) => min_int!(v), + NativeArray::UInt64(v) => min_int!(v), + _ => None, + } + } + + /// Native `contains` fast path for numeric/bool storage. + /// + /// Only used when no custom equality comparer is supplied. Extracts `value` to + /// the element type and scans natively; if `value` cannot be represented as the + /// element type, returns `None` so the caller falls back to the general + /// `rich_compare` path (which preserves Python cross-type equality semantics). + pub fn try_native_contains( + &self, + value: &Bound<'_, PyAny>, + ) -> Option> { + macro_rules! contains_prim { + ($vec:expr, $t:ty) => {{ + match value.extract::<$t>() { + Ok(v) => Some(Ok($vec.iter().any(|&x| x == v))), + Err(_) => None, + } + }}; + } + match self { + NativeArray::Int8(v) => contains_prim!(v, i8), + NativeArray::UInt8(v) => contains_prim!(v, u8), + NativeArray::Int16(v) => contains_prim!(v, i16), + NativeArray::UInt16(v) => contains_prim!(v, u16), + NativeArray::Int32(v) => contains_prim!(v, i32), + NativeArray::UInt32(v) => contains_prim!(v, u32), + NativeArray::Int64(v) => contains_prim!(v, i64), + NativeArray::UInt64(v) => contains_prim!(v, u64), + NativeArray::Float32(v) => contains_prim!(v, f32), + NativeArray::Float64(v) => contains_prim!(v, f64), + NativeArray::Bool(v) => contains_prim!(v, bool), + NativeArray::PyObject(_) => None, } } } diff --git a/src/fable-library-py/src/util.rs b/src/fable-library-py/src/util.rs index 3992ef47db..0f782944dd 100644 --- a/src/fable-library-py/src/util.rs +++ b/src/fable-library-py/src/util.rs @@ -1,4 +1,5 @@ use pyo3::class::basic::CompareOp; +use pyo3::intern; use pyo3::{prelude::*, types::PyAnyMethods}; /// Helper class for sort_in_place_by that applies projection before comparison @@ -32,11 +33,13 @@ impl ProjectionComparer { let projected_x = self.projection.bind(py).call1((x,))?; let projected_y = self.projection.bind(py).call1((y,))?; - // Compare the projected values using the comparer + // Compare the projected values using the comparer. `__call__` is invoked + // O(n log n) times during a sort, so intern the method name to avoid + // recreating the "Compare" string on every comparison. let result = self .comparer .bind(py) - .call_method1("Compare", (projected_x, projected_y))?; + .call_method1(intern!(py, "Compare"), (projected_x, projected_y))?; result.extract() } } diff --git a/src/fable-library-py/tests/test_array.py b/src/fable-library-py/tests/test_array.py index 4daefdaf9f..09268ccd48 100644 --- a/src/fable-library-py/tests/test_array.py +++ b/src/fable-library-py/tests/test_array.py @@ -1439,3 +1439,169 @@ class Data(BaseModel): restored = Data.model_validate_json(json_str) assert list(restored.values) == [10, 20, 30] assert type(restored.values).__name__ == "FSharpArray" + + +# --------------------------------------------------------------------------- +# Native numeric aggregate fast paths (sum / average / min / max / contains) +# +# These assert the native typed-storage fast path is equivalent to the +# Python-callback path, which is exercised by building a Generic array from the +# SAME wrapper objects: the callback path's wrapper arithmetic wraps identically, +# so it is a faithful oracle for the native path. +# --------------------------------------------------------------------------- + +# Numeric element strategies keyed by array type (reuse the module-level ones). +_NUMERIC_ARRAY_TYPES = ["Int8", "UInt8", "Int16", "UInt16", "Int32", "UInt32", "Int64", "UInt64", "Float32", "Float64"] +_INT_ARRAY_TYPES = ["Int8", "UInt8", "Int16", "UInt16", "Int32", "UInt32", "Int64", "UInt64"] + +_ZERO = { + "Int8": sbyte(0), + "UInt8": byte(0), + "Int16": int16(0), + "UInt16": uint16(0), + "Int32": int32(0), + "UInt32": uint32(0), + "Int64": int64(0), + "UInt64": uint64(0), + "Float32": float32(0.0), + "Float64": float64(0.0), +} + + +class _Adder: + """Minimal IGenericAdder; GetZero returns a typed wrapper so callback-path + arithmetic wraps at the element width (matching the native path).""" + + def __init__(self, zero: Any) -> None: + self._zero = zero + + def GetZero(self) -> Any: + return self._zero + + def Add(self, a: Any, b: Any) -> Any: + return a + b + + +class _Averager(_Adder): + def DivideByInt(self, a: Any, n: int) -> Any: + return a / n + + +class _Comparer: + def Compare(self, a: Any, b: Any) -> int: + return (a > b) - (a < b) + + +def _typed_and_generic(array_type: str, elements: list[Any]) -> tuple[Any, Any]: + typed = Array(array_type=array_type, elements=list(elements)) + generic = Array(array_type="Generic", elements=list(elements)) + return typed, generic + + +def _agg_eq(a: Any, b: Any) -> bool: + """Equality that treats two NaNs as equal (float inf + -inf sums produce NaN).""" + import math + + fa, fb = float(a), float(b) + if math.isnan(fa) and math.isnan(fb): + return True + return bool(a == b) + + +@pytest.mark.parametrize("array_type", _NUMERIC_ARRAY_TYPES) +@given(data=st.data()) +def test_native_sum_matches_callback(array_type: str, data: st.DataObject) -> None: + """Native sum on typed storage equals the callback path (value and type).""" + elements = data.draw(st.lists(array_types[array_type], min_size=0, max_size=64)) + typed, generic = _typed_and_generic(array_type, elements) + adder = _Adder(_ZERO[array_type]) + typed_sum = typed.sum(adder) + generic_sum = generic.sum(adder) + assert _agg_eq(typed_sum, generic_sum) + # Result wrapper type is preserved (Int32 sum -> Int32, etc.) + assert type(typed_sum).__name__ == array_type + + +@pytest.mark.parametrize("array_type", _NUMERIC_ARRAY_TYPES) +@given(data=st.data()) +def test_native_average_matches_callback(array_type: str, data: st.DataObject) -> None: + """Native average on typed storage equals the callback path.""" + elements = data.draw(st.lists(array_types[array_type], min_size=1, max_size=64)) + typed, generic = _typed_and_generic(array_type, elements) + averager = _Averager(_ZERO[array_type]) + assert _agg_eq(typed.average(averager), generic.average(averager)) + + +@pytest.mark.parametrize("array_type", _NUMERIC_ARRAY_TYPES) +def test_native_average_empty_raises(array_type: str) -> None: + """Average of an empty typed array raises, like the callback path.""" + empty = Array(array_type=array_type, elements=[]) + with pytest.raises(Exception): + empty.average(_Averager(_ZERO[array_type])) + + +@pytest.mark.parametrize("array_type", _INT_ARRAY_TYPES) +@given(data=st.data()) +def test_native_min_max_matches_callback(array_type: str, data: st.DataObject) -> None: + """Native min/max on integer storage equals the callback path (by value).""" + elements = data.draw(st.lists(array_types[array_type], min_size=1, max_size=64)) + typed, generic = _typed_and_generic(array_type, elements) + comparer = _Comparer() + assert typed.max(comparer) == generic.max(comparer) + assert typed.min(comparer) == generic.min(comparer) + + +@pytest.mark.parametrize("array_type", _INT_ARRAY_TYPES) +def test_native_min_max_empty_raises(array_type: str) -> None: + """Min/max of an empty typed array raises, like the callback path.""" + empty = Array(array_type=array_type, elements=[]) + with pytest.raises(Exception): + empty.max(_Comparer()) + with pytest.raises(Exception): + empty.min(_Comparer()) + + +@pytest.mark.parametrize("array_type", _NUMERIC_ARRAY_TYPES) +@given(data=st.data()) +def test_native_contains_matches_callback(array_type: str, data: st.DataObject) -> None: + """Native contains on typed storage equals the callback path.""" + elements = data.draw(st.lists(array_types[array_type], min_size=0, max_size=64)) + typed, generic = _typed_and_generic(array_type, elements) + # Draw a probe value of the same element type (present or absent). + value = data.draw(array_types[array_type]) + assert typed.contains(value, None) == generic.contains(value, None) + + +# --------------------------------------------------------------------------- +# Value semantics for generic (PyObject) storage. +# +# Generic storage is a plain Vec: cloning deep-copies element references, so a +# copy/sort never aliases the source's storage. These guard against the previous +# Arc sharing where sort()/copy() mutated the source in place. +# --------------------------------------------------------------------------- + + +def test_generic_copy_is_independent() -> None: + arr = Array(array_type="Generic", elements=[3, 1, 2]) + copied = arr.copy() + copied[0] = 99 + assert list(arr) == [3, 1, 2], "mutating the copy must not affect the source" + assert list(copied) == [99, 1, 2] + + +def test_generic_sort_does_not_mutate_source() -> None: + arr = Array(array_type="Generic", elements=[3, 1, 2]) + result = arr.sort(_Comparer()) + assert list(arr) == [3, 1, 2], "sort() must not mutate the source array" + assert list(result) == [1, 2, 3] + + +@given(elements=st.lists(st.integers(), min_size=0, max_size=64)) +def test_generic_copy_round_trip(elements: list[int]) -> None: + arr = Array(array_type="Generic", elements=list(elements)) + copied = arr.copy() + assert list(copied) == list(elements) + # Independent: appending/replacing in the copy leaves the source untouched. + if elements: + copied[0] = "changed" + assert list(arr) == list(elements) From 64c30a2c51d4819a7dee83a9082f495abf7a29c0 Mon Sep 17 00:00:00 2001 From: Dag Brattli Date: Fri, 10 Jul 2026 15:36:28 +0200 Subject: [PATCH 3/5] chore(python): document fast-path invariants and cover cross-type contains Address review follow-ups on the fable-library-core perf work: - Reword the generic (PyObject) storage comment: soundness rests on PyO3's pyclass borrow model (&mut self => runtime-enforced exclusive access), not the GIL, so it holds on free-threaded/no-GIL builds too. - Document the load-bearing invariant at the numeric sum/max fast paths: bypassing the injected adder/comparer is safe only because Fable routes custom monoids/key-comparers through sum_by/max_by, not sum/max. - Add test_native_contains_cross_type_fallback covering the extraction-failure fallback (int probed with float/bool, float with int, bool with int), asserting native == callback oracle == Python. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/fable-library-py/src/array.rs | 8 ++++++- src/fable-library-py/src/native_array.rs | 8 ++++--- src/fable-library-py/tests/test_array.py | 30 ++++++++++++++++++++++++ 3 files changed, 42 insertions(+), 4 deletions(-) diff --git a/src/fable-library-py/src/array.rs b/src/fable-library-py/src/array.rs index 7b9198a22a..75e08ee860 100644 --- a/src/fable-library-py/src/array.rs +++ b/src/fable-library-py/src/array.rs @@ -1647,6 +1647,10 @@ impl FSharpArray { pub fn sum(&self, py: Python<'_>, adder: &Bound<'_, PyAny>) -> PyResult> { // Fast path: numeric storage sums natively without per-element Python calls. + // This bypasses `adder` entirely, which is safe because Fable only ever + // injects the standard numeric adder for primitive-typed arrays (custom + // monoids reach the runtime through `sum_by`, not `sum`). If that invariant + // ever changes, this fast path must be gated accordingly. if let Some(result) = self.storage.try_native_sum(py) { return result; } @@ -2895,7 +2899,9 @@ impl FSharpArray { pub fn max(&self, py: Python<'_>, comparer: &Bound<'_, PyAny>) -> PyResult> { // Fast path: integer storage compares natively (floats fall back to the // comparer path to preserve its NaN ordering; empty falls back to the - // canonical empty-array error in reduce_impl). + // canonical empty-array error in reduce_impl). Bypassing `comparer` is safe + // because Fable always injects the standard comparer here; a custom key + // comparer reaches the runtime through `max_by`, which is not fast-pathed. if let Some(result) = self.storage.try_native_max(py) { return result; } diff --git a/src/fable-library-py/src/native_array.rs b/src/fable-library-py/src/native_array.rs index af20e96e8a..eb4440ffc7 100644 --- a/src/fable-library-py/src/native_array.rs +++ b/src/fable-library-py/src/native_array.rs @@ -127,9 +127,11 @@ pub enum NativeArray { Float32(Vec), Float64(Vec), Bool(Vec), - // Generic storage. A plain Vec is sufficient: the GIL serializes all access - // (every method that touches storage holds a `Python` token / uses `&mut self` - // through PyO3's borrow model), so no Mutex is needed. Cloning deep-copies the + // Generic storage. A plain Vec is sufficient — no Mutex needed. The soundness + // comes from PyO3's pyclass borrow model, not the GIL: mutating methods take + // `&mut self`, so PyO3 enforces exclusive access at runtime (a conflicting + // borrow raises `AlreadyBorrowed`). That holds even on free-threaded/no-GIL + // builds, where the GIL no longer serializes anything. Cloning deep-copies the // element references, giving correct F# value semantics (a clone never aliases // the source's mutable storage). PyObject(Vec>), diff --git a/src/fable-library-py/tests/test_array.py b/src/fable-library-py/tests/test_array.py index 09268ccd48..8278466ed9 100644 --- a/src/fable-library-py/tests/test_array.py +++ b/src/fable-library-py/tests/test_array.py @@ -1572,6 +1572,36 @@ def test_native_contains_matches_callback(array_type: str, data: st.DataObject) assert typed.contains(value, None) == generic.contains(value, None) +# Cross-type probes: the probe value is not representable as the element type, so +# the native fast path bails out (`try_native_contains` returns None) and the +# scan falls back to `rich_compare`, preserving Python's cross-type equality +# (2 == 2.0, True == 1). The typed (native) path must agree with the generic +# (callback) oracle in every case. +@pytest.mark.parametrize( + ("array_type", "elements", "probe", "expected"), + [ + # int storage probed with a float that equals / doesn't equal an element + ("Int32", [1, 2, 3], 2.0, True), + ("Int32", [1, 2, 3], 2.5, False), + # int storage probed with a bool (True == 1 in Python) + ("Int32", [0, 1], True, True), + # float storage probed with an int + ("Float64", [1.0, 2.0], 2, True), + ("Float64", [1.0, 2.0], 3, False), + # bool storage probed with an int (extract:: rejects a plain int) + ("Bool", [True, False], 1, True), + ("Bool", [True, False], 0, True), + ("Bool", [True], 2, False), + ], +) +def test_native_contains_cross_type_fallback(array_type: str, elements: list[Any], probe: Any, expected: bool) -> None: + """Cross-type probes fall back to rich_compare; native == callback == Python.""" + typed = Array(array_type=array_type, elements=list(elements)) + generic = Array(array_type="Generic", elements=list(elements)) + assert typed.contains(probe, None) == expected + assert typed.contains(probe, None) == generic.contains(probe, None) + + # --------------------------------------------------------------------------- # Value semantics for generic (PyObject) storage. # From 84a07a81a236583bf8078b19f8a6de217b8a7c53 Mon Sep 17 00:00:00 2001 From: Dag Brattli Date: Fri, 10 Jul 2026 16:28:37 +0200 Subject: [PATCH 4/5] docs(python): clarify why Mutex removal is sound on generic storage Expand the PyObject storage comment to record the coupled Arc<->Mutex reasoning: the Mutex was only load-bearing while the Arc allowed two distinct FSharpArray instances to alias one Vec (PyO3's borrow flag is per-instance and cannot see that aliasing). Removing the aliasing gives each Vec a single owner, so PyO3's atomic borrow check enforces exclusive access on both GIL and free-threaded builds. Note it does not make concurrent mutation thread-safe -- like .NET arrays, it never was; the Mutex only provided memory safety, which the borrow checker now provides. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/fable-library-py/src/native_array.rs | 30 ++++++++++++++++++------ 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/src/fable-library-py/src/native_array.rs b/src/fable-library-py/src/native_array.rs index eb4440ffc7..0fcc7bd8b4 100644 --- a/src/fable-library-py/src/native_array.rs +++ b/src/fable-library-py/src/native_array.rs @@ -127,13 +127,29 @@ pub enum NativeArray { Float32(Vec), Float64(Vec), Bool(Vec), - // Generic storage. A plain Vec is sufficient — no Mutex needed. The soundness - // comes from PyO3's pyclass borrow model, not the GIL: mutating methods take - // `&mut self`, so PyO3 enforces exclusive access at runtime (a conflicting - // borrow raises `AlreadyBorrowed`). That holds even on free-threaded/no-GIL - // builds, where the GIL no longer serializes anything. Cloning deep-copies the - // element references, giving correct F# value semantics (a clone never aliases - // the source's mutable storage). + // Generic storage. A plain Vec is sufficient — no Mutex needed — but only + // *because* we also dropped the `Arc`: the two changes are coupled. + // + // Every mutation of this Vec goes through a `&mut` receiver (with a plain Vec + // it can't be otherwise — there is no interior mutability). PyO3 atomically + // borrow-checks each pyclass method at entry (`try_borrow_mut`, backed by an + // AtomicUsize CAS), so two threads can never hold conflicting borrows of the + // same FSharpArray; a racing conflict raises `AlreadyBorrowed` rather than + // tearing the Vec. This holds on both GIL and free-threaded/no-GIL builds. + // + // The Mutex was only load-bearing under the old `Arc>`: the borrow + // flag is per pyclass instance, so two *distinct* FSharpArray objects aliasing + // one Vec (the old sharing bug) each passed their own borrow check and could + // race on the same buffer — the Mutex was the only guard. With the Arc gone, + // every Vec has exactly one owning instance and the borrow checker covers it. + // + // Note this does not make concurrent *mutation* logically correct: like .NET + // arrays, these are not thread-safe for unsynchronized writes (the Mutex never + // gave logical safety either, only memory safety). Callers must synchronize + // shared mutation exactly as they would in .NET. + // + // Cloning deep-copies the element references, giving correct F# value semantics + // (a clone never aliases the source's mutable storage). PyObject(Vec>), } From e6a847a01783aff52ef5e4931d49da82e5270c01 Mon Sep 17 00:00:00 2001 From: Dag Brattli Date: Fri, 10 Jul 2026 16:29:58 +0200 Subject: [PATCH 5/5] docs(python): tighten generic-storage comment Condense the PyObject storage comment (23->10 lines) while keeping the coupled Arc<->Mutex reasoning and the not-thread-safe caveat. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/fable-library-py/src/native_array.rs | 31 +++++++----------------- 1 file changed, 9 insertions(+), 22 deletions(-) diff --git a/src/fable-library-py/src/native_array.rs b/src/fable-library-py/src/native_array.rs index 0fcc7bd8b4..6aa607844f 100644 --- a/src/fable-library-py/src/native_array.rs +++ b/src/fable-library-py/src/native_array.rs @@ -127,29 +127,16 @@ pub enum NativeArray { Float32(Vec), Float64(Vec), Bool(Vec), - // Generic storage. A plain Vec is sufficient — no Mutex needed — but only - // *because* we also dropped the `Arc`: the two changes are coupled. + // Generic storage. No Mutex needed: all mutation goes through `&mut` receivers, + // which PyO3 borrow-checks per pyclass instance (atomic `try_borrow_mut`), so a + // racing conflict raises `AlreadyBorrowed` instead of tearing the Vec — on both + // GIL and no-GIL builds. This works only *because* the Arc is also gone: the + // borrow flag is per-instance, so the old `Arc` aliasing (two arrays, one + // Vec) needed the Mutex; with one owner per Vec, the borrow checker suffices. + // (Like .NET arrays, this is still not thread-safe for unsynchronized writes — + // the Mutex only gave memory safety, not logical safety.) // - // Every mutation of this Vec goes through a `&mut` receiver (with a plain Vec - // it can't be otherwise — there is no interior mutability). PyO3 atomically - // borrow-checks each pyclass method at entry (`try_borrow_mut`, backed by an - // AtomicUsize CAS), so two threads can never hold conflicting borrows of the - // same FSharpArray; a racing conflict raises `AlreadyBorrowed` rather than - // tearing the Vec. This holds on both GIL and free-threaded/no-GIL builds. - // - // The Mutex was only load-bearing under the old `Arc>`: the borrow - // flag is per pyclass instance, so two *distinct* FSharpArray objects aliasing - // one Vec (the old sharing bug) each passed their own borrow check and could - // race on the same buffer — the Mutex was the only guard. With the Arc gone, - // every Vec has exactly one owning instance and the borrow checker covers it. - // - // Note this does not make concurrent *mutation* logically correct: like .NET - // arrays, these are not thread-safe for unsynchronized writes (the Mutex never - // gave logical safety either, only memory safety). Callers must synchronize - // shared mutation exactly as they would in .NET. - // - // Cloning deep-copies the element references, giving correct F# value semantics - // (a clone never aliases the source's mutable storage). + // Cloning deep-copies the element references (F# value semantics, no aliasing). PyObject(Vec>), }