Review Request: KImageCache optimization

[Michael Pyne]

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


The big issue is a race condition between finding an entry and its eventual use later... this might be suitable for limited use-cases where end-user code can buy into that arrangement, but is not suitable in general for KSharedDataCache or KImageCache.

To answer a different comment, the cache is always fixed-size because of the design of the underlying data structure (currently a fixed-size hash table). The entire structure is pre-allocated on disk for architectures that support posix_fallocate (IIRC) to avoid crashes for OS'es that overcommit disk space.


kdecore/util/kshareddatacache.h
<http://git.reviewboard.kde.org/r/104052/#comment8785>

    As you note here in the comment, there is a race condition in general between unlocking the cache and actually finally copying the data to the eventual destination.
    
    Although I deeply appreciate the need for core code like KSharedDataCache to be as efficient as possible, this can't be done by adding even more segfaults (however rare).
    
    One possible alternative that might be possible and keep your core idea is to possibly find a way to invalidate those "fromRawData"'d QByteArrays if the cache (or better yet, the affected data pages) is modified in between, similar to how QPointer<T> knows when the QObject it's pointing to has been deleted. Obviously this is easier said than done for a low-level class like QByteArray.



kdecore/util/kshareddatacache.cpp
<http://git.reviewboard.kde.org/r/104052/#comment8786>

    The cache is unlocked here but temp is still pointing into its internals (as mentioned above)... it's possible to extract the lock out to wrap around this copy too but then I'm not sure how that would make anything different, you'd still be performing the copy.



kdeui/util/kimagecache.cpp
<http://git.reviewboard.kde.org/r/104052/#comment8787>

    Is this still needed? (I'm assuming not. ;)



kdeui/util/kimagecache.cpp
<http://git.reviewboard.kde.org/r/104052/#comment8788>

    Like some of the other reviewers I would prefer to maintain at least some compression of the image (since in general CPU is nowadays cheaper than memory and memory bandwidth), in addition to the concern with indexed/paletted images (from the Qt docs it seems the color table will still be left empty here).
    
    Of course, the reason the QImage must be constructed that way is mostly related to being able to use the rawData QByteArray directly, which we can't really do without the cache locked... assuming we can find a way around that this construct might still be useful for QImages which we verify have no color table.


- Michael Pyne


On Feb. 23, 2012, 7:23 p.m., Mark Gaiser wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104052/
> -----------------------------------------------------------
> 
> (Updated Feb. 23, 2012, 7:23 p.m.)
> 
> 
> Review request for kdelibs, David Faure and Michael Pyne.
> 
> 
> Description
> -------
> 
> I was running KWin through callgrind to see where possible bottlenecks are. I wasn't expecting much since it improved greatly during the 4.8 dev cycle, however one stood out. The saving of PNG images was taking about 1/5th of the time in KWin that i could see directly. That looked like something i might be able to optimize.
> 
> What this patch is doing is storing the actual image bits to prevent saving a PNG image to the mmapped cache. That was a hot code path in time (cycles), not even in calls. I've also reduced the amount of memory copies to a bare minimum by adding a rawFind function to KSharedDataCache which fills a QByteArray::fromRawData thus preventing a expensive memory copy. The rawFind is used for looking up an image and fetching it's data without copying it. That is done because QImage seems to make a copy itself internally. I don't have any performance measurements, however, prior to this patch my kwin test was using up ~5.000.000.000 cycles. After this patch it's using up 1.370.000.000. I don't have raw performance numbers to see if the cache itself is actually faster, it certainly has become a lot cheaper to use the cache. Logic wise i would say creating a QImage from the cached data should be way faster now since there is no step involved anymore in decoding the image. Storing is certainly an order of magnitude faster.
> 
> Benchmark numbers. insert(write) and find(read)
> 
> -- Before patch --
> READ : 0.019 msecs per iteration (total: 79, iterations: 4096)
> WRITE: 0.010 msecs per iteration (total: 88, iterations: 8192)
> 
> -- After patch --
> READ : 0.019 msecs per iteration (total: 79, iterations: 4096)
> WRITE: 0.0026 msecs per iteration (total: 87, iterations: 32768)
> 
> Reading is equal in speed, writing is ~5x faster after the patch.
> 
> Special thanks go to David Faure for helping me a great deal with this.
> 
> 
> Diffs
> -----
> 
>   kdecore/util/kshareddatacache.h 339cecc 
>   kdecore/util/kshareddatacache.cpp 9fe3995 
>   kdeui/tests/CMakeLists.txt 63788f6 
>   kdeui/tests/kimagecachetests.h PRE-CREATION 
>   kdeui/tests/kimagecachetests.cpp PRE-CREATION 
>   kdeui/util/kimagecache.cpp a5bbbe1 
> 
> Diff: http://git.reviewboard.kde.org/r/104052/diff/
> 
> 
> Testing
> -------
> 
> I've also written a bunch of test cases (greatly improved by David Faure) to see if i didn't break anything. According to the test (which is also comparing the actual image bits) it's all passing just fine.
> 
> 
> Thanks,
> 
> Mark Gaiser
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20120224/d520492b/attachment.htm>