Introduce Array API - #25
Conversation
Added GIL release for array functions Added check_bytes_within_dist to mirror check_hexstrings_within_dist
Fixed 4 space/8 space tab problems
mrecachinas
left a comment
There was a problem hiding this comment.
This looks fine. I only noted two changes before we'll want to merge. I'm not certain off-hand why you're observing that benchmark behavior, but I'd have to look further into it.
I'm comfortable merging and cutting a release after you make those changes if it suits your needs. 👍
There was a problem hiding this comment.
Pull Request Overview
This PR adds cleaner APIs for array handling by introducing three new functions for finding matches within arrays of byte data. The changes include GIL release for array operations, comprehensive test coverage, documentation updates, and benchmarking support for the new functionality.
Key changes:
- Added three new array operation functions: first, best, and all matches within a specified Hamming distance
- Introduced
check_bytes_within_distfunction for byte-to-byte comparison with explicit naming - Added GIL release mechanisms for improved performance in multi-threaded environments
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| test/test_hexhamming.py | Comprehensive test coverage for new array APIs with edge cases and benchmarking |
| hexhamming/python_hexhamming.cc | Implementation of new wrapper functions with GIL release and improved error handling |
| hexhamming/_version.h | Version bump to 2.3.0 reflecting new functionality |
| README.rst | Documentation for array APIs with examples and usage tips |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
- Fix missing return NULL after PyErr_NoMemory in check_bytes_arrays_all_within_dist - Fix memory leak of out buffer on error paths - Add missing Py_DECREF(tup) after PyList_Append - Optimize _best to use early-termination check before computing full distance - Optimize _all to use early-termination check before computing full distance - Replace tabs with spaces for consistent formatting
The invalid language='c++11' was silently ignored by setuptools, causing the extension to not link against libstdc++. This resulted in undefined symbol errors for operator new[]/delete[] (_Znam/_ZdaPv) on Linux CI.
Resolved conflicts in test/test_hexhamming.py: - Kept PR's new array API imports (first, best, all, check_bytes_within_dist) - Applied master's black formatting style - Kept PR's renamed function names (first/best/all variants)
|
Hey, I dug into the benchmark anomaly -- turns out it was a combination of bugs that we've now fixed:
After fixing those (plus a malloc safety fix and a NEON edge case), benchmarks now show the expected ordering: |
* Fix security and correctness issues in array API - Fix Py_BuildValue format specifiers: use 'L' for int64_t and 'K' for uint64_t instead of 'i' which silently truncates 64-bit values - Fix signed integer overflow UB in best_within_dist when max_dist is large: use -1 sentinel instead of max_dist+1 - Replace new[]/delete[] with malloc/free + NULL check to prevent uncaught std::bad_alloc from crashing the process - Fix NEON bug: max_dist > 0 should be max_dist >= 0 so that max_dist=0 correctly delegates to the native path Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Add Python 3.14 to CI build matrix Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Added cleaner APIs for array handling (find first, best, all matches)
Added GIL release for array ops
Added docs for array ops to Readme
Added tests for array ops
Added explicit
hamming_distance_byteto have the same interface ashamming_distance_string. Functionally the find first array op does the same thing, but this being Python I'm weary of having two apparently interchangeable functions that have different return types, that just smells like trouble. ;)It all seems to work fine, as far as I can tell. The only thing I don't understand is the benchmarking: the
bestandallarray ops are about 2 orders of magnitude faster than thefirstone. Either the compiler does very high-level optimization magic, or something is wrong that I can't see...Thanks!