Destroy all actors on runtime termination #2058
Conversation
|
Nice. |
|
I think this change is interesting enough that it deserves a CHANGELOG - FIXED. Thoughts? |
|
Agreed. |
Praetonus
added
the
changelog - fixed
|
Just to be clear, I think this only makes a difference for when the pony runtime is being run as embedded in a larger program, and that larger program isn't over yet - right? (for example, when embedding the pony runtime in your own C program, or when running JIT tests in the compiler test suite) In cases where the pony runtime termination is immediately followed by the termination of the OS process (as is the case for a normal Pony binary), I think this only matters from a pedantry perspective, because the OS will free memory allocated by the OS process when it gets terminated - is that correct? I'm not trying to say we shouldn't do this change - obviously we still should - I just want to make sure we're being clear and not overstating the scope of the problem that was fixed. |
src/libponyrt/gc/cycle.c
Outdated
| @@ -139,6 +139,9 @@ DEFINE_HASHMAP(ponyint_perceivedmap, perceivedmap_t, perceived_t, | |||
| perceived_hash, perceived_cmp, ponyint_pool_alloc_size, | |||
| ponyint_pool_free_size, perceived_free); | |||
|
|
|||
| DECLARE_STACK(ponyint_actorstack, actorstack_t, pony_actor_t); | |||
| DEFINE_STACK(ponyint_actorstack, actorstack_t, pony_actor_t); | |||
There was a problem hiding this comment.
Should we choose a name for this stack instance that makes it more clear that it is only used at termination time?
My first thought when I saw this stack declaration is that we'd be pushing to it at creation time for every actor, which is a much scarier concept in terms of program overhead and coordination than what's actually going on (pushing to it and popping from it at runtime termination).
There was a problem hiding this comment.
I don't see what could be a better name. Maybe we could move the stack declaration closer to where it is used instead?
There was a problem hiding this comment.
I think terminate could be included in the name somehow.
If you don't want to change the name, can you add a comment above the declaration that explains how/when it is used?
There was a problem hiding this comment.
I'll see how it looks like in the code.
There was a problem hiding this comment.
I went with pendingdestroystack, since that's what it's storing.
|
@jemc Yes, it doesn't make any difference if the program exits immediately after the runtime terminates and the OS claims the memory back. |
The memory for actors that were still alive on runtime termination wasn't being freed, leading to memory leaks.
Praetonus
force-pushed
the
runtime-terminate-actor-free
branch
from
c8944d0
to
b0d1585
Compare
The memory for actors that were still alive on runtime termination wasn't being freed, leading to memory leaks.