Skip to content

Prevent Memory Buildup by Clearing Joinsets #221

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

Closed
wants to merge 2 commits into from

Conversation

carlaKC
Copy link
Contributor

@carlaKC carlaKC commented Feb 21, 2025

TIL that the memory for a task spawned in a joinset will not be released until its corresponding entry in the joinset is consumed ❗ ❗

This means that the pattern that we currently use of spawning a bunch of tasks then waiting on the joinset at the end of the simulation results in massive memory blowup over time, because we never free the memory from out completed tasks.

Created this PR while diagnosing this issue, and opening it up so that it's available if anyone needs a quickndirty fix.
This can and should be done in a slightly neater way!

Previously, we'd only clear out this joinset at the end of the loop
when we exit. This means that we'd let it build up to track N payments
over the lifetime of the simulation.

Instead, we opportunistically clear out whatever's ready each time we
loop around.
This is a really lazy way of making sure that this joinset doesn't
build up too much over time - ideally we don't have a persistent joinset
in the SimGraph (this seems like a bad pattern if they're going to build
up over time).
@carlaKC
Copy link
Contributor Author

carlaKC commented Feb 21, 2025

I'm going to be OOO for a week (back 3 March), just opening this so that it's here!

@carlaKC carlaKC added this to the V2.4 milestone Mar 7, 2025
@elnosh
Copy link
Collaborator

elnosh commented Mar 7, 2025

hi, I'm new here :) as discussed in the call I thought to have a background task clearing those tasks from the JoinSet.

I was playing a bit with it and added something roughly like this to wait_for_shutdown where that function could be running in the background.
`

    loop {
        tokio::select! {
            _ = self.shutdown_listener.clone() => {
                while let Some(res) = self.tasks.join_next().await {
                    if let Err(e) = res {
                        log::error!("Graph task exited with error: {e}");
                    } 
                }
                break;
            },
            _ = self.tasks.join_next() => {}
        }
    };

`

This would loop and clear tasks from the set as they are finished or clear them all if it received a shutdown signal.

I tried running this code with the scenario from this branch https://github.com/carlaKC/sim-ln/tree/review-club-memleak with Heaptrack and it seems to be clearing the tasks as finished and avoiding the memory buildup.

@carlaKC
Copy link
Contributor Author

carlaKC commented Mar 7, 2025

hi, I'm new here :)

Welcome!

I was playing a bit with it and added something roughly like this to wait_for_shutdown where that function could be running in the background.

That looks great!

Would you like to put this all together and take over the PR? I'm quite short on time at the moment, but would happily review once you've put it all together!

@elnosh
Copy link
Collaborator

elnosh commented Mar 7, 2025

sure!

@carlaKC
Copy link
Contributor Author

carlaKC commented Mar 11, 2025

Replaced by #229 🚀

@carlaKC carlaKC closed this Mar 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants