-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Comments
Here's what I've got so far. A bit stuck with understanding why the objects under the header aren't being cleaned up. What I've discovered so far:
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() |
[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/
[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/
[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/
Trying to look at memory usage. First, how many of these objects do I have live in main running instance:
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?
And running that with massif and looking at the peak of the allocation top allocation graph with a variety of counts: That doesn't look like a huge amount of memory. |
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
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:
How to reproduce
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:
The text was updated successfully, but these errors were encountered: