[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-core-devel
Subject: Re: Review Request 127866: Oxygen: Fix QCache usage
From: Mark Gaiser <markg85 () gmail ! com>
Date: 2016-05-15 20:57:42
Message-ID: 20160515205742.8443.65044 () mimi ! kde ! org
[Download RAW message or body]
--===============1160109119452405498==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
> On mei 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.
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
- Mark
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127866/#review95480
-----------------------------------------------------------
On mei 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 mei 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
>
>
--===============1160109119452405498==
MIME-Version: 1.0
Content-Type: text/html; charset="utf-8"
Content-Transfer-Encoding: 7bit
<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 \
solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;"> \
<tr> <td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/127866/">https://git.reviewboard.kde.org/r/127866/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On mei 15th, 2016, 8:51 a.m. UTC, <b>Hugo Pereira \
Da Costa</b> wrote:</p> <blockquote style="margin-left: 1em; border-left: 2px solid \
#d0d0d0; padding-left: 10px;"> <pre style="white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">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).</p> <p \
style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">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) ? </p> <p style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: inherit;">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. </p> <p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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) ? </p> <p \
style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">Or get rid of using QCache (because this absence of guarantee at the \
insertion stage is too much of a pain to handle) ? </p> <p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Or \
just commit and wait for bug reports about freezes ? (but with a happy coverty) \
?</p></pre> </blockquote>
<p>On mei 15th, 2016, 2:21 p.m. UTC, <b>Michael Pyne</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre> </blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">Disclaimer: i'm not an oxygen dev! I just read the patch and want to share \
my opinion :)</p> <p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">I'm also quite puzzeled with this change..</p> <p \
style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">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.</p> \
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">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.</p> <p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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..</p> <p style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: inherit;">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])</p> <p style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: inherit;">Cheers, Mark</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">[1] http://doc.qt.io/qt-5/qpixmapcache.html [2] \
https://en.wikipedia.org/wiki/Cache_algorithms#LRU</p></pre> <br />
<p>- Mark</p>
<br />
<p>On mei 8th, 2016, 5:03 a.m. UTC, Michael Pyne wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: \
1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; \
-webkit-border-radius: 6px;"> <tr>
<td>
<div>Review request for kde-workspace and Hugo Pereira Da Costa.</div>
<div>By Michael Pyne.</div>
<p style="color: grey;"><i>Updated mei 8, 2016, 5:03 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
oxygen
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" \
style="border: 1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">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).</p> <p style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: inherit;"><em style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
normal;">NOTE</em> 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.</p> <p style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: inherit;">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 <em style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
normal;">must</em> 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.</p> <p \
style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">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...).</p></pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: \
1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">Compiled without warnings, installed and ran <code \
style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: \
0;line-height: inherit;">oxygen-demo5 -style oxygen</code>. Used the GUI Benchmark \
feature to automatically cycle through all the listed features -- no crashes or \
obvious rendering errors.</p></pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>kstyle/oxygenstylehelper.cpp <span style="color: grey">(612ba37)</span></li>
<li>liboxygen/oxygenhelper.h <span style="color: grey">(a6453a0)</span></li>
<li>liboxygen/oxygenhelper.cpp <span style="color: grey">(4843604)</span></li>
<li>liboxygen/oxygenshadowcache.cpp <span style="color: grey">(907e586)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/127866/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
--===============1160109119452405498==--
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic