make codegen threadsafe, sinking the necessary lock now into JuliaOJIT #55106
Conversation
| @@ -412,7 +411,6 @@ void *jl_create_native_impl(jl_array_t *methods, LLVMOrcThreadSafeModuleRef llvm | |||
| // finally, make sure all referenced methods also get compiled or fixed up | |||
| jl_compile_workqueue(params, policy); | |||
There was a problem hiding this comment.
I think it'd be interesting to parallelize the jl_emit_codeinst call some lines above this; iirc that was occupying a good amount of time during precompilation after the LLVM portion was parallelized. Might also serve to quickly root out any threading bugs.
There was a problem hiding this comment.
That call doesn't take any significant time. My plan was to move this code around later, so that it can more properly call addModule earlier, when each subgraph is complete, as LLVM is where the time is mostly spent.
There was a problem hiding this comment.
Maybe I'm misremembering and it was the entirety of this for loop that was expensive... still I think this for loop should be parallel since addModule doesn't get called here as part of image generation.
| void *fptr; | ||
| jl_read_codeinst_invoke(codeinst, &specsigflags, &invoke, &fptr, 0); | ||
| if (specsig ? specsigflags & 0b1 : invoke == jl_fptr_args_addr) { | ||
| protoname = jl_ExecutionEngine->getFunctionAtAddress((uintptr_t)fptr, invoke, codeinst); |
There was a problem hiding this comment.
I wonder if this should just go all the way and just make up names for the functions in codegen; the JIT can substitute in proper names later, but this would drop one more jit dependency from codegen and the associated locking.
There was a problem hiding this comment.
That is possible. I didn't want to drop the optimization quite yet, but it probably doesn't matter and does make this messier. One reason we did this it this was is because it causes code_llvm to lie about the names, so that those remain somewhat constant between calls. It may still take a bit more work to make sure they are always locally generated names, so that they are constant for code_llvm, and then get corrected later to the real name.
ad35ddf
to
9551c5c
Compare
9551c5c
to
0ddbb6d
Compare
0ddbb6d
to
0c150f9
Compare
This adds a new helper
jl_read_codeinst_invokethat should help manage reading the state out of a CodeInstance correctly everywhere. Then replaces all of the places where we have optimizations in codegen where we check for this (to build a name in the JIT for it) with that call. And finally moves thejl_codegen_lockintojl_ExecutionEngine->jitlockso that it is now more clear that this is only protecting concurrent access to the JIT state it manages (which includes the invoke field of all CodeInstance objects). In a subsequent followup, thatjitlockandcodeinst_in_flightwill be replaced with something akin to the new engine (for CodeInfo inference) which helps partition that JIT lock mechanism (for CodeInstance / JIT insertion) to correspond just to a single CodeInstance, and not globally to all of them.