[Kde-games-devel] Review Request: KCardCache: Delay loading SVG file until it is actually needed.

[Parker Coates]

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

(Updated 2009-05-15 06:47:58.667470)


Review request for KDE Games and Andreas Pakulat.


Changes
-------

The previous patch ended up being an improvement in some situations, but an annoyance in others. For example, having the load time occur on start up is much nicer than having it occur the first time you flip a card in Klondike.

As a result, I looked using KCardCache's built in threaded loading. Unfortunately, that logic had some issues. The biggest of these was that it tried to use QPixmaps in a non GUI thread. The "dokill" boolean was also uninitialised on creation, so threads often decided to kill themselves for no reason. (That one took a while to figure out.)

This updated patch contains a working threaded loading implementation. Support for threaded loading of the backside has been dropped as it would have required a lot of additional code zero performance gain. (The backside is almost always loaded in the GUI thread and the time to load the small backside SVG is neglible.)

I also have a six line patch to KPat that adds threaded loading. The result is a much faster response on startup, resize and deck change, even on my single core processor. I would imagine it would be even slicker on a multicore system. A similar patch for LSkat should be easy enough.

I'm quite keen to get these changes into KDE4.3. As far as I know, the patch is binary compatible, but I would appreciate it if someone could double check that. I also should mention, again, that this is my first time working with threads, so if I've committed a faux pas anywhere, please let me know.


Summary
-------

Some new card decks have entered into SVN, which although very beautiful there also quite large. For example, the Ancient Egyptians card deck adds 3 seconds to KPat's start up time compared to a lighter deck like Nicu White. This increase in start up time isn't due to rendering time, as KPat already makes use of KCardCache to cache the rendered pixmaps between runs. The time consumed is used purely to load the SVG file into the KSvgRenderer.

This patch to KCardCache causes the SVG file to only be loaded on into the KSvgRenderer on first use. At first I thought this would be sufficient to improve start up time when the cards are already cached, but it is not. KPat must also call defaultFrontSize() on start up to learn the aspect ratio of the cards (something LSkat currently doesn't do). Previously, this function used to get the element size from the renderer, which then has to be loaded. This patch also causes that function to store it's default size in the cache, eliminating the need to access the renderer the next time the function is run, even across sessions.

I should point out that the expense of loading the SVG hasn't been removed, just delayed until a card rendering is needed that is already cached. If this patch works out, I'll look into using KCardCache's built-in threading to load the SVG file in the background.

I have tried to be careful about properly locking the mutexes, but I have no previous experience with QThreads, so please let me know if I'm doing anything dangerous.


Diffs (updated)
-----

  /trunk/KDE/kdegames/libkdegames/cardcache.cpp 967725 
  /trunk/KDE/kdegames/libkdegames/cardcache_p.h 967725 

Diff: http://reviewboard.kde.org/r/676/diff


Testing
-------

A fair bit of testing with KPat. A little bit of testing with LSkat.


Thanks,

Parker