Make mx.compile cache erasing thread safe - #4248
Conversation
|
A question rather than a correction, since you may have scoped this out already.
An erase reaching the tracing thread while it is inside The same thing re-entrant on one thread, when a traced body drops another wrapper over the same callable: Repro for the second, no threads needed: constexpr std::uintptr_t outer_id = 0xf11d;
std::function<std::vector<array>(const std::vector<array>&)> fun =
[&](const std::vector<array>& inputs) {
detail::compile_erase(detail::compiler_cache(), outer_id);
return std::vector<array>{inputs[0] + array(1.0f)};
};
auto compiled = detail::compile(fun, outer_id);
eval(compiled({array(3.0f)}));The re-entrant one predates this PR and reproduces on main. The cross-thread one only becomes reachable once erases reach the tracing thread's cache. Holding the |
|
Technically it won't happen in python bindings because it is guaranteed that a function won't be destructed until nothing is using it, i.e. erasing won't happen while compiling the same function. But it might make sense not giving users a foot gun when it is not hard to do. |
|
The CI failures are a teardown crash introduced by this branch. Three lines reproduce it, no import mlx.core as mx
f = mx.compile(lambda x: x + 1)
mx.eval(f(mx.array([1.0])))Linux, CPU-only build: exit 139 on The symbolized stack for thread 1 is further down that same log, addresses trimmed:
An early return fixes it here, repro 0/3 and 835 tests OK: ~ThreadCleanup() {
if (!Py_IsInitialized()) {
return;
}
nb::gil_scoped_acquire gil;
mx::detail::compile_clear_cache(cache);
}That covers the after-finalization case only, it does not change what a thread exiting during |
9e92a55 to
f7811bd
Compare
Close #3940.
Compiled python function would erase the cache automatically on destruction, but the destruction can happen on any thread so it could happen that a cache entry gets deleted on a different thread from creation.
This PR enforces the erasing to happen on the original cache rather than the cache in the current thread, and makes
CompilerCachethread safe to the race condition above. The test is from #4096.The code is not strictly thread safe to avoid unnecessary overheads based on following assumptions: