Make sure we clean up old QItemSelectionModels #7950
Conversation
[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/
There was a problem hiding this comment.
Thank you for the excellent explanation! As it appears to be a small leak and may be a bug in QT, I'd lean towards making a QT bug report or contacting QT devs before we attempt a hacky fix of our own.
That being said, it's been years since I looked at this code :)
| # TreeView::setModel() | ||
| # header->setModel() # creates a new selection model in QHeaderView | ||
| # QAbstractItemView::setModel() # creates a new selection model in QTreeView | ||
| # QTreeView::setSelectionModel() | ||
| # header->setSelectionModel() # tells the QHeaderView to use the selection model from the QTreeView |
There was a problem hiding this comment.
Given that we're not calling setSelectionModel directly, but QT is calling setSelectionModel internally, I'm leaning towards this being a bug in TreeView::setModel.
There was a problem hiding this comment.
Ah, almost forgot to reply. Yeah I agree it's an upstream issue. I'll try to find time to make a C++ reproducer (just do set new models, show we are cleaning up all the selection models we can, print out the increasing number of children) and raise one.
I do think this is pretty safe to merge as it is though.
There was a problem hiding this comment.
I spent a couple of hours trying to c++ then thought to search the bug tracker: https://bugreports.qt.io/browse/QTBUG-49966
I'll probably merge this soon after fixing the inevitable typos. And adding a link to the qtbug.
There was a problem hiding this comment.
That makes sense to me -- is it worth creating a tracking issue to remove this if the QT bug is fixed upstream?
There was a problem hiding this comment.
Hmmm, not sure. Do we normally do that? I think this code is pretty safe, like delete children that aren't being used should be a no-op if they aren't being created, so there's no harm on leaving it in even when not needed. I would lean toward just putting the upstream bug ticket in a comment and if anyone wonders what all this code is for they can click through and see that it isn't required anymore.
Actually thinking about the design choices with setModel(), maybe we should never be calling setModel(None) and should just be deleting the model and letting the connections to model::destroyed ensure that state is cleaned up. Thinking with signals! I haven't tested that yet, but maybe that would be less surprising, and require less explanation, than this explicit cleanup?
There was a problem hiding this comment.
It seems like a good thing to do in general -- even if the code is "safe" we don't want confusing/extra code sitting around if it ends up being unnecessary, but I don't think it's a big deal either way (especially if there's a cleaner workaround!).
There was a problem hiding this comment.
What I normally do is add a # WORKAROUND for https://bugreports.qt.io/browse/QTBUG-49966 comment of some sorts. Then when I drop old Qt versions, I grep for WORKAROUND and remove anything that is not relevant anymore.
Would be good to have this here as well. No strong opinions on how to do the clean-up, but I'm fine with this kind of explicit clean-up, seems quite simple and easy to follow (especially with the great explanation).
There was a problem hiding this comment.
I'll have a crack at re-working this whole PR next week to just do old_model.deleteLater() and NOT set setModel(None). I'm pretty sure QTreeView and friends get cleaned just up fine (eg remove references to the model, delete selection models) when the model's destroyed signal fires.
While we are calling deleteLater() currently the problem is that when you call setModel(None) it creates a selection model connected to the static empty model, which never gets deleted. If you just delete the model it'll clean up the old selection model without creating a new one.
Cons:
- requires another round of changes, verification and review
- bit more magical, it's not called out in the docs that this is the way you should, or could, use the API
Pros:
- should require a lot less explanatory text that no-one wants to read
I put a bit of effort into explaining this one because I'm not very happy with the sledgehammer approach of looping though children. But I think I have a good enough handle on what's happening now and what our options are to say this is all we can do.
What I haven't done yet is try to see the concrete impact of these objects accumulating. How much memory do they take up? Based on Gammaray they do not keep the actual CompletionModel around, so maybe they don't take up much memory?
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, callsetModel(), 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 callsheader.setModel(), which creates a new selection model in the header, but then it callsheader.setSelectionModel(). Which doesn't clean up the short lived selection model created byheader.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:
Fixes: #7947