Review Request 127866: Oxygen: Fix QCache usage
Michael Pyne
mpyne at kde.org
Sat May 21 22:26:37 BST 2016
> On May 15, 2016, 8:51 a.m., Hugo Pereira Da Costa wrote:
> > To be honest, I am quite puzzle by this whole thing.
> > Now, every insertion in the cache requires at least two searches in there and (in many case) at least one copy constructor being called. This is quite expansive ... (even though this happens only if the object is not found in the cache).
> >
> > Also: not sure I understand what issue we are trying to fix and how: why is it that if the object inserted in the cache is immediately deleted, just retrying an indefinite amount of time will "fix" the issue. Are we not just transforming a crash into a freeze (infinite loop) ?
> >
> > The Qt documentation is very vague about cases where the object is deleted immediately, and the only case it mentions is: " In particular, if cost is greater than maxCost(), the object will be deleted immediately."
> > Well, in such cases (that should not appear here), the infinite loop will not help. Right ?
> > Since we have no idea on how "predictible" the other deletion cases are, I don't think the fix is a good fix.
> >
> > Does that mean that we should change the code in order to use references rather than pointer everywhere ? (as you did in the first patch on this topic) ?
> >
> > Or get rid of using QCache (because this absence of guarantee at the insertion stage is too much of a pain to handle) ?
> >
> > Or just commit and wait for bug reports about freezes ? (but with a happy coverty) ?
>
> Michael Pyne wrote:
> For the most part the requirements are determined by the current return type within the code. If we return a pointer then currently it has to have come directly out of the QCache. Since QCache is assumed to be the owner, the calling code won't delete the pointer ; but if the caller won't delete the pointer then we'll have a memory leak if we return a pointer to something that had been new'd instead.
>
> References (i.e. QColor&) are a similar issue; it's safe enough to return a reference to something held within the QCache, but we can't return a reference to a local variable since that reference will invalidate as soon as we return from the function. Of course a reference to a cached QColor may *also* invalidate with the next call to an insert method of that cache, but that's a separate story.
>
> It is unfortunate that the Qt docs are vague about this, since if the **only** thing we had to worry about was cost being >maxCost(), we could pretty much just mark 'ignore' for all the Coverity issues associated with this (and I'd be fine doing that). The docs do kind of hint at that but don't make it clear if is the only way that an entry would be deleted immediately.
>
> I think you're right that a loop is not a good idea... I was figuring that eventually QCache would remove enough other items to make it work but then I suppose QCache::insert() would have done that with the very first attempt.
>
> As far as other options, I would definitely recommend against QCache for the QColors: I'd say just hold onto specific QColors directly (perhaps in a QHash) and, if possible, return them as values instead of references.
>
> I'm not sure if we could get away with the same for TileSets, but if so it would again make things easier. If we can't we could look into making TileSet an implicitly shared class so that we can return it by value cheaply.
>
> I wouldn't recommend a commit only to make Coverity happy. I've marked other reports as "False Positive" and even "That's a bug, but we're ignoring it" before. But it does seem to me that if a crash *is* possible (especially in underlying library code) we should do something to avoid it.
>
> Either way I'll see if I can work up a revised patch but I'd still appreciate advice on what's workable or not within Oxygen.
>
> Mark Gaiser wrote:
> Disclaimer: i'm not an oxygen dev! I just read the patch and want to share my opinion :)
>
> I'm also quite puzzeled with this change..
>
> A cache is "usually" being used to store the result of a "heavy" operation and use that result the next time to speed things up.
> That principle sounds great to me and should be used in places if needed. A better approach would be to speed up the slow operation to make caching simply not needed, but that's a different story.
>
> The changes you've made now make QCache usage heavier and most certainly much more difficult to follow because of the added while loops to keep an hypothetical case from happening. If QCache requires measures like this then perhaps we should not be using QCache at all.
>
> I understand you're trying to do your best, Michael Pyne, but a patch like this feels like heading in the wrong direction to me..
>
> I don't know if it's usefull, but there is als QPixmapCache [1] (which uses a QCache internally, but seems to behave as a normal sane LRU cache [2])
>
> Cheers,
> Mark
>
> [1] http://doc.qt.io/qt-5/qpixmapcache.html
> [2] https://en.wikipedia.org/wiki/Cache_algorithms#LRU
>
> Frank Reininghaus wrote:
> > If we can't we could look into making TileSet an implicitly shared class so that we can return it by value cheaply.
>
> Or alternatively, one could return not raw pointers, but shared pointers (like `std::shared_ptr<TileSet>` or `QSharedPtr<TileSet>`). Then it would be 100% clear that the caller, who gets the pointer from the cache, is now a co-owner of the object and can be sure that it won't be deleted as long as they use it.
>
> Having a `QCache<QSharedPointer<TileSet>>` would then store pointers to shared pointers, unfortunately, but if we made `TileSet` implicitly shared, then it would essentially be the same, even if one of the pointers would be hidden inside the implementation.
>
> I have the feeling that we need a replacement for `QCache` that works with shared pointers :-) `QCache` was probably designed in a time when there were only raw pointers, and no clear ways to express ownership (which is one of the reasons why tools like Coverity are needed).
>
> One could argue that a cache that deals with shared pointers adds a small overhead (due to the reference counting), but since a cache is supposed to be used with objects that are expensive to create, this should not matter at all.
It would probably not be too difficult to make a simple cache that held shared pointers directly (to avoid pointers to shared pointers). If we embed an assumption that it's a FIFO then we already have QQueue<T> and it's simple. Even QCache itself is not very big, in fact I was able to verify directly that there are no other ways to delete a pointer upon ::insert other than exceeding the maxCost, which is already an error. It would be nice if Qt would document that this is the *only* failure mode (instead of that this is a *particular* failure mode), in that case we could just add an assert that the cost going in is less than maxCost and I /think/ that should satisfy Coverity (which is pretty smart in its static analysis).
However I've also started on tweaking the patch to return QColor directly (instead of a reference) which simplifies much of this right off the bat, and leaves TileSet alone as the ugly case. I do think that we need to do something different in here though (probably shared pointers!)... right now I'm not sure if the cache is really needed to reduce memory usage or if it's simply a way to share common data without duplicating it in memory everywhere.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127866/#review95480
-----------------------------------------------------------
On May 8, 2016, 5:03 a.m., Michael Pyne wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127866/
> -----------------------------------------------------------
>
> (Updated May 8, 2016, 5:03 a.m.)
>
>
> Review request for kde-workspace and Hugo Pereira Da Costa.
>
>
> Repository: oxygen
>
>
> Description
> -------
>
> This should mostly complete the QCache fixes I kicked off in a previous RR, 127837. Hugo noted there were many other similar usages, and boy he wasn't kidding! ;) The long story short is that these usages can theoretically cause use-after-free behavior (which can lead to crashes and even undefined behavior if the compiler ever gets smart).
>
> *NOTE* It is -much- easier to review if you download the diff to your git repository for oxygen and then run "git diff -b" to ignore whitespace changes, particularly for the QPixmap changes.
>
> For QPixmaps we return values instead of pointers, so we simply make a separate copy to be cached when we do insert. For QColor we return references to values so we *must* return pointers, and those have to be owned by a QCache to avoid memleaks. So I added a helper function to loop until the cache accepts the new entry. TileSets are a similar concern, except those have manual loops since I was uncertain about whether TileSet's copy constructor was the best idea or not.
>
> This fixes a ton of Coverity issues (59717 - 259733, 259739, 259742 - 259752, 1336154, 1336155) and might be associated with Qt bug 38142 and KDE bug 219055 (which doesn't actually appear to be a dupe of a different bug to me...).
>
>
> Diffs
> -----
>
> kstyle/oxygenstylehelper.cpp 612ba37
> liboxygen/oxygenhelper.h a6453a0
> liboxygen/oxygenhelper.cpp 4843604
> liboxygen/oxygenshadowcache.cpp 907e586
>
> Diff: https://git.reviewboard.kde.org/r/127866/diff/
>
>
> Testing
> -------
>
> Compiled without warnings, installed and ran `oxygen-demo5 -style oxygen`. Used the GUI Benchmark feature to automatically cycle through all the listed features -- no crashes or obvious rendering errors.
>
>
> Thanks,
>
> Michael Pyne
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20160521/0bb7abb4/attachment.htm>
More information about the kde-core-devel
mailing list