How avoid memory leak with GUI programming

TheEnigmist · 11 Feb 2023, 18:24

Hello,
I always preferred programmatically set all gui elements in C++.
Now I wonder, since all tutorials/examples show all elements creation on heap (with new), how can I avoid any memory leak while create multiple times a widget (after deleting the older one)?
It is enough to set a parent on the object and when parent is destroyed it destroy all its child?
Sometimes I see that an element hasn't a parent setted, but placed in a layout. Again, when that layout is destroyed, it destroy all its elements?
What I found out is that some object that doesn't stay in a layout, like QGraphicEffect or QAbstractItemModel, if not set a parent even if use it correctly (setModel or setGraphicsEffect) after the widget is deleted, the widget doesn't delete it.

Chris Kawa · 2 Nov 2023, 18:25

It is enough to set a parent on the object and when parent is destroyed it destroy all its child?

Yes, parent QObjects delete their children. You can delete them earlier yourself, but if you don't they won't leak. The parent will clean up.

Sometimes I see that an element hasn't a parent setted, but placed in a layout. Again, when that layout is destroyed, it destroy all its elements?

A layout does not own a widget, so it won't delete it. But adding a widget to a layout changes that widget's parent to be the widget that the layout is set on. Sounds a bit convoluted so an example:

QLayout* layout = new QLayout(); //layout without a widget
QWidget* widget = new QWidget();
layout->addWidget(widget);
delete layout; // layout does not delete its elements. widget leaks

but

QWidget* parent = new QWidget();
QLayout* layout = new QLayout(parent); //parent now owns layout
QWidget* widget = new QWidget();
layout->addWidget(widget); //parent now owns widget too

delete parent; // deletes both the layout and the widget

What I found out is that some object that doesn't stay in a layout, like QGraphicEffect or QAbstractItemModel

Both effects and models can be set on multiple widgets or views, so these widgets and views don't take ownership of these objects and don't delete them. The documentation of methods that assign objects always explicitly states when the ownership is NOT transferred, for example QAbstarctItemView::setModel states:

"The view does not take ownership of the model unless it is the model's parent object because the model may be shared between many different views."

So in short:

objects are always deleted by their parents
layouts don't take ownership of their elements
layouts re-parent their elements to their widget
methods that assign potentially shared objects, like models, actions or effects, don't transfer ownership and this is always documented.

TheEnigmist · 11 Feb 2023, 22:00

@Chris-Kawa said in How avoid memory leak with GUI programming:

It is enough to set a parent on the object and when parent is destroyed it destroy all its child?

Yes, parent QObjects delete their children. You can delete them earlier yourself, but if you don't they won't leak. The parent will clean up.

Sometimes I see that an element hasn't a parent setted, but placed in a layout. Again, when that layout is destroyed, it destroy all its elements?

A layout does not own a widget, so it won't delete it. But adding a widget to a layout changes that widget's parent to be the widget that the layout is set on. Sounds a bit convoluted so an example:
QLayout* layout = new QLayout(); //layout without a widget
QWidget* widget = new QWidget();
layout->addWidget(widget);
delete layout; // layout does not delete its elements. widget leaks
but
QWidget* parent = new QWidget();
QLayout* layout = new QLayout(parent); //parent now owns layout
QWidget* widget = new QWidget();
layout->addWidget(widget); //parent now owns widget too

delete parent; // deletes both the layout and the widget
What I found out is that some object that doesn't stay in a layout, like QGraphicEffect or QAbstractItemModel

Both effects and models can be set on multiple widgets or views, so these widgets and views don't take ownership of these objects and don't delete them. The documentation of methods that assign objects always explicitly states when the ownership is NOT transferred, for example QAbstarctItemView::setModel states:

"The view does not take ownership of the model unless it is the model's parent object because the model may be shared between many different views."

So in short:

objects are always deleted by their parents

layouts don't take ownership of their elements

layouts re-parent their elements to their widget

methods that assign potentially shared objects, like models, actions or effects, don't transfer ownership and this is always documented.

Thx a lot, this is so clear.
Now I know how check my code 'couse I'm sure I left something on the road that can lead to a leak.
There is something else I should know and check to be leak-free?

Chris Kawa · wrote on 11 Feb 2023, 22:00

@TheEnigmist said:

There is something else I should know and check to be leak-free?

There's plenty of ways you can leak memory, whether it's Qt or not. There are entire books written on the topic.
In super general terms I would say follow best practices for the codebase you're in. Use RAII or smart pointers where appropriate, think of relationships when you design your software, keep track of ownership and lifetimes. In Qt specifically make sure everything is either parented or has a well defined scope of life. Don't create interfaces where it's unclear who owns the memory etc.
And very important - use tools! There's lots of static analyzers and memory leak detection tools and libraries for every popular toolchain. Build an infrastructure around your app where you can easily employ them to test large portions of your app i.e. dedicated build configurations or automation.

TheEnigmist · 7 Mar 2023, 23:45

@Chris-Kawa said in How avoid memory leak with GUI programming:

@TheEnigmist said:

There is something else I should know and check to be leak-free?

There's plenty of ways you can leak memory, whether it's Qt or not. There are entire books written on the topic.
In super general terms I would say follow best practices for the codebase you're in. Use RAII or smart pointers where appropriate, think of relationships when you design your software, keep track of ownership and lifetimes. In Qt specifically make sure everything is either parented or has a well defined scope of life. Don't create interfaces where it's unclear who owns the memory etc.
And very important - use tools! There's lots of static analyzers and memory leak detection tools and libraries for every popular toolchain. Build an infrastructure around your app where you can easily employ them to test large portions of your app i.e. dedicated build configurations or automation.

Hello, I don't know if is good to open a new thread but this is directly related to this thread.
I used Visual Studio built-in memory analyzer tool to find out what happen in my memory, I found some little mem leak for incorrect use of QStyle, but what I found out is that I create a really huge me leak with QPixmap.

There are a lot of mem leak with it, but actually I'm trying to solve this one:
I've a QAbstractTableModel and inside its data function I do this to show a little image on a cell:

switch (role)
	{
	case Qt::DisplayRole:
		return QString();
	case Qt::DecorationRole: {
		QPixmap p("file path");
		p = p.scaled(QSize(50, 50), Qt::KeepAspectRatio, Qt::FastTransformation);
		return QVariant(p);
	}
	default:
		break;
	}
	break;

VS points to p = p.scaled line that add every time 10k byte of data (50x50x4 bytes), and each time I create and detroy the widget where that QAbstractTableModel is I see in memory that all that Pixmap are never destroyed.
I thought that only 1 copy of that Pixmap lives in memory, instead they increase each time.
This problem is everywhere I use p.scaled, so solving this will help me to solve other pixmap related leaks

Chris Kawa · 8 Mar 2023, 09:08

@TheEnigmist a lot to talk about here, but first of all I doubt there's an actual memory leak. You said the pixmap is not destroyed, but have you actually put a breakpoint in its destructor to make verify that?

Another thing - Qt uses a pixmap cache. That is a mechanism that allows a reuse and speedup of loading of pixmap. Every time you create a pixmap from a file it is put in a cache, so the next time you create a pixmap with the same path no loading from the drive is actually done. You just get a pixmap that is already there. That's why might not see any meory usage decrease when a pixmap is deleted. It's just the pixmap object that is deleted but the actual data still persists in the cache. This happens up to a certain size. By default there's 10MB reserved for that cache. If you exceed that the new pixmaps are not cached. You can control the size of the cache with QPixampCache::setCacheLimit.

Another thing is the data() function. It's a function that can be called hundreds or even thousands time a second in some situations, for example when you scroll your view or resize the widget. This function should be optimized to the full of your ability. It should do no allocations unless absolutely necessary. It should do no calculations and it should definitely avoid file I/O. If you have to calculate something put it in a cache and reuse. Definitely don't load pixmaps from disk and scale them every time it is called. It's a very bad performance killer. You should load the pixmap once, scale it and cache it (see QCache class that you can use for that). The data() function should mostly be just switch over the data role and a return of a cached value.

Kent-Dorfman · 8 Mar 2023, 04:47

Where is Pixmap p allocated? on the stack?

then you return the stack allocated object?

In pure C++, unless Pixmap has a copy constructor, this would be bad ju-ju.

I would create the pixmap on the heap and return its address.

For safety sake I'd also be concerned about the assigment of a p.method() output to p itself.

I don't know what trickery QVariant does to p.

understanding of course, that I'm not reviewing the API right now to see what is safe. Just erring on the side of caution.

jeremy_k · 3 Aug 2023, 05:24

@Kent-Dorfman said in How avoid memory leak with GUI programming:

Where is Pixmap p allocated? on the stack?

then you return the stack allocated object?

That's fine. Explicitly creating a QVariant from it isn't necessary. QPixmap::operator QVariant() will perform the conversion implicitly.

In pure C++, unless Pixmap has a copy constructor, this would be bad ju-ju.

It has a copy constructor, and a move operator. It's also one of the implicitly shared data classes.

I would create the pixmap on the heap and return its address.

When would you destroy the heap allocated pixmap? QAbstractItemModel::data() isn't returning a smart pointer, and the framework provided views aren't expecting one.

I don't know what trickery QVariant does to p.

That makes it hard to reason about reasonable use of the class. Pulling from the documentation:

A QVariant object holds a single value of a single type() at a time

If type is T rather than T *, such as in this case, QVariant will maintain the lifetime as you would expect. That includes return value optimization.

TheEnigmist · 8 Mar 2023, 17:08

@Chris-Kawa said in How avoid memory leak with GUI programming:

@TheEnigmist a lot to talk about here, but first of all I doubt there's an actual memory leak. You said the pixmap is not destroyed, but have you actually put a breakpoint in its destructor to make verify that?

Another thing - Qt uses a pixmap cache. That is a mechanism that allows a reuse and speedup of loading of pixmap. Every time you create a pixmap from a file it is put in a cache, so the next time you create a pixmap with the same path no loading from the drive is actually done. You just get a pixmap that is already there. That's why might not see any meory usage decrease when a pixmap is deleted. It's just the pixmap object that is deleted but the actual data still persists in the cache. This happens up to a certain size. By default there's 10MB reserved for that cache. If you exceed that the new pixmaps are not cached. You can control the size of the cache with QPixampCache::setCacheLimit.

Another thing is the data() function. It's a function that can be called hundreds or even thousands time a second in some situations, for example when you scroll your view or resize the widget. This function should be optimized to the full of your ability. It should do no allocations unless absolutely necessary. It should do no calculations and it should definitely avoid file I/O. If you have to calculate something put it in a cache and reuse. Definitely don't load pixmaps from disk and scale them every time it is called. It's a very bad performance killer. You should load the pixmap once, scale it and cache it (see QCache class that you can use for that). The data() function should mostly be just switch over the data role and a return of a cached value.

I didn't test the limit of QPixmapCache, I only checked allocated memory on several snapshot after recreating the same widget more and more and always that 10.000 bytes vars are there and increase in number. I will test it putting a limit on cache and see. So if I'm not wrong what is cached is not the file on disk, but its scaled version, so each time it get called it creates a new cached pixmap since scaled creates for the system a new pixmap.
About data() function now is more clear and I try to optimize it, I need to create the scaled pixmap outside of that function , maybe cache it only once, and then only return it without any transformation on it. Thanks for the info

I'm focused on pixmap due to a lot of new "unresolved allocation" and wanted to decrease it.

If I create more snapshots I see that, for example, 0x1E219491360 will be always there, but as you said that is ok due to pixmap caching system.

Christian Ehrlicher · 8 Mar 2023, 19:39

@TheEnigmist said in How avoid memory leak with GUI programming:

VS points to p = p.scaled line that add every time 10k byte of data (50x50x4 bytes), and each time I create and detroy the widget where that QAbstractTableModel is I see in memory that all that Pixmap are never destroyed.

There is no leak and can't be one - you should take a look into QPixmapCache and for the sake of performance and implicit sharing don't load the pixmap in your data() method every time.

Chris Kawa · wrote on 8 Mar 2023, 17:08

@TheEnigmist said in How avoid memory leak with GUI programming:

So if I'm not wrong what is cached is not the file on disk, but its scaled version

No. How would Qt know that it's the same pixmap? What's cached is the pixmap that you create from a file, and the path (with some additional info) becomes a key to look it up in the cache.

TheEnigmist · wrote on 8 Mar 2023, 19:39

@Christian-Ehrlicher said in How avoid memory leak with GUI programming:

@TheEnigmist said in How avoid memory leak with GUI programming:

VS points to p = p.scaled line that add every time 10k byte of data (50x50x4 bytes), and each time I create and detroy the widget where that QAbstractTableModel is I see in memory that all that Pixmap are never destroyed.

There is no leak and can't be one - you should take a look into QPixmapCache and for the sake of performance and implicit sharing don't load the pixmap in your data() method every time.

Yeah I just found out that I was really wrong with that :( I will fix it right now!
So I can go over when see pixmap in my allocated memory due to QPixmapCache. Ofc is always better to improve my code and don't load so many times a pixmap!

@Chris-Kawa said in How avoid memory leak with GUI programming:

@TheEnigmist said in How avoid memory leak with GUI programming:

So if I'm not wrong what is cached is not the file on disk, but its scaled version

No. How would Qt know that it's the same pixmap? What's cached is the pixmap that you create from a file, and the path (with some additional info) becomes a key to look it up in the cache.

I see, but why the debugger points me to the scaled() function when looking for the allocated memory istance?