Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FlatMenu memory leak #2372

Open
arjones6 opened this issue Apr 11, 2023 · 7 comments · May be fixed by #2373
Open

FlatMenu memory leak #2372

arjones6 opened this issue Apr 11, 2023 · 7 comments · May be fixed by #2373

Comments

@arjones6
Copy link
Contributor

Operating system: Windows 10
wxPython version & source: self-built
Python version & source: 3.9.6 stock

Description of the problem:

After a FlatMenu object no longer is referenced in the code, it's memory is not cleaned up, I assume because of cyclical references between the FlatMenu and FlatMenuItems. Even if you explicity Destroy and delete the menu, it does not free all the memory.

Separately, a wx.BitMap object is created for the checkbox and radiobox images for each FlatMenuItem that is added. On windows, after a large number of items are created the bitmap creation fails and an exception is thrown.

Traceback (most recent call last):
File "C:\Work\autolab\test\scratch.py", line 15, in
fm.Append(-1, 'item {}'.format(i), kind=wx.ITEM_CHECK)
File "C:\NER\venv\autolab\lib\site-packages\wx\lib\agw\flatmenu.py", line 5476, in Append
newItem = FlatMenuItem(self, id, item, helpString, kind)
File "C:\NER\venv\autolab\lib\site-packages\wx\lib\agw\flatmenu.py", line 4865, in init
self._radioMarkBmp.SetMask(wx.Mask(self._radioMarkBmp, wx.WHITE))
wx._core.wxAssertionError: C++ assertion ""bitmap.IsOk()"" failed at ....\src\msw\bitmap.cpp(1611) in wxMask::Create(): invalid bitmap in wxMask::Create

Since the menu items aren't cleaned up even if the parent menu is destroyed, if a program is dynamically creating context menus with many items, it can hit that limit.

Code Example (click to expand)
import wx
import wx.lib.agw.flatmenu as FM
import tracemalloc
app = wx.App()

f = wx.Frame(None)

tracemalloc.start()
current = tracemalloc.get_traced_memory()[0]
for i in range(300):
    fm = FM.FlatMenu(f)
    for i in range(200):
        fm.Append(-1, 'item {}'.format(i), kind=wx.ITEM_CHECK)
    fm.Destroy()
    del fm
    new = tracemalloc.get_traced_memory()[0]
    print("Current", new, "Added", new - current)
    current = new

tracemalloc.stop()
@arjones6 arjones6 linked a pull request Apr 11, 2023 that will close this issue
@Metallicow
Copy link
Contributor

If you think there is a visual memory error @kdschlosser might be able to help you... maybe not.
If he cannot further help you you might ask @infinity77 , but he might be retired....
Unless either one can give a clue I have to be stuck inbetween for now.

@kdschlosser
Copy link
Contributor

kdschlosser commented Apr 28, 2023

you are not forcing a garbage collection. Python is only going to garbage collect when one of these things happens

A: it has time to
B: when you are running out of memory.

You can force a garbage garbage collection by using the following code.

import gc

gc.collect()

@Metallicow
Copy link
Contributor

Metallicow commented May 1, 2023 via email

@Metallicow
Copy link
Contributor

Metallicow commented May 1, 2023 via email

@kdschlosser
Copy link
Contributor

I just ran the script that was provided on Python 3.8

Here is the output

Current 358180 Added 358180
Current 660800 Added 302620
Current 996310 Added 335510
Current 460906 Added -535404
Current 727957 Added 267051
Current 999749 Added 271792
Current 1269499 Added 269750
Current 1539249 Added 269750
Current 1940231 Added 400982
Current 861287 Added -1078944
Current 1131037 Added 269750
Current 1400787 Added 269750
Current 1670537 Added 269750
Current 1940287 Added 269750
Current 1131037 Added -809250
Current 1400787 Added 269750
Current 1670537 Added 269750
Current 1940287 Added 269750
Current 2210037 Added 269750
Current 1402499 Added -807538
Current 1670649 Added 268150
Current 1940399 Added 269750
Current 2210149 Added 269750
Current 2479899 Added 269750
Current 1675817 Added -804082
Current 1940511 Added 264694
Current 2210261 Added 269750
Current 2480011 Added 269750

I see garbage collections happening. no issue there.

Python 3.10

Current 362001 Added 362001
Current 664541 Added 302540
Current 1000113 Added 335572
Current 460693 Added -539420
Current 731896 Added 271203
Current 1006577 Added 274681
Current 1276319 Added 269742
Current 1546061 Added 269742
Current 868067 Added -677994
Current 1137809 Added 269742
Current 1407551 Added 269742
Current 1677293 Added 269742
Current 1947035 Added 269742
Current 1140689 Added -806346
Current 1407551 Added 266862
Current 1677293 Added 269742

same deal. No issues with a memory leak. has got to be something specific with the environment the script is running in. OS related problem possibly. I am running Windows 11.

@Metallicow
Copy link
Contributor

Metallicow commented May 2, 2023 via email

@arjones6
Copy link
Contributor Author

arjones6 commented May 2, 2023

You're right. Garbage collection is happening, so memory leak was the wrong terminology. But since garbage collection is resource dependent you can't rely on it happening when the system is busy. And yes, you can force it to happen which does fix the issue, but expecting a user to explicitly call for garbage collection after explicitly calling Destroy on an object is a bit much. In my mind calling Destroy on a wx object should destroy that object and all its children.

My bigger issue was the eventual exception that gets thrown. That is platform specific as I've been running this code on linux with no problem. So I see two issues with how the code is currently written.

  1. We're allocating a separate bitmap for each menu item unnecessarily. It is the same image for all of them, so why not allocate one image use that image for whatever menu items need it.

  2. If I explicitly call Destroy on an object it should clean itself and any of its children up. I would expect this to happen when I call Destroy and not at some indeterminant time in the future.

The PR I submitted (#2373) fixes both those issues. It makes the check mark bitmaps class variables for FlatMenuItem instead of instance variables so it isn't unnecessarily creating separate bitmaps for each. And it adds a Destroy method to FlatMenu that calls Clear on the menu to destroy the menu items when the menu is destroyed.

I ran into this because the gui program I had that has to dynamically create relatively large context menus would randomly crash on windows sometimes if the user opened the menu multiple times in close succession.

Output before:

Current 359618 Added 359618
Current 662270 Added 302652
Current 998996 Added 336726
Current 1268850 Added 269854
Current 1538925 Added 270075
Current 1942053 Added 403128
Current 593095 Added -1348958
Current 863069 Added 269974
Current 1132923 Added 269854
Current 1402777 Added 269854
Current 1672631 Added 269854
Current 865757 Added -806874
Current 1133259 Added 267502
Current 1403001 Added 269742
Current 1672743 Added 269742
Current 1942485 Added 269742
Current 2212339 Added 269854
Current 1133483 Added -1078856
Current 1403225 Added 269742
Current 1672967 Added 269742
Current 1942709 Added 269742
Current 2212451 Added 269742
Current 1403225 Added -809226
Current 1672967 Added 269742
Current 1942709 Added 269742
Current 2212451 Added 269742
Current 2482193 Added 269742
Current 1673079 Added -809114
Current 1942821 Added 269742
Current 2212563 Added 269742
Current 2482305 Added 269742
Current 2752047 Added 269742
Current 1946133 Added -805914
Current 2212675 Added 266542
Current 2482417 Added 269742
Current 2752159 Added 269742
Current 3021901 Added 269742
Current 3553515 Added 531614
Current 2474659 Added -1078856
Current 2744401 Added 269742
Current 3014143 Added 269742
Current 3283885 Added 269742
Current 3553627 Added 269742
Current 2744401 Added -809226
Current 3014143 Added 269742
Current 3283885 Added 269742
Current 3553627 Added 269742
Current 3823369 Added 269742
Traceback (most recent call last):
  File "C:\Work\autolab\test\scratch.py", line 15, in <module>
    fm.Append(-1, 'item {}'.format(i), kind=wx.ITEM_CHECK)
  File "C:\NER\venv\autolab\lib\site-packages\wx\lib\agw\flatmenu.py", line 5476, in Append
    newItem = FlatMenuItem(self, id, item, helpString, kind)
  File "C:\NER\venv\autolab\lib\site-packages\wx\lib\agw\flatmenu.py", line 4865, in __init__
    self._radioMarkBmp.SetMask(wx.Mask(self._radioMarkBmp, wx.WHITE))
wx._core.wxAssertionError: C++ assertion ""bitmap.IsOk()"" failed at ..\..\src\msw\bitmap.cpp(1611) in wxMask::Create(): invalid bitmap in wxMask::Create

Output after:

Current 99092 Added 99092
Current 100266 Added 1174
Current 100266 Added 0
Current 100266 Added 0
Current 100487 Added 221
Current 103158 Added 2671
Current 103158 Added 0
Current 103406 Added 248
Current 103526 Added 120
Current 103526 Added 0
Current 103526 Added 0
Current 103526 Added 0
Current 103526 Added 0
Current 103526 Added 0
Current 103526 Added 0
Current 103526 Added 0
etc...

The latter output is more memory efficient and more in line with what I would think a user would expect.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants