QAbstractListModel memory leak

[whatisaphone]

I ran the modelview_color example under valgrind, and it's leaking QVariant objects. Trimmed output:

==2624937== 128,000 bytes in 4,000 blocks are definitely lost in loss record 302 of 302
==2624937==    at 0x4849F93: operator new(unsigned long) (vg_replace_malloc.c:487)
==2624937==    by 0x1753B2D: QVariant_new (libqvariant.cpp:41)
==2624937==    by 0x16F5A66: libqvariant.QVariant.New (libqvariant.zig:48)
==2624937==    by 0x16F563D: main.onData (main.zig:66)

You can observe memory usage climbing if you keep scrolling the list up and down.

It's leaking both the color and string objects. I'm not sure the right way to free them. Any ideas?

[rcalixte]

Good catch! My initial thoughts are that we could capture the objects into an array or slice so that they could be freed but even then, I would want to test to see if we could free while the object is in use. Another option would be to pre-create each object (since they're static in this case) and pass those values rather than allocating new instances for each loop. I can add a patch in a few minutes with the latter idea if you want to give it a test?

The patch file: mvc.patch

When applied to the example code, it looks clean on my end and I can run it through Valgrind later as well if you don't have the time. The patch moves the allocations out of the callback and into the main function. This actually has two benefits: one, we're not allocating at each turn but two, we can control the cleanup of the objects.

If possible, this might be worth documenting because in this case, Qt is copying the returned value even though we are allocating it which means there is no owner of the memory, explaining the leak. This is also why we can reuse the same memory for the variant_new that represents the empty new type of QVariant. All in all, this was a fascinating exercise. And now I need to apply the same patch to the C example and check for any other callbacks that might be leaking the same way. 😅

[whatisaphone]

That sounds like it would just be hiding the root issue, to be honest. The data in a real app isn't static, so it wouldn't work in a real app.

Looking at the docs, it doesn't look like C++ needs any workarounds to free those objects. Qt must free them internally after they're used, which is what I would expect to happen after you pass the object to Qt. So I wonder why they're leaking using the bindings.

[whatisaphone]

Sorry, I replied before your edit. That does look like it should fix it! But it will be a little unwieldy managing those objects for a dynamic model.

I wish I understood why the corresponding C++ doesn't have the same problem. Understanding that might lead to a cleaner solution.

[rcalixte]

No need to apologize! And you're right because in typical Qt usage, QVariant is stack-allocated. The extra care we need to take isn't even because of the bindings or the heap allocations but because we've opted in to manual memory management. 😄 This isn't an issue for the bindings for Go because of the garbage collector. Where Qt can have stack-allocated types, all of ours will be heap-allocated and require manual cleanup. The same goes for the QColor objects in the example as they do for the QVariant objects. Based on the results, we could also have a patch with a single global QVariant placeholder and keep most of the original code as it was but replacing the return statements with the capture, Delete, and SetValue sequence. I think that makes it manageable but there's still a lot of experimentation to be had.

I'm taking a look through the other examples now but I don't think this type of memory semantics has a large blast radius even though it is a significant one from the standpoint of data models. I also consider the example to be somewhat simple but if all that is needed is a placeholder QVariant object so that all of the allocations in the callback can be freed, I think it's manageable overhead. I wish we could do without it but at the same time, I can't say that I even notice a performance difference between the before and after even though the latter is performing significantly less allocations. More stringent benchmarking would probably reveal the difference that we know is there.

Regarding documentation, I'm thinking that the best we can do is add a blurb that explains this as best we can. Ultimately, it's up to Qt's API so care must be had when it comes to managing allocations... but that's the same as it has always been for us. The key with this specific callback is that it is stack-allocated and copying the returned object by value not by reference.

[rcalixte]

All in all, this was a fun exercise and a good lesson for me too! I wish I could say I was an expert at consuming the library by now but it's been a crash course in a lot of ways and it still continues to provide lessons like these. That's part of the reason I keep finding the energy to work on it!

[whatisaphone]

Ah right, I forgot these wrappers were going on the heap, despite the obvious operator new in the stack trace. My bad 😅 I understand what's going now, thanks!

[rcalixte]

I've updated the code in the examples to at least patch the leaks (as detected by Valgrind) from the code we can control. There are still some leaks in Qt, notably around dialogs. For better or worse, the modelview_color example was the worst, just due to the multiplicative nature of where the leaks were located. Thanks again!

[rcalixte]

Hi! I just pushed some changes today that handle the memory leaks in this situation and I've updated the modelview_color example to match. It's a trade-off but one that hopefully makes the developer ergonomics a little better in the long run.