Protect API objects via GIL instead of atomics

[adamreichold]

Instead of slowing everything down via atomics, utilize that we already hold the GIL when accessing PY_ARRAY_API and PY_UFUNC_API.

[codecov]

Codecov Report

Merging #269 (8ae2634) into main (bc4bf35) will increase coverage by 1.18%.
The diff coverage is 85.88%.

@@            Coverage Diff             @@
##             main     #269      +/-   ##
==========================================
+ Coverage   81.66%   82.84%   +1.18%     
==========================================
  Files          14       13       -1     
  Lines        1554     1551       -3     
==========================================
+ Hits         1269     1285      +16     
+ Misses        285      266      -19     

Impacted Files	Coverage Δ
src/npyffi/array.rs	75.00% <75.00%> (-3.05%)	⬇️
src/dtype.rs	94.06% <82.35%> (-0.81%)	⬇️
src/npyiter.rs	79.61% <82.60%> (+0.23%)	⬆️
src/array.rs	85.40% <94.44%> (+0.23%)	⬆️
src/npyffi/mod.rs	100.00% <100.00%> (ø)
src/sum_products.rs	98.27% <100.00%> (+0.12%)	⬆️
src/npyffi/ufunc.rs

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bc4bf35...8ae2634. Read the comment docs.

[davidhewitt]

Nice, agree there's no need for an atomic here!

I wonder, do you think it's good enough to make all of these unsafe with the # Safety requirement "the caller must be holding the Python GIL"? That would drop the need to have py entirely. (And you could always use Python::assume_gil_acquired() if needed.)

[adamreichold]

I wonder, do you think it's good enough to make all of these unsafe with the # Safety requirement "the caller must be holding the Python GIL"? That would drop the need to have py entirely. (And you could always use Python::assume_gil_acquired() if needed.)

I am actually rather fond of passing the token around as type-level proof of having the lock. I am not sure whether passing a zero-sized type is actually zero-cost in the current Rust ABI, but it should probably not matter at all on architectures with a sufficient number of registers to pass arguments in.

Also note that supplying the token was really mechanic work. There was not a single place where I had to conjure it up out of thing air (using assume_gil_acquired or going via with_gil), but already had safe access to it everywhere I needed it due to this change.

In summary, I would prefer to keep the token around as it significantly improves the maintainability of this code.

[davidhewitt]

Sounds reasonable to me!