[PATCH] Request review for KSharedDataCache (Round 2)

[Michael Pyne]

On Sunday, April 25, 2010 15:37:43 Oswald Buddenhagen wrote:
> On Sat, Apr 24, 2010 at 11:05:07PM -0400, Michael Pyne wrote:
> > +class KImageCache::Private
> > +{
> > +    bool enableCaching;
> 
> enablePixmapCaching

fixed

> > +QPixmap KImageCache::findPixmap(const QString &key) const
> 
> this should also automatically insert into the pixmap cache.

done

> > +    bool insertImage(const QString &key, const QImage &image);
>                                     ^^
> grammar

fixed

> > +    void setPixmapCaching(bool enable);
> > +
> 
> a somewhat bizarre workaround for the brokeness of KPixmapCache, if you
> ask me. :D
> but whatever.

Well the idea (from KPixmapCache) is useful at least, to allow for copy-on-
write if feasible so I didn't want to drop it completely.

> > +#define KSHAREDDATACACHE_H
> > 
> > +class QAbstractItemModel;
> 
> obsolete

fixed

> > +    enum ItemSize
> > +    {
> > +        /// Items of less than 4 kilobytes of size on average.
> > +        SmallItemSize,
> > +        /// Items of between 4 kilobytes to 64 kilobytes of size on
> > average. +        MediumItemSize,
> > +        /// Items larger than 64 kilobytes of size on average.
> > +        LargeItemSize
> > +    };
> > +
> 
> this is unwise.
> - unnecessary restriction of possible ranges
> - from the user's pov arbitrary quantizaation
> - no actual advantage api-wise

Is your suggestion then to allow the user to simply select the page size 
themselves (with a sane default)? That's easy enough.

> > +    enum EvictionPolicy
> > +    {
> > +        // It's important that the default value be 0 due to mmap().
> 
> grammar

I actually disagree this time, but I will remove the contraction to make it 
clear (i.e. "It is important that the default value be 0, due to mmap()")

> > +    /**
> > +     * Sets the entry removal policy for the shared cache to
> > +     * @p newPolicy. The default is EvictionPolicy::NoRemovalPreference.
> 
> outdated enum value reference

fixed

> > +     * Note that even if the insert was successful that the newly added
> > entry
> 
>                                                       ^^^^
> comma instead? (not that i actually knew comma rules, but it feels less
> wrong :D)

No, you're right. Fixed.

> > +    unsigned cacheSize() const;
> > +    unsigned cacheFree() const;
> > +
> 
> maybe totalSize() and freeSize() instead.

ok.

> > +//
> > ========================================================================
> > = +// Description of the cache:
> > +//
> > +// The page
> > +// table contains shared memory split into fixed-size pages (the exact
> > page +// size is actually configurable at run-time, to allow for
> > balancing the +// demands of index/page table sizes and fragmentation.
> 
>                                                          ^
> missing closing paren. that's usually a good sign of overusing them -
> hyphens and periods often do wonders.

ok, reworded.

> 
> > +struct indexTableEntry
> > +struct pageTableEntry
> 
> even internal structures should be CamelCased.

Fixed.

> > +struct sharedPage
> > +{
> > +    // The actual struct is much larger in size, but since the size
> > varies we +    // just use a 1-length array here for a placeholder.
> > +    char data[1];
> > +};
> > +
> 
> i find it a bit pointless to define a struct for that. it just adds
> casts.

Hmm, true. It did used to be more useful before my last submission. ;)

> > +// This is effectively the layout of the shared memory segment. The
> > variables +// contained within form the header, data contained
> > afterwards is pointed to +// by using special accessor functions.
> > +struct SharedMemory
> > +{
> > +    static const uint PIXMAP_CACHE_VERSION = 1;
> > +    static const uint MINIMUM_CACHE_SIZE = 4096;
> > +
> 
> while the values are actually inlined in optimized builds, the constant
> is still emitted as a memory location. so i prefer enums for constants.

ok.

> > +    mutable pthread_mutex_t lockMutex;
> > +    QAtomicInt ready; ///< DO NOT INITIALIZE
> > +    uint       cacheSize;
> > +    uint       cacheAvail;
> > 
> > +    quint8     version;
> 
> it would be wise to put that field first and define that any version
> upgrades may not change that.
> ready should be the second field. no, actually, it should be the first
> and fixed as well, as it is used even prior to checking the version.

Well ready is only ever used once in the setup, and likewise with version, 
whereas lockMutex, cacheSize, and cacheAvail are used much more frequently.

I'm not a CPU cache expert but shouldn't access speed be based on vicinity to 
other commonly-accessed variables? I suppose I could move ready to be just 
before version to put the commonly used variables clumped together just before 
the once-only variables.

But I'd rather not move the version field if at all possible. If it does help 
I'll move it because I have to figure I (and possibly you ;) are the only ones 
who'd have to manually delete the cache on disk.

> > +        // These must be updated to make some of our auxiliary functions
> > +        // work right since their values will be based on the cache
> > size. +        cacheSize = _cacheSize;
> > +        pageSize = _pageSize;
> > +        version = PIXMAP_CACHE_VERSION;
> > +
> > 
> > 
> > This doesn't do perfect
> > +        // defragmentation since there may be space at the beginning,
> > but it +        // should result in a minimum of memcpy.
> > +
> 
> the saving from that is likely to be minimal, as the first hole is
> likely to be found soon.
> but it may create the situation of needing to free up more pages and
> defragment again right after defragmentation, which you are handing with
> extra code.

You're right, it did lead to extra code and was overall not worth it, fixed.

> 
> > +            if (curIndex < 0) {
> > +                kWarning(264) << "Unable to remove enough used pages to
> > allocate" +                              << numberNeeded << "pages. In
> > theory the cache is empty, weird.";
> 
> kError() then
> 
> > +            pagesRemoved +=
> > intCeil(indexTable()[curIndex].totalItemSize, cachePageSize());
> 
> this parallel data maintenance looks weird.
> why not use the free page count directly?

Hmm, good question.

> > +    bool lock() const
> > +    {
> > +        struct timespec timeout;
> > +        timeout.tv_sec = 1;
> 
> too optimistic. see next.
> 
> > +        while (shm->ready != 2) {
> > +            busyCount++;
> > +            if (++busyCount >= 20) {
> > +                // Didn't acquire within .1 seconds?  Assume an issue
> > exists
> 
> way too optimistic. a little swapping can easily throw this off.
> 10 seconds with an exponential delay time growth may work best.

It's perhaps too optimistic and I can increase the timeouts, but I don't want 
to set it too high either as the idea is that if it takes too long the cache 
is probably corrupt.

I can certainly wait more than .1 seconds if necessary on startup though. By 
10 seconds with exponential growth, do you mean "no more than 10 seconds 
total, but make the waits between lock attempts increase exponentially?"

> i'm sick an tired, so this is not a perfectly thorough review. :)

o_O Well it certainly seems a very thorough review, thanks for taking the 
time. I've attached the current patch updates just for 
k{image,shareddata}cache.{h,cpp}

Regards,
 - Michael Pyne
-------------- next part --------------
A non-text attachment was scrubbed...
Name: kdelibs-add-shareddatacache-rev-2.patch.bz2
Type: application/x-bzip
Size: 16489 bytes
Desc: not available
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20100427/3c40a9ae/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20100427/3c40a9ae/attachment.sig>