Review Request: Use QPixmapCache for KStandardItemListWidget::pixmapForIcon

[Frank Reininghaus]

> On Oct. 25, 2012, 9:05 p.m., Christoph Feck wrote:
> > Sorry, QPixmapCache is obsolete, please port to KImageCache.
> 
> Christoph Feck wrote:
>     Before you ask, here is what the API docs say:
>     
>     Deprecated:
>     KPixmapCache is susceptible to various non-trivial locking bugs and inefficiencies, and is supported for backward compatibility only (since it exposes a QDataStream API for subclasses). Users should port to KImageCache for a very close work-alike, or KSharedDataCache if they need more control.
> 
> Michael Pyne wrote:
>     In fairness, QPixmapCache is nowhere near the same as KPixmapCache. QPixmapCache is a thread-shared cache (i.e. different processes will have different QPixmapCaches). There are plausible scenarios where you might not need either the additional features or different semantics of KImageCache.
>     
>     However, given that QPixmapCache has no means for namespacing it should not be your first choice for library code (e.g. if the library adds a pixmap called "icon_back", what happens if an application using that library also tries to add or use a pixmap called "icon_back"). If used in library code the key should be modified to include a namespace element e.g. key = QLatin1String("org.kde.KSILW.") + key. But doing this will make the cache itself slower as it has to fight with many different elements with a long common prefix (using a common suffix is probably OK but introduces a minor risk of collision again).
> 
> Christoph Feck wrote:
>     AHHHH... sorry, indeed this is about QPixmapCache, not KPixmapCache. So just forget my comment.

Thanks for your comments, Christoph and Michael!

Hm, it was my understanding that KSharedDataCache stores the cache contents on disk, whereas QPixmapCache does in in memory. Therefore, I thought that using KSharedDataCache might not bring the same performance increase as QPixmapCache for our limited use case. On the other hand, it might enable us to share the pixmaps between different Dolphin processes. It seems that we need to do some careful profiling to see what works out better. I think we're quite safe with QPixmapCache at the moment though because it is only used in this single place in Dolphin.

Michael, I'm not quite sure why adding the namespace element at the beginning or at the end of the key would make a difference. It was my understanding that the string is passed to a hash function, which reads the entire string anyway.


- Frank


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/107039/#review20922
-----------------------------------------------------------


On Oct. 25, 2012, 3:43 p.m., Emmanuel Pescosta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107039/
> -----------------------------------------------------------
> 
> (Updated Oct. 25, 2012, 3:43 p.m.)
> 
> 
> Review request for Dolphin and Frank Reininghaus.
> 
> 
> Description
> -------
> 
> Use QPixmapCache for KStandardItemListWidget::pixmapForIcon(const QString& name, int size) -> Avoid KIcon loading and rescaleing
> 
> 
> Diffs
> -----
> 
>   dolphin/src/kitemviews/kstandarditemlistwidget.cpp 72d10cf 
> 
> Diff: http://git.reviewboard.kde.org/r/107039/diff/
> 
> 
> Testing
> -------
> 
> According to valgrind/callgrind lesser cpu time and according to valgrind/massif lesser memory consume
> 
> 
> Thanks,
> 
> Emmanuel Pescosta
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20121026/a4114e77/attachment.htm>