Review Request: KImageCache optimization

Sun Feb 26 22:09:42 GMT 2012

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

Overall I like the idea of taking a more direct role in KImageCache compression to ensure that only the bare necessities are put into the cache (and done so quickly). A bit more work is needed to get it library-quality but it's coming along nicely.

The hashing of the image contents (since KSDC already hashes the keys) is to ensure that we don't waste time overwriting an entry that's already cached, no? It *might* be better to see why such an entry is being cached in the first place and just avoid making the redundant insertImage call (the fastest thing a computer can do is nothing ;) but I'll leave the analysis of that to your existing KWin benchmarks to see if that's feasible/useful.

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

    Lower than MurmurHash2 or higher than? (If it's lower than MH2 then why not use that?)

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

    Please don't change the coding style (the extra spaces added).

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

    Please use the C++-style cast here, if anything so that it's easier to grep for.

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

    Likewise with the cast here.

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

    And another reinterpret_cast<> change here.

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

    This would need to become a 16 here so that KSharedDataCache knows to re-generate existing (older) caches when encountered.

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

    Your comment had mentioned that rawFind() was removed but it's still present in this version of the diff (unless I'm reviewing the wrong one...)

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

    This approach can't work in general. You can't rely on 2 different items remaining in the cache at the same time.

    What needs to happen instead is that the checksum is stored /with/ the image's cached data (i.e. all in the same QByteArray).

    If the image remains cached then it will always have its checksum with it.

    You might want to pad the checksum out to 16 bytes to ensure that the image data is aligned to at least 16 bytes for performance.

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

    You should use a QScopedPointer<QIODevice> here to hold the filter device, as it will make sure the destructor is called correctly. You could then delete the two "delete dev" lines.

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

    QDataStream requires a bit of care.

    You should explicitly set a version on the QDataStream before you use it (see QDataStream::setVersion and http://developer.qt.nokia.com/doc/qt-4.8/qdatastream.html#versioning).

    As recommended in that versioning guide you should also add a version parameter (and a unique identifier "magic") as the first parameter so that it's possible to upgrade KImageCache without having to bump the version on KSharedDataCache in the future.

    Finally, you should cast the integer values you're inputting/outputting to/from the QDataStream to the desired type (e.g. qint32). It would be acceptable to use C-style casts here IMHO.

    For example:

    QDataStream stream(dev);

    stream << (quint32) 0x1C022012; // magic
    stream << (quint32) 0x00010001; // version

    // Must be set before streaming Qt types
    stream.setVersion(QDataStream::Qt_4_6);

    stream << givenImageBits;
    // etc.

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

    The QDataStream status should be checked before closing the device and inserting the QByteArray to ensure that there were no problems (unlikely as that should be).

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

    Don't forget to adjust here based on the changes from line 346 for specific types, versioning, etc. This would include ignoring entries of the wrong version and setting the right Qt version on the QDataStream being used.

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

    QScopedPointer here as well please.

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

    The QDataStream device status should be checked to ensure no errors occurred before using the result.

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

    This shouldn't matter once rawFind() is eliminated anyways, but this must happen *after* the check that the destination pointer is actually set.

- Michael Pyne

On Feb. 26, 2012, 8:08 p.m., Mark Gaiser wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104052/
> -----------------------------------------------------------
> 
> (Updated Feb. 26, 2012, 8:08 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.
> 
> -- update 26-02-2012 --
> After spending a lot more time trying to get compression in the mix "someone" came with the suggestion to use KFilterDev. Never heard of it, but i gave it a shot anyway. It turned out to be the bull-eye in this case. Data is now greatly compressed using gzip (any other compression available in KFilterDev makes it a lot slower). Gzip gives some loss in speed, but compared to the current stock KImageCache it's still a _lot_ faster.
> 
> New benchmarks compared to the stock 4.8.0 KImageCache:
> Read: ~8x faster (was 5x in my previous patch)
> Write: ~5x faster (equally fast compared to stock, no speedup here in my previous patch)
> Inserting the same image with the same key twice is now about free (depending on the size of the image, it's about free for icon sized images with 48x48).
> 
> I've also added a more detailed description of insertImage for the details that it saves. My point of view is that KImageCache should be used for caching of (system)images and perhaps thumbnails. Things like image text should not be stored since that should be fetched from the originating image. The color table (when available) is stored.
> 
> A last massive performance improvement that is possible but complicated to get is by using the IZ compression from "kdepepo". Just using IZ to compress the bits will make the image size a lot smaller then is currently done with gzip and will do it at the very least about 5x faster as well. That means insertion will benefit massively here. However, that will also mean that the stored image data is only going to be:
> - width
> - height
> - format
> - bits
> No color tables! But this is something for the future. Right now the current diff is about fine. The last remaining issues are the possible race conditions as mentioned in the comments below. 
> I need help in that area. I can't do what's suggested (i lack the knowledge to do it myself).
> 
> 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 c8b8c85 
>   kdeui/tests/kimagecachetests.h PRE-CREATION 
>   kdeui/tests/kimagecachetests.cpp PRE-CREATION 
>   kdeui/util/kimagecache.h 54112de 
>   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/20120226/6f61e492/attachment.htm>