Slow Memory Leak

[CosmicHorrorDev]

Required information:

• ytop version (ytop -V): ytop 0.6.2
• The output of uname -a: Linux Cthulhu 5.7.6-arch1-1 #1 SMP PREEMPT Thu, 25 Jun 2020 00:14:47 +0000 x86_64 GNU/Linux

Include any of the following information if relevant:

• Terminal emulator (e.g. iTerm or gnome terminal): alacritty 0.4.3
• tmux version (tmux -V): NA
• Any relevenat hardware info: NA

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 😅. 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.

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]

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]

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]

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]

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.