search

cjbassi / ytop

A TUI system monitor written in Rust
MIT License
2.16k stars 84 forks source link

Slow Memory Leak #96

Open CosmicHorrorDev opened 4 years ago

CosmicHorrorDev commented 4 years ago

Required information:

Include any of the following information if relevant:

Please copy or attach ~/.local/state/ytop/errors.log if it exists and contains logs:

Hello, there is a very slow memory leak in ytop. It probably hasn't been reported yet since it would take several days before using a noticeable amount more RAM. I actually only noticed it after I was poking around the code making other changes :sweat_smile:. It's nothing too exciting, but it stems from pushing all the readings for the graphs into Vecs without every actually restricting the number of readings that are kept. I decided to monitor this over some time just to see how quickly it would build up over time (This is on a laptop with 4 CPU threads since it matters for the number of CPU readings taken). It would be several more days before the increase in usage would jump from ~30 MiB to ~60MiB, so I decided it was enough information to go ahead and report this.

image

The two ways I could think of fixing this are

  1. Band-aid solution: just keep readings to some arbitrarily high number like 1,000.
  2. Dynamic solution: Keep track of the maximum width used for each graph and limit the number of readings to that value. This would mean if you started ytop in a half-width window, waited for a while then made it full-width it would take time to fill in the extra information, but I would prefer this solution since it seems like less of a hack and handles shrinking and growing the windows well.

I'd be more than happy to implement whatever solution you decide to pick!

neeldug commented 4 years ago

Was just wondering for both cases wouldn’t a VecDeque be optimal since all of the important data is more recent, and its default is pushback popfront?

Edit: after looking up some more, perhaps looking into the circular queue crate is worth considering or implementing a circular queue struct.

CosmicHorrorDev commented 4 years ago

Yes you are right that VecDeque would be more fitting here, I was just trying to keep the change small. Something like a RingBuffer would be even more ideal since you don't have to worry about explicitly dropping old elements as long as the size is set and then later reserved correctly. Once @cjbassi drops in to merge pull requests I can work on seeing about other possible changes (ring_buffer would add another dependency so VecDeque might be a good alternative for that reason).

Also just as another note I'm monitoring the version from #98 with heaptrack and there still seems to be a memory leak from one of this program's dependencies that I would like to track down, but #98 fixes the most egregious leaks.

Thanks for the suggestion though, I'll definitely keep it in mind when the PR gets closer to merging!

Edit: using RingBuffer would also essentially keep track of the max size as its capacity, which I didn't want to rely on with Vec's capacity since it would be easy to do an extra push that would resize automatically.

neeldug commented 4 years ago

Yep sounds good, specifically I had seen a crate here, that also seems to fulfill the same purpose, but honestly it’s probably just a matter of API and implementation.

CosmicHorrorDev commented 4 years ago

Thanks for pointing that out! The only problem I noticed from my admittedly short look was that I don't see any way of modifying the size after creating the CircularQueue which would be needed to handle the 2nd approach. You could go about making a larger CircularQueue and copying over all the elements I suppose to deal with that. But like you said, that's all down to API and implementation.