Review Request: Use QPixmapCache for KStandardItemListWidget::pixmapForIcon

[Mark Gaiser]

> 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.
> 
> Frank Reininghaus wrote:
>     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.
> 
> Michael Pyne wrote:
>     KSharedDataCache uses a disk file but doesn't actually ever force the OS to commit to disk until the KSharedDataCache is later destroyed. The actual cache updates are done all in memory, which the OS may page to disk at its leisure. However because it is process-shared and not just thread-shared it is probably still not as efficient as what you could get with Qt or STL maps or hashes. The balance really just depends on whether you expect many different processes to do the same intensive cacheable work or not. If processes will normally do work that does not help other processes (or if the work doesn't take so long that it would be nice to persist across multiple process executions) then KSharedDataCache is probably not useful.
>     
>     As far as the [namespace,key] question, it's true that using a hash table instead of a map would tend to make the point moot. I think I've heard of hash table implementations where long common prefixes actually increase the risk of hash collisions though, but I'm not up to researching that right now. I do think I was incorrect on using a suffix introducing a collision risk, I think that's related to crypto hash signature functions instead on second thought.

Just a little note that everyone seems to forget..
KImageCache = file cache (as is mentioned in here) and also pixmap caching  IF a QPixmap is inserted! Then it's using QCache. However, it defaults to just 16 KiB (why so small?) so you can use it just fine, just increase the size.


- Mark


-----------------------------------------------------------
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/20121127/77b9e73b/attachment.htm>