Implement thread-safe cpuinfo deinitialization for multiple consumers - #400
Implement thread-safe cpuinfo deinitialization for multiple consumers#400crvineeth97 wants to merge 5 commits into
Conversation
|
@GregoryComer @georgthegreat @malfet Requesting a review |
|
I would suggest making this behavior an opt-in. Why ORT (which is the only library that poorly supports static linking) should cause a disruption to every other program? |
|
At the moment the code is susceptible to a race condition if cpuinfo initialization takes place in pre-main (which does not look impossible to me). |
|
There is nothing wrong with global variables being left upon program exit. Valgrind properly handles this, memory sanitizers simply does not report it by default. I think the original problem regards ORT only and should be fixed accordingly. |
|
Thanks @georgthegreat, that's a fair point. I've updated the PR to make the deinit behavior opt-in so it doesn't affect any existing consumers. For what it's worth, even the opt-in path uses a constant-initialized lock, so it should be safe to acquire from a pre-main initializer. I agree that for process exit, leaked globals are cleaned up by the OS and memory sanitizers handle this well. The motivating case is different thought, at least on Windows, it's a dynamic DLL unload mid-process. ORT is often loaded as a DLL inside another DLL and then FreeLibrary'd while the process keeps running. When that happens, cpuinfo's globals are never released and get flagged as a memory leak. @GregoryComer @fbarchard, I noticed there don't seem to be any tests actually running in the CI pipeline. Not sure if I'm missing something here. I've added a simple ctest that exercises the deinit path. Please let me know if that should be removed |
It is a bit arguable. In ONNX Runtime's case, if you don't destroy the global OrtEnv object correctly , it may lead to a crash on exit. This is because C++ objects' destruction order on Windows is different than other platforms. A wrong destruction order could lead to use-after-free issues, which could cause crashes. But, let's say, if you have a global object whose destructor depends on nothing else but the standard C/C++ runtime, in most cases it should be fine. Otherwise you will need to be cautious on that. I am indicating this change should be taken or not. But, I was about to say, the new code does leak something: the mutex you are adding. On some platforms, for example macOS and Android and Linux libc++, the std::mutex's destructor is not a no-op. It's hard to tell whether calling pthread_mutex_destroy is necessary, because it is very platform dependent and even OS version dependent . I just wanted to point out that the new code is introducing another potential memory leak. The easiest way to implement Singleton pattern is to use C++'s function local static, which is also faster than most other choices. But this is C code. Similarly, if you can use std::mutex, use std::mutex. Otherwise you will have to deal with the platform differences, and that is much more complicated and could lead to endless debates. Like, what's wrong if I don't call pthread_mutex_destroy at all? |
|
Yep, a lot of fuss here is caused by the lack of C++ runtime. |
|
Thanks @snnn, I agree std::mutex would be the clean approach. Unfortunately, we have to deal with C code here. A couple of clarifications: the code uses a static-init pthread_mutex_t, where the destroy is effectively a no-op on mainstream libcs, and SRWLOCK has no cleanup API and allocates no resources. The lifecycle lock has to outlive the init/deinit cycles so it cannot be destroyed. A use-after-destroy is worse than leaving a single fixed-size static around. It also doesn't accumulate across DLL load/unload and doesn't get flagged by leak checkers. |
108a790 to
55827f0
Compare
|
@malfet Rebased to latest main which should fix the cmake-uwp build.
Should the new build I added run the tests? |
|
@malfet @GregoryComer Requesting your reviews |
malfet
left a comment
There was a problem hiding this comment.
This is effectively a roll-back of #387 , because all of the changes you are adding as well as test are guarded behind #ifdefs which are off by default. Let's just revert the original change and you can propose a new one, with clear init/deinit routines and tests, which are enabled by default
55827f0 to
3c8fbe4
Compare
Summary
This PR reintroduces the
cpuinfo_deinitialize()cleanup from #387 with thread-safe lifecycle management for multiple independent consumers.#387 correctly released cpuinfo's heap-allocated global state, but it reset
pthread_once_t/INIT_ONCEguards and freed that shared state on the firstcpuinfo_deinitialize()call. In programs where libraries such as TensorFlow Lite, ruy, pthreadpool, and ONNX Runtime use cpuinfo independently, one consumer could therefore deinitialize cpuinfo while another consumer was still reading it. The change was reverted in #411.This implementation keeps deinitialization enabled by default while addressing those concurrency and compatibility issues.
Implementation
SRWLOCKon Windowspthread_mutex_ton pthread platformscpuinfo_initialize()performs platform initializationcpuinfo_deinitialize()releases one reference and performs platform cleanup only after the final reference is releasedprocessor_infos.There is no opt-in build flag: the synchronized lifecycle is the default implementation.
Tests
The lifecycle tests use only the public cpuinfo API and cover:
The simultaneous-consumer test deterministically reproduces the #387 failure: with the old implementation, one thread frees the shared state and other threads abort when a getter observes cpuinfo as uninitialized.
init-testis run in the Linux, macOS, and Windows CMake CI jobs.