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

QItemSelectionModel leak? #7947

Open
toofar opened this issue Sep 30, 2023 · 2 comments · May be fixed by #7950
Open

QItemSelectionModel leak? #7947

toofar opened this issue Sep 30, 2023 · 2 comments · May be fixed by #7950

Comments

@toofar
Copy link
Member

toofar commented Sep 30, 2023

Version info:

Latest git main: 7750a2f
Qt 6.6.0 (local build)

Does the bug happen if you start with --temp-basedir?:
yep!

Description
My main instance has been running for a while and has been accumulating ram. So I figured I would point https://github.com/KDAB/GammaRay/ at it and see if I could spot where the memory was hiding. I think there was too much stuff in the application at that point and it made gamma ray painfully slow, but I did spot one thing. Lots and lots of QItemSelectionModels in the Objects tab.

Questions I don't have time to look into right now:

  1. where are these models coming from?
  2. how much memory do they use?
  3. why aren't they going away?

How to reproduce

  1. Attach gamma ray
  2. open completion (I was using the :open completion)
  3. close completion
  4. repeat 2 & 3 many times
  5. watch objects accumulate

Here is a professional and scientific video recording of me reproducing it:

QItemSelectionModelLeak_maybe-2023-09-30_22.19.30.mp4

The c++ stack trace in gamma ray (from when the object was created I presume) says:

QItemSelectionModel::QItemSelectionModel(QAbstractItemModel*, QObject*)
QAbstractItemView::setModel(QAbstractItemModel*)
QTreeView::setModel(QAbstractItemModel*)
@toofar
Copy link
Member Author

toofar commented Oct 1, 2023

Here's what I've got so far. A bit stuck with understanding why the objects under the header aren't being cleaned up. I'm supposed to be looking at other stuff so if anyone else wants to pick it up that would be great. nvm, got distracted by this

What I've discovered so far:

  • Two new QItemSelectionModel objects which are not cleaned up are created every time we close the completion. One is a child of our CompletionView, one is a child of a QHeaderView which is a child of our CompletionView.
  • QAbstractItemView::setModel() will create new QItemSelectionModel instances every time it is called, even if you are setting the model to None
  • The docs say it doesn't manage the QItemSelectionModel lifecycle itself and callers need to take care of cleaning them up, and give an example of the order of calls to do this. It does also connect model.destroyed to selection_model.deleteLater(), so I guess that's why they aren't getting deleted properly when setting the model to None.
  • We don't currently clean up the selection model if we are setting a new model and the old one was None, aha: https://github.com/qutebrowser/qutebrowser/blob/0718b25/qutebrowser/completion/completionwidget.py#L367
  • That takes care of one of the two instances, but the other one is still accumulating. The one under a QHeaderView object
    • The call stack for this is

      QTreeView::setModel(QAbstractItemModel*)
      QHeaderView::setModel(QAbstractItemModel*)
      QAbstractItemView::setModel(QAbstractItemModel*)
      QItemSelectionModel::QItemSelectionModel(QAbstractItemModel*, QObject*)

    • QHeaderView also implements QAbstractItemModel and QTreeView calls setModel() on its header and then itself.

    • We should be able to clean it up the same way as with the main view, save a reference to it before setting the mode, deleteLater() afterwards.

    • That's not working for me though. Not sure what I'm doing wrong. I suggest the next step is to make a c++ reproducer to debug with as it's hard to compare object IDs between python and c++.

    • Okay, looks like we are cleaning up some models under the header correctly, but there seems to be an extra one being created at some other part of the lifecycle that we aren't hooking into yet.

    • Next step: drop into gdb with a breakpoint in QItemSelectionModel::QItemSelectionModel() and look at the backtrace of where they are all being created

Here's a patch which resolves the leaks. Still go a dirty workaround in it to clean up the spurious models under the header though:

diff --git i/qutebrowser/completion/completionwidget.py w/qutebrowser/completion/completionwidget.py
index f042be0a1c3f..0eef01dc6dae 100644
--- i/qutebrowser/completion/completionwidget.py
+++ w/qutebrowser/completion/completionwidget.py
@@ -143,13 +143,14 @@ class CompletionView(QTreeView):
         assert isinstance(model, completionmodel.CompletionModel), model
         return model
 
-    def _selection_model(self) -> QItemSelectionModel:
+    def _selection_model(self, allow_none=False) -> QItemSelectionModel:
         """Get the current selection model.
 
         Ensures the model is not None.
         """
         model = self.selectionModel()
-        assert model is not None
+        if model is None and not allow_none:
+            assert model is not None
         return model
 
     @pyqtSlot(str)
@@ -362,12 +363,46 @@ class CompletionView(QTreeView):
             model: The model to use.
         """
         old_model = self.model()
-        if old_model is not None and model is not old_model:
-            old_model.deleteLater()
-            self._selection_model().deleteLater()
+        old_selection_model = self._selection_model(allow_none=True)
+        old_header_selection_model = self.header().selectionModel()
+
+        # Does this situation actually happen?
+        if old_model is model:
+            return
 
         self.setModel(model)
 
+        if old_model:
+            old_model.deleteLater()
+        # Cleaning up these selection models ourselves is probably only
+        # necessary if old_model is None. But it can't hurt. The
+        # QAbstractItemView docs explicitly say it doesn't handle cleaning
+        # them up. But in practise it seems like they get cleaned up after the
+        # model does.
+        if old_selection_model:
+            old_selection_model.deleteLater()
+        if old_header_selection_model:
+            old_header_selection_model.deleteLater()
+
+
+            # There seems to be a spurious selection model being created as a
+            # child of the header. When looking at all of the
+            # QItemSelectionModel children of the header we can see one which
+            # we've never seen when querying header().selectionModel() before.
+            # This is a workaround to clean those spurious models up until we
+            # understand how they are being created.
+            from qutebrowser.qt import sip
+            current_header_selection_model = self.header().selectionModel()
+            header_children = self.header().findChildren(QItemSelectionModel)
+            print("")
+            print(f"## {'closed' if model is None else 'opened'} ###")
+            print(f"old_model=0x{sip.unwrapinstance(old_header_selection_model):x}")
+            print(f"current_model=0x{sip.unwrapinstance(current_header_selection_model):x}")
+            for item in header_children:
+                if item != current_header_selection_model:
+                    print(f"deleting=0x{sip.unwrapinstance(item):x}")
+                    item.deleteLater()
+
         if model is None:
             self._active = False
             self.hide()

toofar added a commit that referenced this issue Oct 1, 2023
[Gammaray][] showed that over time we accumulate QItemSelectionModels.
It turns out that even though they are created by the Qt classes we
inherit from, those classes don't take care of deleting them itself. So
we have to manage their lifecycles ourselves.

For the CompletionView that's easy enough, a new QItemSelectionModels is
always created by `setModel()`. The QAbstractItemView docs say to keep a
reference to the old model, call `setModel()`, then delete the old one.
So that's easy enough.

There is a QHeaderView which is a child of the CompletionView though
(via QTreeView) which is a bit problematic. When we call `QTreeView.setModel()`
that calls `header.setModel()`, which creates a new selection model in
the header, but then it calls `header.setSelectionModel()`. Which
doesn't clean up the short lived selection model created by
`header.setModel()`. And there is no reference to it saved anywhere.

So we have to go hunting for it by looking at child objects. It's a bit
yuck because we have to make several assumptions about the Qt behaviour
that I can't think of many ways to verify. I wish there was some other
way to very that the child items aren't being used, but all the do when
disconnected is call `self.clear()`, which just clears the selection.

Hopefully the fact that `header.selectionModel()` returns a different
one is reassuring enough that the child objects are fine to delete.

I moved everything into a context manager simply to avoid adding the huge
block of text into the calling function. It's still 100% coupled to the
calling function.

TODO:
* fix tests?
* raise bug ticket with Qt?

Fixes: #7947

[Gammaray]: https://github.com/KDAB/GammaRay/
toofar added a commit that referenced this issue Oct 1, 2023
[Gammaray][] showed that over time we accumulate QItemSelectionModels.
It turns out that even though they are created by the Qt classes we
inherit from, those classes don't take care of deleting them itself. So
we have to manage their lifecycles ourselves.

For the CompletionView that's easy enough, a new QItemSelectionModels is
always created by `setModel()`. The QAbstractItemView docs say to keep a
reference to the old model, call `setModel()`, then delete the old one.
So that's easy enough.

There is a QHeaderView which is a child of the CompletionView though
(via QTreeView) which is a bit problematic. When we call `QTreeView.setModel()`
that calls `header.setModel()`, which creates a new selection model in
the header, but then it calls `header.setSelectionModel()`. Which
doesn't clean up the short lived selection model created by
`header.setModel()`. And there is no reference to it saved anywhere.

So we have to go hunting for it by looking at child objects. It's a bit
yuck because we have to make several assumptions about the Qt behaviour
that I can't think of many ways to verify. I wish there was some other
way to very that the child items aren't being used, but all the do when
disconnected is call `self.clear()`, which just clears the selection.

Hopefully the fact that `header.selectionModel()` returns a different
one is reassuring enough that the child objects are fine to delete.

I moved everything into a context manager simply to avoid adding the huge
block of text into the calling function. It's still 100% coupled to the
calling function.

TODO:
* fix tests?
* raise bug ticket with Qt?

Fixes: #7947

[Gammaray]: https://github.com/KDAB/GammaRay/
toofar added a commit that referenced this issue Oct 3, 2023
[Gammaray][] showed that over time we accumulate QItemSelectionModels.
It turns out that even though they are created by the Qt classes we
inherit from, those classes don't take care of deleting them itself. So
we have to manage their lifecycles ourselves.

For the CompletionView that's easy enough, a new QItemSelectionModels is
always created by `setModel()`. The QAbstractItemView docs say to keep a
reference to the old model, call `setModel()`, then delete the old one.
So that's easy enough.

There is a QHeaderView which is a child of the CompletionView though
(via QTreeView) which is a bit problematic. When we call `QTreeView.setModel()`
that calls `header.setModel()`, which creates a new selection model in
the header, but then it calls `header.setSelectionModel()`. Which
doesn't clean up the short lived selection model created by
`header.setModel()`. And there is no reference to it saved anywhere.

So we have to go hunting for it by looking at child objects. It's a bit
yuck because we have to make several assumptions about the Qt behaviour
that I can't think of many ways to verify. I wish there was some other
way to very that the child items aren't being used, but all the do when
disconnected is call `self.clear()`, which just clears the selection.

Hopefully the fact that `header.selectionModel()` returns a different
one is reassuring enough that the child objects are fine to delete.

I moved everything into a context manager simply to avoid adding the huge
block of text into the calling function. It's still 100% coupled to the
calling function.

TODO:
* more tests?
* raise bug ticket with Qt?

Fixes: #7947

[Gammaray]: https://github.com/KDAB/GammaRay/
@toofar toofar linked a pull request Oct 3, 2023 that will close this issue
@toofar
Copy link
Member Author

toofar commented Oct 3, 2023

Trying to look at memory usage. First, how many of these objects do I have live in main running instance:

# run in :debug-console
from qutebrowser.qt.core import QItemSelectionModel, Qt
from qutebrowser.misc import objects
from qutebrowser.mainwindow.mainwindow import MainWindow
windows = [obj for obj in objects.qapp.allWidgets() if isinstance(obj, MainWindow)]
sum([len(window.findChildren(QItemSelectionModel, options=Qt.FindChildOption.FindChildrenRecursively)) for window in windows])

Gives me 1665 objects.

Here's a simple c++ application to allocate objects. I have no idea how realistic these instantiations are, should I be passing them more arguments?

// g++ -g -o main main.cpp `pkgconf --libs --cflags Qt6Core` -fPIC
// rm massif.out.* ; valgrind --tool=massif --time-unit=B ./main && ms_print massif.out.*
#include <iostream>
#include <QItemSelection>

int main(int argc, char *argv[])
{
    std::list<QItemSelectionModel*> references;
    int count=100;
    if (argc == 2) {
      count = std::stoi(argv[1]);
    }
    for (int i = 0; i < count; i++) {
      references.push_back(new QItemSelectionModel());
    }
    return 0;
}

And running that with massif and looking at the peak of the allocation top allocation graph with a variety of counts:
100=127KB
1000=471KB
2000=854KB
10000=3.8MB
1000000=373MB

That doesn't look like a huge amount of memory.

toofar added a commit that referenced this issue Jun 3, 2024
To avoid a leak when calling `QTreeView.setModel(None)`, this commit switches
to relying on the `model.destroyed` signal to make sure related state is
cleaned up. Upstream bug: https://bugreports.qt.io/browse/QTBUG-49966

When you call `setModel(None)` on a QTreeView it causes a small memory leak
of `QItemSelectionModel` objects created by the QTreeView's child QHeaderView
object.

`QAbstractItemView` will create a new `QItemSelectionModel` whenever a model
is set, even if that model is `None`. When the new model is non-null the
new selection model will be set to be deleted when the new model is, but when
the new model is None the new selection model will be linked to the
static `QAbstractItemModelPrivate::staticEmptyModel()`. Since that empty model
lives forever, so do the related section models, unless callers take care to
clean them up themselves.

Both `QTreeView` and it's child `QHeaderView` implement `QAbstractItemView`
and have this behaviour. For the `QTreeView` we were making sure to delete
the old selection model ourselves (as of fe1215c). But for the one
created by the `QHeaderView` we can't get a reference it because
`QTreeView.setModel()` would call `QHeaderView.setModel()` and then
`QHeaderView.setSelectionModel()` right away to assign it's own
selection model to the child, leaving no references to the selection
model created by `QHeaderView.setModel()`.

I was previously using `header.findChildren(QItemSelectionModel)` to clean up
old orphaned selection models, but this approach is a lot simpler!

To observe this for yourself you can plonk something like this in
`set_model()` before the early return and switch between the old and new
implementation and see how it changes behaviour.

        header = self.header()
        header_children = header.findChildren(QItemSelectionModel)
        our_children = self.findChildren(QItemSelectionModel)
        print(f"{len(our_children)=} {len(header_children)=}")

You can also observer the selection models accumulating in Gammaray
(https://github.com/KDAB/GammaRay/) if you just open and close the selection a
lot and then filter the object view by "model".

The relevant code is in `QTreeView` and `QAbstractItemView`'s
`setModel()`, `setSlectionModel()` and `modelDestroyed()`. Bot mostly in
the `setModels()` where you can see the relevant signals being connected
and disconnected.
https://code.qt.io/cgit/qt/qtbase.git/tree/src/widgets/itemviews/qtreeview.cpp#n179

Fixes: #7947
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.

1 participant