-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add a healthy dose of locks to generic functions #17250
Conversation
@@ -1888,12 +1890,14 @@ static void jl_add_linfo_root(jl_lambda_info_t *li, jl_value_t *val) | |||
size_t rlen = jl_array_dim0(m->roots); | |||
for(size_t i=0; i < rlen; i++) { | |||
if (jl_array_ptr_ref(m->roots,i) == val) { | |||
jl_mutex_unlock(&li->def->writelock); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JL_UNLOCK
?
@@ -1878,6 +1879,7 @@ static void jl_add_linfo_root(jl_lambda_info_t *li, jl_value_t *val) | |||
if (jl_is_leaf_type(val) || jl_is_bool(val) || jl_is_symbol(val)) | |||
return; | |||
JL_GC_PUSH1(&val); | |||
JL_LOCK(&li->def->writelock); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
&m->writelock
Nice!! #16524 probably needs a few more atomic operations to make sure the write is visibly in a consistent state for the reader but this should be good enough for now. |
I rebased this to resolve the conflict with #17116 (also resolved #17250 (comment)) There seems to be many other issues according to the CI though... |
@@ -1176,13 +1180,16 @@ void *jl_get_llvmf(jl_tupletype_t *tt, bool getwrapper, bool getdeclarations) | |||
} | |||
if (linfo->jlcall_api == 2) { | |||
// normally we don't generate native code for these functions, so need an exception here | |||
if (linfo->functionObjectsDecls.functionObject == NULL) | |||
to_function(linfo); | |||
// Yes, this leaks lots of memory. No, I don't care. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uh, why not? This isn't the kind of comment we want to have sitting around in the codebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
also fix a couple of recently-introduced data-flow ordering bugs in typeinf
fix #17059
fix #16524