lxqt / qps

Qt process viewer and manager
https://lxqt.github.io
GNU General Public License v2.0
79 stars 38 forks source link

Memory leak on some desktop environments #150

Closed shlyakpavel closed 5 years ago

shlyakpavel commented 5 years ago

In less than 19 minutes it can take more memory than gnome shell

image

https://github.com/lxqt/qps/pull/149 helped a little, but there is still a huge leak left somewhere else. My proposal is to track everything regarding the leaks here.

tsujan commented 5 years ago

Under Linux+LXQt, it's only 22 MiB after 2 hours. I'll continue using it...

My proposal is to track everything regarding the leaks here.

Definitely.

shlyakpavel commented 5 years ago

Under Linux+LXQt, it's only 22 MiB after 2 hours. I'll continue using it...

Did you try changing update period to 1ms? :D

tsujan commented 5 years ago

For real usage? No. I set it to 500ms and left it in the systray.

But I tested with 10ms while making the patch. I saw a "leak" but it stopped at ~20 MiB. I removed the graph as icon and it stopped at the beginning. So, IMO, it was about how setIcon works in Qt (for systray and the main window).

tsujan commented 5 years ago

Please remove trayicon->setIcon(*pm); and setWindowIcon(*pm); from Qps::set_load_icon() and test again! That might make it easier to find the cause.

shlyakpavel commented 5 years ago

https://programmersexception.blogspot.com/2019/04/memory-leak-con-qicon-en-qt-594-y.html This guy states that QIcon(qpixmap) solves the problem. Moreover, according to https://doc.qt.io/qt-5/qicon.html it's us who use qicon in inproper way. I will dig it deeper in the evening

shlyakpavel commented 5 years ago

In other words We make qpixmap pointer, use it in qicon, it is being copied. When the qpixmap is copied, qicon is correctly used and destroyed. QPixmap is left in the memory.

tsujan commented 5 years ago

This guy states that QIcon(qpixmap) solves the problem.

I'd tried it to no avail but please test it.

it's us who use qicon in inproper way.

I haven't seen such a usage of QIcon anywhere else but I don't think it's wrong.

We make qpixmap pointer,

I'd changed that by changing a few functions (I mean I removed the pointer) but it made no difference.

tsujan commented 5 years ago

BTW, here, the memory usage is 28 MiB now (after ~4 hours).

tsujan commented 5 years ago

OK, it really stopped at 28 MiB (qps shows 28 and gnome-system-monitor shows 28.4 for an hour; I checked several times) — which is very good because I'd opened some of its dialogs.

So, there's no (detectable) memory leak here.

Why it uses so much memory under gnome-shell, I have no idea but memory usage isn't determined by a single factor.

shlyakpavel commented 5 years ago

Linux+LXQt

Tested here on fresh Lubuntu,~~ it's the same as it was on Gnome~~

Please remove trayicon->setIcon(*pm); and setWindowIcon(*pm); from Qps::set_load_icon() and test again! That might make it easier to find the cause.

The leak has gone this way!

shlyakpavel commented 5 years ago

Stopped on 29mb in a couple of minutes (git master on LXQt)

image
tsujan commented 5 years ago

Here, it's still 28 MiB — more precisely, 28.4. I'm starting to trust this app :) It can be left in systray without problem. No memory leak at all.

We should see what's the difference between our use cases. One thing is your 1-ms update interval. I think even 250 ms is too short for a real use case and we should correct that spin-box.

Other things can be gnome or even the virtual machine (I use it with the real computer).

shlyakpavel commented 5 years ago

Other things can be gnome

Looks much like that

or even the virtual machine (I use it with the real computer).

It stops at 29Mib on the same VM with LXQt so it's not the point

tsujan commented 5 years ago

It stops at 29Mib on the same VM with LXQt so it's not the point

Oh, I didn't see your other comment. Good!

Please close this if you agree with me that there's no leak.

tsujan commented 5 years ago

Ha ha.... Gnome is the most nonstandard DE. It even doesn't have a systray.

shlyakpavel commented 5 years ago

Please close this if you agree with me that there's no leak.

Is it possible that leak on gnome is a qps fault? I also wanna test this on KDE and Xfce

tsujan commented 5 years ago

Is it possible that leak on gnome is a qps fault?

I really doubt that. But there's no harm in leaving this open.

shlyakpavel commented 5 years ago

It even doesn't have a systray.

But QPS has a tray icon on gnome anyway (look at my screenshot)...

tsujan commented 5 years ago

That's not the tray icon; it's the window icon. Gnome doesn't have a tray anymore -- at least, not its latest version. I won't be surprised if they remove the panel, icons, texts and everything after a few years ;)

agaida commented 5 years ago

Gnome has no default tray handling anymore - it is now a more or less (to be honest less) working plugin that provided a systray - The reason why it work in Ubuntu: They don't released with the latest Gnome exactly for that reason - damn, @tsujan was faster :sunglasses:

tsujan commented 5 years ago

A gnome-shell extension, probably. It'll break with the next Gnome update.

shlyakpavel commented 5 years ago

@agaida could you please answer my issue about Russian translation?

shlyakpavel commented 5 years ago
image

LXDE shows the same thing as gnome. It's a qps bug. It's the same on update period > 100ms. Just takes a bit more time to leak

tsujan commented 5 years ago

It's a qps bug.

I'm not sure at all. It can be caused by the Qt icon-engine used in GTK environments -- I don't know what it is.

I have Gnome on my other laptop; will test under it later.

ghost commented 5 years ago

I'm running Ubuntu MATE 19.04 using qps from master, and after running for a little over an hour it reports itself as using (only) 47MB, while mate-system-monitor reports it as using 110.6 MB.

Despite having 24GB of RAM, I was unable to open new programs till I closed a few open ones, which never happened before on this computer.

Additionally, I was unable to open new terminal sessions with mate-terminal until I killed qps, which locked up my computer for several minutes before dying.

The following lines appear in .xsesion-errors whe nI try to open mate-terminal:

** (mate-terminal:9467): WARNING **: 20:49:56.530: Unable to connect to dbus: GDBus.Error:org.freedesktop.DBus.Error.LimitsExceeded: Failed to determine seats of user "1000": Too many open files
Window manager warning: Log level 128: posix_spawn avoided (automatic reaping requested) (fd close requested) (child_setup specified) 

These lines show up many times during the time-span that qps was open, sometimes with a different process name, and sometimes without any process name.

tsujan commented 5 years ago

@plonibarploni If you open a bug report at LXQt, please avoid general and vague descriptions like the above one.

tsujan commented 5 years ago

@shlyakpavel I tested with gnome-shell 3.32.2+11 under Manajro (almost Arch) minutes ago: qps's memory usage stopped at 25 MiB when I gave it a 10-ms update interval. gnome-system-monitor showed the same number.

I'm afraid the cause of your problem should be somewhere else (I suspect the virtual machine).

EDIT: Who knows? The cause may be in older Gnome versions too.

shlyakpavel commented 5 years ago

I'm afraid the cause of your problem should be somewhere else (I suspect the virtual machine).

Then I'll try to install linux on my Mac natively. Seems to be challenging, tough

EDIT: Who knows? The cause may be in older Gnome versions too.

But lxde was also affected!

shlyakpavel commented 5 years ago

Screenshot from 2019-08-28 10-39-19 A very real screenshot on my very real machine showing a very real leak. And also showing that qps is not ready for hidpi. The issue is by no way related to VM

tsujan commented 5 years ago

The issue is by no way related to VM

Then, you should find to what it is related. As I said, it's not reproducible here, with 2 laptops and under 2 different DEs.

tsujan commented 5 years ago

And also showing that qps is not ready for hidpi

No it isn't because no one set Qt::AA_UseHighDpiPixmaps to true for it:

qps

As I mentioned elsewhere, the GUI needs rewriting; the widgets and icons are nonstandard.

tsujan commented 5 years ago

@shlyakpavel

To make sure that this is about Qt's pixmap cache and not a leak, I set:

QPixmapCache::setCacheLimit(1);

at the beginning of Qps c-tor (in qps.cpp). The memory usage didn't change from start (10 MiB).

You could test that in the DEs where you think there's a memory leak.

NOTE: The default cache limit is 10240 KB.

shlyakpavel commented 5 years ago

Please remove trayicon->setIcon(*pm); and setWindowIcon(*pm); from Qps::set_load_icon() and test again! That might make it easier to find the cause.

The leak has gone this way!

No, no and no. This doesn't help here on GNOME. But commenting out the whole set_load_icon() function makes the leak disappear P. S. Crazy things happening here. Leak comes and goes. Let me dig dipper

shlyakpavel commented 5 years ago

I'm now 100% sure that return QSystemTrayIcon::isSystemTrayAvailable(); in TrayIcon::hasSysTray() causes the leak on my system. If I replace it with "return true" or "return false" everything is fine. Perhaps we should call it once the program is started and then store the value instead of calling it on every refresh. Where to file a bug for it?)

tsujan commented 5 years ago

That may be a bug in the Qt plugins of those DEs (with the versions you have) because isSystemTrayAvailable shouldn't cause any memory leak but yes, there's no need to call hasSysTray continuously, especially when calling it once fixes the memory leak you see.

However, it's preferable to wait for systray at startup, in the way FeatherNotes does it -- as far as remember, I implemented it in LXQt notifications too.