Bug 428048 - Ksysguard leaks memory when opened for a long time
Summary: Ksysguard leaks memory when opened for a long time
Status: RESOLVED FIXED
Alias: None
Product: ksysguard
Classification: Applications
Component: ksysguard (show other bugs)
Version: 5.20.0
Platform: Other Linux
: VHI normal
Target Milestone: ---
Assignee: KSysGuard Developers
URL:
Keywords:
: 424299 426792 428027 (view as bug list)
Depends on:
Blocks:
 
Reported: 2020-10-21 08:49 UTC by Riccardo Robecchi
Modified: 2020-11-19 22:58 UTC (History)
11 users (show)

See Also:
Latest Commit:
Version Fixed In: 5.20.3


Attachments
Ksysguard leaks memory (116.86 KB, image/png)
2020-10-21 08:49 UTC, Riccardo Robecchi
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Riccardo Robecchi 2020-10-21 08:49:58 UTC
Created attachment 132596 [details]
Ksysguard leaks memory

SUMMARY
I keep Ksysguard open when I use my PC and I noticed that it leaks a ton of memory after the update to 5.20.0. 

STEPS TO REPRODUCE
1. Open Ksysguard
2. Leave it open for a few hours

OBSERVED RESULT
Ksysguard reaches very high levels of memory consumption (more than 600 MB in the attached screenshot!) after a few hours.

EXPECTED RESULT
Ksysguard keeps its memory footprint limited.

SOFTWARE/OS VERSIONS
Linux: KDE neon Focal
KDE Plasma Version: 5.20.0
KDE Frameworks Version: 5.75.0
Qt Version: 5.15.0

ADDITIONAL INFORMATION
On top of the standard tabs "process table" and "system load", I have a third tab dedicated to temperatures, which however I had not opened when I took the screenshot (and therefore it shouldn't have loaded).
Comment 1 JanKusanagi 2020-10-22 18:39:23 UTC
For contrast, I haven't experienced this problem after running KSysGuard for 3+ hours.

The only difference in versions, apparently, is that here in Mageia I have Qt 5.15.1, which maybe fixes something in 5.15.0 that "breaks" KSysGuard...
Comment 2 Riccardo Robecchi 2020-10-22 20:48:40 UTC
(In reply to JanKusanagi from comment #1)
> For contrast, I haven't experienced this problem after running KSysGuard for
> 3+ hours.
> 
> The only difference in versions, apparently, is that here in Mageia I have
> Qt 5.15.1, which maybe fixes something in 5.15.0 that "breaks" KSysGuard...

As I am writing this, ksysguard is the process consuming most RAM on my system, standing at 673 MB. It has been open since this morning around 9, which makes it roughly 13 hours. I found this is reliably reproducible as I have experienced it ever since I updated to 5.20.0 without fails.
Comment 3 Umbertho dellarojadecanariaportedelacruzkarrrrrdinalll 2020-10-23 01:41:45 UTC
I can confirm,


Plasma 5.20.1
Frameworks 5.75.0
Qt 5.15.1

Memory used by KSysGuard will steadily increase and can reach 2GiB overnight.

I also suspect this to have started happening for me since version 5.20.0.

There is also https://bugs.kde.org/show_bug.cgi?id=426792 indicating the same for libksysguard, though it was originally reported with 5.19.90.
Comment 4 JanKusanagi 2020-10-23 02:09:49 UTC
Still no issue here. KSysGuard running for 9h17m, using 32,6 MiB.
Comment 5 Umbertho dellarojadecanariaportedelacruzkarrrrrdinalll 2020-10-24 19:03:44 UTC
In case the other bug report isn't monitored:


** https://github.com/KDE/libksysguard/commit/40cb8d6acbb12e516c1d327a9b2574560adcb4ec seems to be working without issues for me

- https://github.com/KDE/libksysguard/commit/23852597c642a5c5303bb994d4d28e20d9c863cb will not compile for me with "error: ‘Updates’ in ‘class KSysGuard::Process’ does not name a type" - like errors

- newer commits will compile ok, but they start eating memory like described


how is it possible that some of us do and others don't experience these problems..
Comment 6 Umbertho dellarojadecanariaportedelacruzkarrrrrdinalll 2020-10-25 18:35:56 UTC
So, 

if a running system updates to a 'bad' version of libksysguard, the effects will not show until the system is rebooted. If a bad version gets replaced with a good one, reboot isn't necessary somehow.

I found that commit 23852597c642a5c5303bb994d4d28e20d9c863cb there might be a problem deleting the objects that are created....

I patched this on my end by changing processcore/read_procsmaps_runnable.cpp to 

-setAutoDelete(false)
+setAutoDelete(true)

I could also just git revert the entire commit and things are back to normal.

I'm having a hard time believing that some will suffer from this and others don't. Reboot if you dare ;)
Comment 7 Bug Janitor Service 2020-10-25 23:00:27 UTC
A possibly relevant merge request was started @ https://invent.kde.org/plasma/libksysguard/-/merge_requests/90
Comment 8 David Edmundson 2020-10-25 23:04:45 UTC
>how is it possible that some of us do and others don't experience these problems..


Based on what I think it is, probably the status bar or something else doing a process list fetch.
Comment 9 David Edmundson 2020-10-26 17:25:26 UTC
Git commit 9b5161cb9a16eaad205631b2a4c382dbbbe2072c by David Edmundson.
Committed on 25/10/2020 at 23:00.
Pushed by davidedmundson into branch 'master'.

Handle smap read result in the correct thread

We set the context to "this" so we should auto disconnect when the
Processes object is destroyed.
But Qt::DirectConnection means that is run in the runner thread.

It is possible to have a scenario where "this" is active when the
connection is made, but destroyed before we get to this line.
Related: bug 428160

But scoping the connect to Processes existing we would have leaked the
worker if Processes got destroyed whilst the worker was running.

M  +8    -5    processcore/processes_linux_p.cpp
M  +3    -8    processcore/read_procsmaps_runnable.cpp
M  +1    -4    processcore/read_procsmaps_runnable.h

https://invent.kde.org/plasma/libksysguard/commit/9b5161cb9a16eaad205631b2a4c382dbbbe2072c
Comment 10 David Edmundson 2020-10-26 17:25:46 UTC
Git commit 2d522614357d6dc5a5a7b738a35574f5d8f69da5 by David Edmundson.
Committed on 26/10/2020 at 17:25.
Pushed by davidedmundson into branch 'Plasma/5.20'.

Handle smap read result in the correct thread

We set the context to "this" so we should auto disconnect when the
Processes object is destroyed.
But Qt::DirectConnection means that is run in the runner thread.

It is possible to have a scenario where "this" is active when the
connection is made, but destroyed before we get to this line.
Related: bug 428160

But scoping the connect to Processes existing we would have leaked the
worker if Processes got destroyed whilst the worker was running.


(cherry picked from commit 9b5161cb9a16eaad205631b2a4c382dbbbe2072c)

M  +8    -5    processcore/processes_linux_p.cpp
M  +3    -8    processcore/read_procsmaps_runnable.cpp
M  +1    -4    processcore/read_procsmaps_runnable.h

https://invent.kde.org/plasma/libksysguard/commit/2d522614357d6dc5a5a7b738a35574f5d8f69da5
Comment 11 David Edmundson 2020-10-26 17:27:18 UTC
This is similar to the comment in #6, only with some more robustness on the syncing to the main thread. 

Lets see if this resolves everything
Comment 12 Riccardo Robecchi 2020-10-26 17:30:19 UTC
(In reply to David Edmundson from comment #11)
> This is similar to the comment in #6, only with some more robustness on the
> syncing to the main thread. 
> 
> Lets see if this resolves everything

Thank you for your help, David. As soon as 5.20.2 is out on Neon I will let you know if the bug was solved.
Comment 13 David Edmundson 2020-10-27 22:19:01 UTC
*** Bug 428027 has been marked as a duplicate of this bug. ***
Comment 14 Umbertho dellarojadecanariaportedelacruzkarrrrrdinalll 2020-10-28 19:08:10 UTC
@David: 'more robustness' was a very kind way of understating my time-bomb ;)


commit 9b5161c does not seem to resolve the issue for me, I tried both master and 5.20.2.

Meanwhile I returned to using e5f0f06 and reverting 2385259 out of that, for a non-explosive solution.
Comment 15 Riccardo Robecchi 2020-10-28 19:46:39 UTC
I can confirm 5.20.2 does not fix the issue so I am marking the bug report as "reopened".
Comment 16 David Edmundson 2020-10-29 09:45:09 UTC
Aha, there's another obvious place for leaking.

   QFile file{m_dir + QStringLiteral("smaps_rollup")};
    if (!file.open(QIODevice::ReadOnly)) {
        return;
    }


We never hit emit finished(), no-one cleans us up.
Also explains why it's some users and not others.
Comment 17 Bug Janitor Service 2020-10-29 11:07:58 UTC
A possibly relevant merge request was started @ https://invent.kde.org/plasma/libksysguard/-/merge_requests/91
Comment 18 David Edmundson 2020-10-29 15:17:47 UTC
Git commit ebcf87527c32db0b6685c63a450489d2c6727069 by David Edmundson.
Committed on 29/10/2020 at 11:07.
Pushed by davidedmundson into branch 'master'.

Autodelete smapsRunnable

The runnable has an early return on error conditions. This means
finished() might never be called.

Now we've removed the accessor and provide the return value in the
signal we can just set autoDelete.

M  +0    -2    processcore/processes_linux_p.cpp
M  +1    -1    processcore/read_procsmaps_runnable.cpp

https://invent.kde.org/plasma/libksysguard/commit/ebcf87527c32db0b6685c63a450489d2c6727069
Comment 19 David Edmundson 2020-10-29 15:24:49 UTC
Git commit 5e48a74d5f2b0cd271b4edc8308dabd8d2a6fd79 by David Edmundson.
Committed on 29/10/2020 at 15:24.
Pushed by davidedmundson into branch 'Plasma/5.20'.

Autodelete smapsRunnable

The runnable has an early return on error conditions. This means
finished() might never be called.

Now we've removed the accessor and provide the return value in the
signal we can just set autoDelete.


(cherry picked from commit ebcf87527c32db0b6685c63a450489d2c6727069)

M  +0    -2    processcore/processes_linux_p.cpp
M  +1    -1    processcore/read_procsmaps_runnable.cpp

https://invent.kde.org/plasma/libksysguard/commit/5e48a74d5f2b0cd271b4edc8308dabd8d2a6fd79
Comment 20 Umbertho dellarojadecanariaportedelacruzkarrrrrdinalll 2020-10-30 02:22:32 UTC
Yes, that does it, thank you very much ;)
Comment 21 Frank Kruger 2020-10-30 21:01:11 UTC
Solves the issue. Thx.
Comment 22 Andrew M 2020-11-19 03:57:44 UTC
*** Bug 424299 has been marked as a duplicate of this bug. ***
Comment 23 Patrick Silva 2020-11-19 22:58:24 UTC
*** Bug 426792 has been marked as a duplicate of this bug. ***