Fix Spesh Memory Leaks #941
Conversation
If we cleanup the main thread (with it's per-thread FSA free list) after cleaning up the FSA itself, that corresponds to a use-after-free, and we should not.
After we've inlined a graph the inliner claims ownership over the memory of the inlinee. With this patch, we destroy the original graph, which should simplify lifetime management. The memory region list is merged with the inliners graph so that the memory allocated for the inlinee nodes is not freed.
Many allocated things either come from a spesh candidate or from malloc (or realloc). Or they are assigned to a spesh candidate. So the only way to really know if they are shared with a spesh candidate (or static frame) is by checking if they are pointer-equal. ASAN leaksanitizer is finally clean now for me.
|
Not sure how I feel about this in terms of the upcoming release. Less memory leaks is nice, but current moar+nqp+rakudo seems to be very stable and ready for the release. Merging this after the release sounds much safer to me, but I'd let @jnthn++ judge (I'll likely run toaster again a bit later, so we still have a chance to sneak some stuff in). |
|
+1 for post-release merge. |
|
This looks good, but I agree it's probably most sensible to hold off until after the release, given it's not so trivial a change that I can say "this can't possibly be wrong". :-) I have a couple of fixes that are in the same boat, fwiw: they fix deopt bugs that could theoretically occur on master (even if I didn't ever observe that happening), but are involved enough to carry a small risk of bringing their own issue. I'm happy to lean towards "devil we know" for both those fixes and this PR. |
|
Thank you for the review. I will merge after the release. Maybe for the future, it is better to have an (explicitly stable) pre-release branch than to lock development on master for an indefinite time. |
|
@bdw there is a post-release branch https://github.com/MoarVM/MoarVM/tree/postrelease-opts and you can merge this PR there. |
|
That is literally the opposite of what I meant 😄 |
Spesh memory management is mostly simple except for the cases where it isn't.
We create spesh graphs in three ways:
When we generate a spesh candidate, we transfer ownership of allocated memory (e.g. for inlines, handlers, local type maps, spesh slots) to that candidate.
I found that we had a number of leaks coming from the third case, and that they were not straightforward to fix. The code assumed that inlinees came from spesh candidates and that the spesh candidate would therefore own the allocated memory, and not free it. But in the third case there is no spesh candidate, so those fields would leak.
I fixed this in two ways:
Further, there is a fix for the FSA, so that the per-thread FSA freelist is cleaned up before freeing the memory of the FSA allocator as a whole.