> 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
> 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. - Frank ----------------------------------------------------------- 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 > >
