Load CppInterOp explicitly instead of from a global constructor - #26
Conversation
Test Results
|
7b7fc7e to
7f3655e
Compare
| if (getenv("CPPJIT_OPT_LEVEL")) | ||
| optLevel = atoi(getenv("CPPJIT_OPT_LEVEL")); | ||
| // set opt level (default to 2 if not given; Cling itself defaults to 0) | ||
| int optLevel = 2; |
There was a problem hiding this comment.
That in practice slows things down because most of the functions are not in hot loops.
There was a problem hiding this comment.
That means it is already slow, because this is not a change that I introduced in this PR, it always has been like this in the forks (and cppyy upstream). My changes are non functional w.r.t what we actually do at interpreter setup time.
What would be the right optlevel?
There was a problem hiding this comment.
0 is the right default and #pragma XXX optimize N should be the annotation for hot code. If that's not new we can measure before and after changing but ROOT had 50% slowdowns when switching from 0 to 1 iirc.
There was a problem hiding this comment.
Okay but currently we don't have a way of #pragma XXX optimize N for clang-repl. Ideally we should guard the following:
if (optLevel != 0) {
std::ostringstream s;
s << "#pragma cling optimize " << optLevel;
Cpp::Process(s.str().c_str());
}
with CPPJIT_USE_CLING since it does not do anything in the clang-repl case.
And in the case of ROOT none of this would apply because the interpreter is created by ROOT, and none of this code runs.
There was a problem hiding this comment.
0 is the right default and
#pragma XXX optimize Nshould be the annotation for hot code. If that's not new we can measure before and after changing but ROOT had 50% slowdowns when switching from 0 to 1 iirc.
And yes, that is not new. You can see in the diff.
There was a problem hiding this comment.
I think we should set the opt level to 0 and move on for now.
There was a problem hiding this comment.
Will address in a follow up patch. I realise we should not unconditionally do Cpp::Process("#pragma cling optimize " << optLevel;")) because that only runs in the clang-repl case, so that would be an unrelated change to what this patch does.
There was a problem hiding this comment.
And we can update that once llvm/llvm-project#214793 lands
There was a problem hiding this comment.
Will address in a follow up patch. I realise we should not unconditionally do
Cpp::Process("#pragma cling optimize " << optLevel;"))because that only runs in the clang-repl case, so that would be an unrelated change to what this patch does.
The equivalent of that code would be to pass the -O2 flags to the interpreter. No pragma is needed because it is done on global scope probably intends to make a TU-wide setting.
There was a problem hiding this comment.
Okay makes sense. So we pass -02 by default when creating the interpreter
7f3655e to
423f302
Compare
|
@vgvassilev Addressed your request to separate the fix from the NFC refactor. I would prefer to keep them in the same PR though, having just the refactor into smaller static functions does not seem worth bloating the history. Commit 1: Replace |
423f302 to
121235f
Compare
vgvassilev
left a comment
There was a problem hiding this comment.
Thank you, @aaronj0. LGTM!
The backend used to dlopen
libclangCppInterOpfrom a global constructor, which runs inside the dlopen oflibcppjititself when Python loads the extension. The nesteddlopenre-enters the loader on partially consistent state, runs the inner library's initializers with no ordering guarantees, and holds the loader lock through the entire interpreter and JIT setup, deadlocking any other thread that touches the loader (as reported by @Vipul-Cariappa). The constructor also had no error channel: on failure it printed and left a half-initialized library.CppInterOp loading and interpreter setup now run in an exported, idempotent
LoadCppInterOp()that _cpython_cppjit.py calls before the extension module import. This returns a status that turns into a proper exception on the Python side. TheApplicationStarterglobal is dropped in favor of a clean set of statics, all called byLoadCppInterOp().