[PATCH] Request review for KSharedDataCache

[Oswald Buddenhagen]

moin,

On Sat, Apr 10, 2010 at 12:54:51AM -0400, Michael Pyne wrote:
> I think it's at least in good enough shape to get some eyeballs on it 
> other than mine.

backreferencing from the page table to the entry it contains is only
ever interesting during defragmentation, which is done seldomly and by
only one process at a time. consequently, it would be acceptable to
build up the backlink information only when it is needed, making it
possible to turn the page table into a bitmap and thus making it 32
times smaller (btw, the comment indicates that you intended to make it a
qint16 in fact).

the note about aligning to the os page size (*) for performance is
incorrect - one should only minimize the number of crossed page
boundaries (and as one can save/add exactly one extra crossing, the
impact is bigger for smaller objects).

(*) which is btw typically 4k on 32bit and 8k on 64bit, not 1k as the
default size together with the comments suggest. the sysconf() call can
be used to determine it dynamically.

for an 16x16 icon cache, a page size of 1k will already cause a lot of
slack. for a background image cache it will cause some unnecessary
allocation overhead.
consequently i think the page size should be user-configurable.

i've also been pondering to suggest a more modern tree based allocator
with no paging at all, but this most probably makes no sense for the
scalability requirements of a cache.

 
nitpick: you often seem to forget the space between control flow keyword
and opening parenthesis.

> +class KDEUI_EXPORT KSharedDataCache
>
this class belongs into kdecore.

> +{
> +public:
> +    /*
> +     * Creates and attaches to an icon cache. 
>
no, it doesn't. ;)

> +    KSharedDataCache(const QString &cacheName, unsigned defaultCacheSize = 0);
> +
i think the default size is a property just like the others. ergo it
does not belong into the c'tor (except as a convenience overload maybe)
and it should be dynamically changeable. yes, it is harder to implement
that, but the user should not be concerned with it. and the end user
does not want to discard the cache just to grow it.

> +    enum CacheRemovalPolicy
> +    CacheRemovalPolicy entryRemovalPolicy() const;
> +
inconsistent and suboptimal naming. use EvictionPolicy.

> Index: kdeui/util/kshareddatacache.cpp
> +template<class T>
> +T* alignTo(const void *start, uint size = ALIGNOF(T))
> +{
> +    quintptr mask = size - 1;
> +
> +    // Cast to int-type to handle bit-twiddling
> +    quintptr basePointer = reinterpret_cast<quintptr>(start);
> +
> +    // If we are not aligned, mask off the offending bits and add size (which
> +    // moves us to the next highest aligned byte in memory)

> +    if(basePointer & mask) {
> +        basePointer = (basePointer & ~mask) + size;
> +    }
> +
basePointer = (basePointer + mask) & ~mask;

> +    return reinterpret_cast<T *>(basePointer);
> +}
> +
> +// Returns a pointer to a const object of type T, assumed to be offset *BYTES*
> +// greater than the base address.
> +template<class T>
> +const T *offsetAs(const void *const base, qint32 offset)
> +{
> +    const char *ptr = reinterpret_cast<const char*>(base);
> +    return reinterpret_cast<const T*>(ptr + offset);
>
this function makes no sense without built-in alignTo as the use cases
show.

> +}
> +
> +/**
> + * @return the smallest integer greater than or equal to (@p a / @p b).
> + */
> +template<class T>
> +T intCeil(T a, T b)
> +{

> +    T result = a / b;
> +    result += (a % b) ? 1 : 0;  // Round up if needed
> +    return result;
>
return (a + b - 1) / b;