console-rs / indicatif

A command line progress reporting library for Rust
MIT License
4.36k stars 241 forks source link

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

Closed gdesmott closed 1 year ago

gdesmott commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 1 year ago

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 commented 1 year ago

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.