Reduce codegen lock scope #46836
Reduce codegen lock scope #46836
Conversation
pchintalapudi
force-pushed
the
pc/typeinf-remove
branch
from
482afa6 to
976d642
Compare
pchintalapudi
force-pushed
the
pc/typeinf-remove
branch
2 times, most recently
from
bc88b43 to
5e1936d
Compare
pchintalapudi
force-pushed
the
pc/codegen-lock-1
branch
from
27fd4ef to
551b349
Compare
| auto decls = jl_emit_code(m, mi, src, jlrettype, output); | ||
| JL_UNLOCK(&jl_codegen_lock); // Might GC | ||
|
|
||
| Function *F = NULL; | ||
| if (m) { |
There was a problem hiding this comment.
This check seems wrong now that m is unconditionally created. How do we indicate failure from jl_emit_code now?
There was a problem hiding this comment.
jl_emit_code resets the module if it errors, so the check is still valid.
| @@ -267,7 +267,6 @@ void *jl_create_native_impl(jl_array_t *methods, LLVMOrcThreadSafeModuleRef llvm | |||
| jl_method_instance_t *mi = NULL; | |||
| jl_code_info_t *src = NULL; | |||
| JL_GC_PUSH1(&src); | |||
| JL_LOCK(&jl_codegen_lock); | |||
There was a problem hiding this comment.
You have inverted the ordering of jl_codegen_lock and the ThreadSafeModule lock. Can you update the devlocks to reflect that? Is that even valid?
There was a problem hiding this comment.
So the lock is taken in small spurts to set up the TSM, but we know that doesn't take the codegen lock, and we release the context lock before attempting to take the codegen lock, and then reacquire the context lock during the params constructor. Since the context lock isn't actively being held during the acquisition of the codegen lock, there should be no priority inversion occurring.
There was a problem hiding this comment.
Ah, okay, I assumed clone.getContext was getting the lock, not jl_codegen_params_t
| @@ -267,7 +267,6 @@ void *jl_create_native_impl(jl_array_t *methods, LLVMOrcThreadSafeModuleRef llvm | |||
| jl_method_instance_t *mi = NULL; | |||
| jl_code_info_t *src = NULL; | |||
| JL_GC_PUSH1(&src); | |||
| JL_LOCK(&jl_codegen_lock); | |||
There was a problem hiding this comment.
Ah, okay, I assumed clone.getContext was getting the lock, not jl_codegen_params_t
(cherry picked from commit 09a6ff8)
KristofferC
added
backport 1.9
Depends on #46825 for type inference lock removal.