Skip to content
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

MultiProgress: unclear if finished bars needs to be manually removed #419

Closed
gdesmott opened this issue Mar 25, 2022 · 6 comments
Closed

Comments

@gdesmott
Copy link

I have a long running app using a MultiProgress to which various ProgressBar are regularly added. It's not clear to me from the doc if I have to manually remove those from the multi once they are done or if calling finish_and_clear() is enough.
I want to be sure that memory won't keep growing forever if I keep adding and finishing bars without removing them.

@chris-laplante
Copy link
Collaborator

chris-laplante commented Mar 28, 2022

You don't need to explicitly call finish_and_clear() - letting the ProgressBars go out of scope and drop has the same effect (well, at least as far as memory is concerned - the actual visual results will depend on the ProgressFinish).

In terms of actually reclaiming memory, we could potentially improve things. The MultiState::draw_states Vec will grow to n, where n is the maximum number of concurrently displayed ProgressBars at any given time. As ProgressBars are removed/dropped, the corresponding entry in draw_states is set to None and the index is added to a free set to be re-used for subsequently created ProgressBars. Perhaps we could expose the ability to shrink the draw_states Vec to reclaim some space. Not sure if it matters for real-world applications though.

@djc
Copy link
Member

djc commented Mar 28, 2022

Exposing API to shrink that thing seems overkill. We could do something where, if the free set is both at least, say, 32 elements and more than half of the active set, we defragment the thing.

@chris-laplante
Copy link
Collaborator

Actually I take back what I said. Even if you drop/finish the ProgressBar, it will still be present in MultiState::draw_states and MultiState::ordering. We are not calling MultiProgress::remove anywhere on drop. Looking into this now.

@arifd
Copy link

arifd commented May 6, 2022

So if a thread creates and attaches a ProgressBar then panics, it can't be .finish_and_clear()ed?

Actually I take back what I said. Even if you drop/finish the ProgressBar, it will still be present in MultiState::draw_states and MultiState::ordering. We are not calling MultiProgress::remove anywhere on drop. Looking into this now.

@chris-laplante
Copy link
Collaborator

So if a thread creates and attaches a ProgressBar then panics, it can't be .finish_and_clear()ed?

Actually I take back what I said. Even if you drop/finish the ProgressBar, it will still be present in MultiState::draw_states and MultiState::ordering. We are not calling MultiProgress::remove anywhere on drop. Looking into this now.

Sorry I never replied to this. What you can do is clone the ProgressBar (since its just an Arc around the internal state) and store it somewhere for safe keeping outside of the thread that might panic. If it does panic, you can call finish_and_clear on the handle from another thread.

@chris-laplante
Copy link
Collaborator

To the original point of the issue: indicatif (for the past year or so) has been pruning bars from MultiProgress when they go out of scope. So I think this issue may now be closed. Feel free to reopen if you have more questions.

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

No branches or pull requests

4 participants