Skip to content

Commit b61b80e

Browse files
committed
fix(python): make writable batch outputs sound
Keep the GIL while writing caller-owned buffers and reject writable _into APIs on free-threaded Python, where buffer exports do not guarantee exclusive access. Packed APIs remain available for detached computation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 01071e57-12fd-4f42-bc67-e2b274d0b007
1 parent 0f1380e commit b61b80e

3 files changed

Lines changed: 133 additions & 60 deletions

File tree

README.rst

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -209,7 +209,11 @@ records:
209209

210210
``hamming_distances_bytes_into`` requires ``out`` to be a writable,
211211
C-contiguous byte buffer of exactly ``count * 8`` bytes; read-only,
212-
non-contiguous, or wrong-size outputs raise ``ValueError``.
212+
non-contiguous, or wrong-size outputs raise ``ValueError``. Writable ``_into``
213+
APIs are unavailable on free-threaded Python because the buffer protocol does
214+
not provide exclusive access; use the corresponding ``_packed`` API there.
215+
On standard Python builds, ``_into`` keeps the GIL while writing; use
216+
``_packed`` when detached computation is more important than buffer reuse.
213217

214218
Multi-query catalog scans run one catalog against many contiguous queries in
215219
one call, mirroring the shape of repeated single-query calls:
@@ -254,7 +258,9 @@ distances and ``u32`` indices instead of Python tuples:
254258
The ``_packed`` variant returns two ``bytes`` objects; ``_into`` writes into
255259
caller-provided writable buffers and returns the match count. Element widths
256260
whose maximum possible distance exceeds ``u16::MAX`` bits, and catalogs with
257-
more than ``u32::MAX`` records, are rejected.
261+
more than ``u32::MAX`` records, are rejected. On free-threaded Python, use
262+
``_packed`` because writable ``_into`` buffers cannot be made exclusive through
263+
the Python buffer protocol.
258264

259265
Benchmark
260266
---------

src/python.rs

Lines changed: 81 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -662,6 +662,20 @@ fn buffer_ranges_overlap(a_ptr: *const u8, a_len: usize, b_ptr: *const u8, b_len
662662
a_start < b_end && b_start < a_end
663663
}
664664

665+
#[inline]
666+
fn ensure_writable_into_supported() -> PyResult<()> {
667+
#[cfg(Py_GIL_DISABLED)]
668+
{
669+
return Err(PyValueError::new_err(
670+
"writable `_into` APIs are unavailable on free-threaded Python; use the packed API",
671+
));
672+
}
673+
#[cfg(not(Py_GIL_DISABLED))]
674+
{
675+
Ok(())
676+
}
677+
}
678+
665679
/// Compute Hamming distances between corresponding fixed-width records in `a`
666680
/// and `b`. Returns a list of `int` distances, one per record.
667681
///
@@ -732,56 +746,58 @@ fn hamming_distances_bytes_packed<'py>(
732746
#[pyfunction]
733747
#[pyo3(signature = (a, b, element_size, output))]
734748
fn hamming_distances_bytes_into(
735-
py: Python<'_>,
749+
_py: Python<'_>,
736750
a: &Bound<'_, PyAny>,
737751
b: &Bound<'_, PyAny>,
738752
element_size: usize,
739753
output: &Bound<'_, PyAny>,
740754
) -> PyResult<usize> {
741-
// Acquire input buffers first (may be bytes fast-path or general buffer),
742-
// then the writable output buffer. Any Bound<'_, PyAny> references stay
743-
// out of the detach closure — only raw slices and pointers cross the GIL
744-
// boundary, which the pinned guards keep alive for the whole call.
755+
ensure_writable_into_supported()?;
745756
let mut buf_out = std::pin::pin!(SimpleByteBuffer::new());
746757
buf_out.as_mut().acquire_writable(output)?;
747758
let out_len = buf_out.len();
748-
// SAFETY: pointer captured while GIL is still held; the pinned guard keeps
749-
// the buffer exported for the entire call including any detached region.
750-
let out_ptr = buf_out.as_mut().raw_mut_ptr();
751-
// Passing raw pointers into `py.detach` requires the closure to satisfy
752-
// `Ungil`. `*mut u8` is `!Sync`, so shipping the value as a `usize` and
753-
// rematerializing it inside the closure keeps the closure `Ungil`-clean.
754-
let out_ptr_addr = out_ptr as usize;
755-
756-
with_two_readonly_buffers(a, b, |a_slice, b_slice, can_detach| {
757-
let compute = || -> PyResult<usize> {
758-
if buffer_ranges_overlap(
759-
a_slice.as_ptr(),
760-
a_slice.len(),
761-
out_ptr_addr as *const u8,
762-
out_len,
763-
) || buffer_ranges_overlap(
764-
b_slice.as_ptr(),
765-
b_slice.len(),
766-
out_ptr_addr as *const u8,
767-
out_len,
768-
) {
769-
return Err(PyValueError::new_err(
770-
"output buffer must not overlap input buffers",
771-
));
772-
}
773-
// SAFETY: exclusive access to the writable buffer for the closure
774-
// duration; the pinned guard keeps the memory alive.
775-
let out_slice =
776-
unsafe { std::slice::from_raw_parts_mut(out_ptr_addr as *mut u8, out_len) };
777-
crate::bytes_pairwise_distances_into(a_slice, b_slice, element_size, out_slice)
778-
.map_err(PyValueError::new_err)
779-
};
780-
if can_detach && a_slice.len() >= ARRAY_GIL_RELEASE_THRESHOLD {
781-
py.detach(compute)
782-
} else {
783-
compute()
759+
let out_ptr_addr = buf_out.as_mut().raw_mut_ptr() as usize;
760+
761+
with_two_readonly_buffers(a, b, |a_slice, b_slice, _can_detach| {
762+
if buffer_ranges_overlap(
763+
a_slice.as_ptr(),
764+
a_slice.len(),
765+
out_ptr_addr as *const u8,
766+
out_len,
767+
) || buffer_ranges_overlap(
768+
b_slice.as_ptr(),
769+
b_slice.len(),
770+
out_ptr_addr as *const u8,
771+
out_len,
772+
) {
773+
return Err(PyValueError::new_err(
774+
"output buffer must not overlap input buffers",
775+
));
776+
}
777+
if element_size == 0 {
778+
return Err(PyValueError::new_err("`element_size` must be >0"));
779+
}
780+
if a_slice.len() != b_slice.len() {
781+
return Err(PyValueError::new_err("bytes are NOT the same length"));
784782
}
783+
if a_slice.len() % element_size != 0 {
784+
return Err(PyValueError::new_err(
785+
"length must be a multiple of `element_size`",
786+
));
787+
}
788+
let count = a_slice.len() / element_size;
789+
let expected = count
790+
.checked_mul(8)
791+
.ok_or_else(|| PyValueError::new_err("output capacity overflows"))?;
792+
if out_len != expected {
793+
return Err(PyValueError::new_err("`out` must be exactly count*8 bytes"));
794+
}
795+
796+
// Writable buffer exports are not exclusive. Keep the GIL attached
797+
// while constructing and using the mutable Rust slice.
798+
let out_slice = unsafe { std::slice::from_raw_parts_mut(out_ptr_addr as *mut u8, out_len) };
799+
crate::bytes_pairwise_distances_into(a_slice, b_slice, element_size, out_slice)
800+
.map_err(PyValueError::new_err)
785801
})
786802
}
787803

@@ -960,13 +976,14 @@ fn check_bytes_arrays_all_within_dist_packed<'py>(
960976
#[pyfunction]
961977
#[pyo3(signature = (array_of_elems, elem_to_compare, max_dist, out_distances, out_indices))]
962978
fn check_bytes_arrays_all_within_dist_into(
963-
py: Python<'_>,
979+
_py: Python<'_>,
964980
array_of_elems: &Bound<'_, PyAny>,
965981
elem_to_compare: &Bound<'_, PyAny>,
966982
max_dist: i64,
967983
out_distances: &Bound<'_, PyAny>,
968984
out_indices: &Bound<'_, PyAny>,
969985
) -> PyResult<usize> {
986+
ensure_writable_into_supported()?;
970987
if max_dist < 0 {
971988
return Err(PyValueError::new_err("`max_dist` must be >=0"));
972989
}
@@ -976,7 +993,6 @@ fn check_bytes_arrays_all_within_dist_into(
976993
buf_i.as_mut().acquire_writable(out_indices)?;
977994
let d_len = buf_d.len();
978995
let i_len = buf_i.len();
979-
// SAFETY: pointers captured under GIL; pinned guards outlive detached use.
980996
let d_ptr_addr = buf_d.as_mut().raw_mut_ptr() as usize;
981997
let i_ptr_addr = buf_i.as_mut().raw_mut_ptr() as usize;
982998
if buffer_ranges_overlap(
@@ -990,11 +1006,14 @@ fn check_bytes_arrays_all_within_dist_into(
9901006
));
9911007
}
9921008

993-
with_two_readonly_buffers(array_of_elems, elem_to_compare, |big, small, can_detach| {
994-
let compute = || -> PyResult<usize> {
995-
let d_ptr = d_ptr_addr as *mut u8;
996-
let i_ptr = i_ptr_addr as *mut u8;
997-
for (ptr, len) in [(d_ptr as *const u8, d_len), (i_ptr as *const u8, i_len)] {
1009+
with_two_readonly_buffers(
1010+
array_of_elems,
1011+
elem_to_compare,
1012+
|big, small, _can_detach| {
1013+
for (ptr, len) in [
1014+
(d_ptr_addr as *const u8, d_len),
1015+
(i_ptr_addr as *const u8, i_len),
1016+
] {
9981017
if buffer_ranges_overlap(big.as_ptr(), big.len(), ptr, len)
9991018
|| buffer_ranges_overlap(small.as_ptr(), small.len(), ptr, len)
10001019
{
@@ -1042,22 +1061,26 @@ fn check_bytes_arrays_all_within_dist_into(
10421061
"out_indices must have capacity for num_records * 4 bytes",
10431062
));
10441063
}
1064+
10451065
let matches = crate::bytes_array_all_within_dist(big, small, max_dist)
10461066
.map_err(PyValueError::new_err)?;
1047-
for (k, (d, idx)) in matches.iter().enumerate() {
1067+
// Writable buffer exports are not exclusive. Keep the GIL attached
1068+
// while serializing into the caller's Python-owned buffers.
1069+
for (k, (distance, index)) in matches.iter().enumerate() {
10481070
unsafe {
1049-
std::ptr::write_unaligned(d_ptr.add(k * 2) as *mut u16, (*d as u16).to_le());
1050-
std::ptr::write_unaligned(i_ptr.add(k * 4) as *mut u32, (*idx as u32).to_le());
1071+
std::ptr::write_unaligned(
1072+
(d_ptr_addr as *mut u8).add(k * 2) as *mut u16,
1073+
(*distance as u16).to_le(),
1074+
);
1075+
std::ptr::write_unaligned(
1076+
(i_ptr_addr as *mut u8).add(k * 4) as *mut u32,
1077+
(*index as u32).to_le(),
1078+
);
10511079
}
10521080
}
10531081
Ok(matches.len())
1054-
};
1055-
if can_detach && big.len() >= ARRAY_GIL_RELEASE_THRESHOLD {
1056-
py.detach(compute)
1057-
} else {
1058-
compute()
1059-
}
1060-
})
1082+
},
1083+
)
10611084
}
10621085

10631086
// ---------------------------------------------------------------------------

test/test_batch.py

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212

1313
import array
1414
import random
15+
import sys
1516

1617
import pytest
1718
from hexhamming import (
@@ -155,6 +156,17 @@ def test_pairwise_into_accepts_unaligned_memoryview():
155156
assert parsed == dists
156157

157158

159+
def test_pairwise_into_accepts_live_writable_alias():
160+
width = 16
161+
count = 5
162+
a = _random_bytes(width * count, 1)
163+
b = _random_bytes(width * count, 2)
164+
out = bytearray(count * 8)
165+
alias = memoryview(out)
166+
assert hamming_distances_bytes_into(a, b, width, out) == count
167+
assert bytes(alias) == hamming_distances_bytes_packed(a, b, width)
168+
169+
158170
def test_pairwise_into_rejects_overlapping_input():
159171
backing = bytearray(range(20))
160172
a = memoryview(backing)[:16]
@@ -382,6 +394,21 @@ def test_into_all_rejects_overlapping_outputs():
382394
)
383395

384396

397+
def test_into_all_accepts_live_writable_aliases():
398+
width = 16
399+
count = 16
400+
catalog = _random_bytes(width * count, 131)
401+
query = catalog[:width]
402+
d_out = bytearray(count * 2)
403+
i_out = bytearray(count * 4)
404+
d_alias = memoryview(d_out)
405+
i_alias = memoryview(i_out)
406+
n = check_bytes_arrays_all_within_dist_into(catalog, query, 128, d_out, i_out)
407+
d_bytes, i_bytes = check_bytes_arrays_all_within_dist_packed(catalog, query, 128)
408+
assert bytes(d_alias[: n * 2]) == bytes(d_bytes)
409+
assert bytes(i_alias[: n * 4]) == bytes(i_bytes)
410+
411+
385412
def test_into_all_rejects_overlapping_input():
386413
catalog = bytearray(range(64))
387414
query = bytes(16)
@@ -395,6 +422,23 @@ def test_into_all_rejects_overlapping_input():
395422
)
396423

397424

425+
@pytest.mark.skipif(
426+
not hasattr(sys, "_is_gil_enabled") or sys._is_gil_enabled(),
427+
reason="requires free-threaded Python",
428+
)
429+
def test_writable_into_apis_rejected_without_gil():
430+
with pytest.raises(ValueError, match="unavailable on free-threaded Python"):
431+
hamming_distances_bytes_into(b"\x00" * 16, b"\x00" * 16, 16, bytearray(8))
432+
with pytest.raises(ValueError, match="unavailable on free-threaded Python"):
433+
check_bytes_arrays_all_within_dist_into(
434+
b"\x00" * 16,
435+
b"\x00" * 16,
436+
0,
437+
bytearray(2),
438+
bytearray(4),
439+
)
440+
441+
398442
def test_pairwise_accepts_bytearray_and_memoryview_and_array():
399443
width = 16
400444
count = 4

0 commit comments

Comments
 (0)