Skip to content
This repository has been archived by the owner on Feb 19, 2021. It is now read-only.

Memory leak of Settings dialogue #51

Open
SilverBut opened this issue Sep 6, 2018 · 9 comments
Open

Memory leak of Settings dialogue #51

SilverBut opened this issue Sep 6, 2018 · 9 comments
Labels
bug Something isn't working
Milestone

Comments

@SilverBut
Copy link
Contributor

In both current master branch and my issue-guis branch, the SettingsDialog is created for everytime you click Settings in menu. However, it is not destroyed after the dialogue got closed. This can be reproduced by adding a singleton class into the __init__ method of class SetingsDialog.

Fix related with this issue will be committed together with other code from #46.

@SilverBut
Copy link
Contributor Author

Seems a possible solution: https://stackoverflow.com/questions/15674422/unable-to-detect-why-qdialog-is-memory-leaking

@SilverBut
Copy link
Contributor Author

Seems a possible solution: https://stackoverflow.com/questions/15674422/unable-to-detect-why-qdialog-is-memory-leaking

Nope. Not the source. But fixed:

         def settingsActionTriggered():
-            SettingsDialog(self._plugin).exec_()
+            if self._settingsDialog == None:
+                self._settingsDialog = SettingsDialog(self._plugin)
+            self._settingsDialog.exec_()

@NeatMonster
Copy link
Member

NeatMonster commented Sep 6, 2018

How did you confirm the memory leak? I read the documentation of QDialog::done and it appears to me that setting the Qt::WA_DeleteOnClose flag would be enough. Am I missing something?

@NeatMonster NeatMonster added the bug Something isn't working label Sep 6, 2018
SilverBut added a commit to SilverBut/IDArling that referenced this issue Sep 6, 2018
@SilverBut
Copy link
Contributor Author

How did you confirm the memory leak? I read the documentation of QDialog::done and it appears to me that setting the Qt::WA_DeleteOnClose flag would be enough. Am I missing something?

Open & close the settings menu using a script and monitor the memory usage ;)

@NeatMonster
Copy link
Member

Sorry for the late reply @SilverBut. I've tested your pull request #52 but I'm still seeing a memory leak. Even if the dialog is reused, memory usage increases every time I open and close it. In all of my testing, the singleton method appears to be equivalent to using Qt::WA_DeleteOnClose.

@SilverBut
Copy link
Contributor Author

Strange. The commit 2714069 should not create any more setting dialogues and its pages. Maybe it is not the dialog which is created for many times.
I may check it later. Kind of busy recently.

@NyaMisty
Copy link
Contributor

I'm not mean to be offensive, but I wonder why we need to care about things like this?
IDArling and IDA aren't used in a large-scale or long-time running scenario, and memory leak for about several KB or MB or even ~10MB each time opening the settings window is quite acceptable, as no one will repeatedly open and close it.
I think we should focus on other main issues instead of this tiny mysterious memory leak

@patateqbool
Copy link
Member

patateqbool commented Sep 12, 2018

totally agree, just keep it in a corner of our head for when there will be no more issue :)).

@NyaMisty
Copy link
Contributor

But I also suggest that we solve this issue when we already have a pre-release

@patateqbool patateqbool added this to the 0.3 milestone Sep 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants