Add MKLMemory class to expose MKL allocated memory via Python buffer protocol - #182
ndgrigorian wants to merge 19 commits into
Conversation
aed1ff6 to
a99e626
Compare
ef09add to
ac6e9fc
Compare
b2cc6fc to
00fea26
Compare
ac6e9fc to
b39b64b
Compare
00fea26 to
c764a81
Compare
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a new MKLMemory Cython extension type backed by MKL’s allocator, exposes it from the top-level mkl package, and introduces tests/build changes to support C11 atomics and nogil MKL calls.
Changes:
- Introduce
mkl._mkl_memorywithMKLMemory(allocation, buffer protocol, pickling, realloc). - Add pytest coverage for allocation, buffer protocol, and pickling behavior.
- Update MKL C-API declarations/build to support
nogilcalls and C11 atomics (plus MSVC flag).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| mkl/tests/test_mkl_memory.py | Adds tests for MKLMemory creation, buffer protocol, and pickling behavior. |
| mkl/_py_mkl_service.pyx | Releases the GIL around MKL buffer-free calls. |
| mkl/_mkl_service.pxd | Marks MKL externs as nogil and adds malloc/calloc/realloc/free declarations. |
| mkl/_mkl_memory.pyx | Adds the new MKLMemory Cython extension implementing allocation + buffer protocol + pickling. |
| mkl/init.py | Exposes MKLMemory at the package top level. |
| meson.build | Enables C11, adds MSVC atomics flag, and builds the new _mkl_memory extension. |
exposes Python buffer protocol
as atomics are a c11+ feature, specific flags are needed to enable on window
0d4ccfb to
0c75d30
Compare
also address issues with undeclared variables and rename MKLMemory class members
0c75d30 to
6aae4fb
Compare
|
@antonwolfy |
Co-authored-by: Anton <100830759+antonwolfy@users.noreply.github.com>
fadedb2 to
34f8ec3
Compare
antonwolfy
left a comment
There was a problem hiding this comment.
In overall LGTM with few minor nits below
| - **`MKLMemory` pickling:** `__reduce__` must rebuild `type(self)`, not `MKLMemory`, and carry the instance `__dict__` so a subclass survives a round trip. `_mkl_memory_from_bytes` takes the class as an optional third argument — optional so that older pickles still load, and omitted for `MKLMemory` itself so that its pickles stay loadable by older versions — and must reject anything that is not a `MKLMemory` subclass, since every pickle names that function. | ||
| - **`MKLMemory` buffer export:** `__getbuffer__` hands the view to `PyBuffer_FillInfo`, which describes a flat block of unsigned bytes and answers `flags` — `format` only under `PyBUF_FORMAT`, `shape` under `PyBUF_ND`, `strides` under `PyBUF_STRIDES` — instead of filling in fields the consumer did not request. It also takes the reference on the exporter, so `__releasebuffer__` must stay a bare decrement of `exported_buffers`. | ||
| - **Claim before fill:** the `atomic_fetch_add(&self.exported_buffers, 1)` comes *before* the fill, with the claim given back in an `except` clause if the fill raises (`PyBuffer_FillInfo` is declared `except -1`). Claiming afterwards leaves a window in which a concurrent `realloc` frees the block the view was already handed, and the consumer keeps that view — every array over the allocation holds it for as long as the array lives, so the cost is a durably dangling array rather than one bad read. Reproduced with the window widened by a 5 ms sleep on 3.13t: the resize went through and ASan reported `heap-use-after-free` in `array_tobytes`; with the claim first the same resize is refused. No test can observe the ordering, so it has to be kept on purpose. It narrows rather than closes the race — a `realloc` already past its own count check can still free under a fill — which only mutual exclusion would fix. | ||
| - **Backing a NumPy array:** `np.asarray(mem)` and `np.frombuffer(mem, dtype=...)` keep a `memoryview` as `.base` and hold the export for the array's whole lifetime, so `realloc` is refused with `BufferError` until the array goes away. `np.ndarray(shape, buffer=mem)` releases the `Py_buffer` and keeps only an object reference, so only the reference check stands in the way and `refcheck=False` leaves the array dangling — `bytearray` behaves the same there, so it is NumPy's property, not this object's. The array cannot resize the allocation either: it does not own its data, which `PyArray_Resize` refuses ahead of its own reference check, so neither `ndarray.resize(..., refcheck=False)` nor a C caller invoking `PyArray_Resize` directly gets past it (both measured). What does drop the export a live array depends on is `arr.base.release()`, which is caller error the same way it is for any exporter. |
There was a problem hiding this comment.
As a follow-up it might be good to add NumPy interop tests.
That seems never tested:
- np.asarray/np.frombuffer hold the export → realloc raises BufferError
- np.ndarray(buffer=) needs only refcheck
| def test_realloc_refcheck_shared(): | ||
| mem = mkl.MKLMemory(1024) | ||
| alias = mem # noqa: F841 | ||
| with pytest.raises(ValueError, match="referenced by"): |
There was a problem hiding this comment.
It matches both the shared==1 and shared==2 messages, so it pins neither branch.
| assert mem._pointer % alignment == 0 | ||
|
|
||
|
|
||
| def test_realloc_refcheck_shared(): |
There was a problem hiding this comment.
It might be helpful to add 3.14+ specific test:
@pytest.mark.skipif(sys.version_info < (3, 14), reason="IsUniquelyReferenced path")
def test_uniquely_referenced_resizes_on_314():
mem = mkl.MKLMemory(1024) # bare local, sole owner
mem.realloc(2048) # must succeed via LOAD_FAST_BORROW/IsUniquelyReferenced
assert mem.nbytes == 2048Python 3.14 introduced LOAD_FAST_BORROW: when the interpreter calls a method on a local variable (mem.realloc(...)), it can push mem onto the evaluation stack as a borrowed reference (no incref), because the local slot provably keeps the object alive for the call's duration. So during the call the refcount stays 1, IsUniquelyReferenced is true, MayBeShared returns 0, and the resize is allowed.
| finally: | ||
| atomic_store(&self.realloc_in_progress, 0) | ||
|
|
||
| def tobytes(self): |
There was a problem hiding this comment.
tobytes/_pointer/__repr__/__len__ are not export-gated.
On free-threaded builds they can tear/UAF under a concurrent realloc.
Another thread's realloc can mkl_realloc (free/move the block) while tobytes is mid-copy → use-after-free; or tobytes reads the old _memory_ptr together with the new, larger _nbytes → over-read of freed memory.
While the copy constructor bothers to claim for the identical kind of read, it might be treated as consistent with the caller-responsibility model (like documented in realloc).
It makes sense to add similar documentation note as in realloc here at least.
| else: | ||
| args = (self.tobytes(), self._alignment, cls) | ||
|
|
||
| return (_mkl_memory_from_bytes, args, getattr(self, "__dict__", None)) |
There was a problem hiding this comment.
A subclass that declares __slots__ (and no __dict__) stores its attributes in slot descriptors, and such instances have no __dict__ at all. So getattr(self, "__dict__", None) returns None, and those slot-held attributes are silently dropped on pickling — they come back unset after a round trip:
class Tagged(mkl.MKLMemory):
__slots__ = ("tag",)
m = Tagged(1024)
m.tag = "hello"
m2 = pickle.loads(pickle.dumps(m))
m2.tag # AttributeError — the slot value was lostSo we probably need to add a one-line note that __slots__-only subclasses don't round-trip. It's mostly a rare use case we mightn't want to cover.
This PR proposes the introduction of
_mkl_memory.pyx, which implements anMKLMemoryclass that exposes memory allocated viamkl_mallocandmkl_callocto Python via the buffer protocolThe class uses an atomic counter incremented as
__getbuffer__and__releasebuffer__are called to track the views on the buffer to permit use ofmkl_reallocin the object (viareallocmethod). This concept was adapted from the PEP which revised the buffer protocol which proposed this kind of approach to tracking views on a bufferCloses #18