Inhibit finalizers during codegen #11991
Conversation
tkelman
added
compiler:codegen
tkelman
force-pushed
the
tk/inhibitfinalizers
branch
from
5f641c3
to
742729c
Compare
|
this seems reasonable to me. why did you mark it as WIP? |
tkelman
changed the title
|
because testing it locally takes a while, to see if it helps with intermittent freezes |
|
Sadly I'm still able to reproduce freezes on win64 on this branch in the spawn test (even running tests in serial). This may be fixing a legit problem, but not the Windows one we've been hitting. If anyone can reliably reproduce the Linux segfaults that look related to finalizers running during codegen and this fixes it, then that would at least be a useful data point. |
|
This change seems prudent, even if it doesn't fix our immediate problems. |
|
this lgtm. when someone gets a chance to fix the merge conflicts, i think it can be merged. |
|
|
tkelman
force-pushed
the
tk/inhibitfinalizers
branch
from
742729c
to
a802070
Compare
|
Just kidding, that wasn't that bad. Rebased. |
Inhibit finalizers during codegen
| void jl_gc_inhibit_finalizers(int state) | ||
| { | ||
| if (jl_gc_finalizers_inhibited && !state && !jl_in_gc) { | ||
| jl_in_gc = 1; |
There was a problem hiding this comment.
Remind me why do we need to set jl_in_gc when running finalizers? There doesn't seem to be a problem to enable GC since the arguments are rooted. Is this just for consistency for not allowing task switch in them?
I'm asking mainly because requiring no GC during finalizer runs doesn't look like a trival synchronization to do.
c.c. @carnaval
There was a problem hiding this comment.
I think it was a concern that finalizers could need to be codegenned which might trigger gc again
There was a problem hiding this comment.
the problem with running finalizers during codegen is that we may be currently trying to generate code for that function or some other required function when the finalizer tries to run. this patch re-introduces that possibility, but just makes it slightly rarer.
for inference, we can check the in-inference flag and if set, we instead run a non-inferred version. the equivalent that would be required here is to the in-compile flag and run a non-compiled version of the function.
i'm not sure of the issue with recursively running finalizers, since that has been allowed in the past.
the jl_in_gc flag was introduced to inhibit calls to yield inside finalizers, which would allow fairly unpredictable task switching to occur
Might close #11956, if I followed @carnaval's idea correctly. Will see if this helps at all with #11818 or #7942.