Skip to content

Commit f9cf751

Browse files
Subbarao Garlapatimeta-codesync[bot]
authored andcommitted
Don't destroy Python objects under the JIT compilation lock
Summary: I ran the AI Labs test in D115743871 and got an error and the following stack trace (just relevant parts shown below): ``` 9133 | Stack trace for 1 thread(s) [8782 cinderx_compile]: 9139 | PyEval_AcquireThread ← blocked waiting for the GIL 9140 | pybind11::gil_scoped_acquire() ← explicitly asking for it 9142 | c10::TensorImpl::decref_pyobject() ← must drop a Python ref 9145 | c10::TensorImpl::~TensorImpl() ← a torch Tensor being freed 9153 | _PyObject_FreeInstanceAttributes 9159 | allocator_traits<pair<CompilationKey, ...Ref<PyFunctionObject>>>::destroy 9160 | cinderx::jit::Context::finalizeMultiThreadedCompile() ← holds the mutex 9818 | Stack trace for 1 thread(s) [8772 icvr_launcher_m]: 9819 | __GI___lll_lock_wait ← blocked, untimed 9822 | pthread_mutex_lock 9824 | cinderx::jit::Context::funcDestroyed(...) ← wants the mutex 9835 | Python stack trace for this thread (holds GIL) 9840 | torch/_inductor/compile_fx.py:773 _recursive_post_grad_passes ``` These logs are from the `watcher.cpp` tool, which prints all the stacktraces of all current threads when it sees that the GIL is being held for a while. It seems that there's a deadlock with `funcDestroyed()` having the GIL and waiting on `JITCompilationLock` while `finalizeMultiThreadedCompile()` has `JITCompilationLock` and is waiting on the GIL in order to do deallocations. The fix here makes it so that the deallocations in `finalizeMultiThreadedCompile()` happen outside the `JITCompilationLock`. Reviewed By: DinoV Differential Revision: D116940812 fbshipit-source-id: d9ed1c346a0b36e3cf32e960148a3685cb1a24ce
1 parent 7de3591 commit f9cf751

1 file changed

Lines changed: 26 additions & 19 deletions

File tree

cinderx/Jit/context.cpp

Lines changed: 26 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -482,27 +482,34 @@ void Context::addDeferredFinalization(
482482
}
483483

484484
void Context::finalizeMultiThreadedCompile() {
485-
JITCompilationLock lock;
486-
fixupFunctionEntryCachePostMultiThreadedCompile();
487-
watchPendingTypes();
488-
489-
for (auto& codes : completed_compiles_) {
490-
makeCompiledFunction(
491-
codes.second.second, codes.first, std::move(codes.second.first));
492-
}
493-
completed_compiles_.clear();
494-
495-
for (auto& [func, key] : deferred_finalizations_) {
496-
// Re-resolve the compile rather than trusting a pointer cached while the
497-
// GIL was released; the CompiledFunction may have been freed since, in
498-
// which case it has already erased itself from compiled_codes_ and there
499-
// is nothing left to attach the function to.
500-
auto it = compiled_codes_.find(key);
501-
if (it != compiled_codes_.end()) {
502-
finalizeFunc(func, it->second);
485+
// Destructed outside the lock: decref can block on the GIL, which deadlocks
486+
// against funcDestroyed().
487+
decltype(completed_compiles_) completed;
488+
decltype(deferred_finalizations_) deferred;
489+
490+
{
491+
JITCompilationLock lock;
492+
fixupFunctionEntryCachePostMultiThreadedCompile();
493+
watchPendingTypes();
494+
495+
for (auto& codes : completed_compiles_) {
496+
makeCompiledFunction(
497+
codes.second.second, codes.first, std::move(codes.second.first));
498+
}
499+
completed.swap(completed_compiles_);
500+
501+
for (auto& [func, key] : deferred_finalizations_) {
502+
// Re-resolve the compile rather than trusting a pointer cached while the
503+
// GIL was released; the CompiledFunction may have been freed since, in
504+
// which case it has already erased itself from compiled_codes_ and there
505+
// is nothing left to attach the function to.
506+
auto it = compiled_codes_.find(key);
507+
if (it != compiled_codes_.end()) {
508+
finalizeFunc(func, it->second);
509+
}
503510
}
511+
deferred.swap(deferred_finalizations_);
504512
}
505-
deferred_finalizations_.clear();
506513
}
507514

508515
bool Context::finalizeFunc(

0 commit comments

Comments
 (0)