[PATCH] Request review for KSharedDataCache (Round 2)

[Oswald Buddenhagen]

hi,

only quick replies for now, as i'm supposed to be working now. :)

On Tue, Apr 27, 2010 at 10:57:48PM -0400, Michael Pyne wrote:
> On Sunday, April 25, 2010 15:37:43 Oswald Buddenhagen wrote:
> > > +    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.
> 
i meant the whole concept. if one wanted to fix it properly, one'd make
the pixmap cache use shared pixmaps in memory. the on-disk part may be
based on kshareddatacache, but the memory-mapping would be pointless, as
it would not contribute to the inter-process sharing, and the mmap would
just use up linear memory. kimagecache wouldn't be involved at all.

> > > +    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.
> 
no, just allow the user to specify the expected average item size in
bytes. it's the implementation's problem how it quantizes that value.

fwiw, for ideal results, one would specify not only the average, but
also a standard deviation - but that's likely to be a bit too much for
the average coder. :)

> > > +    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()")
> 
i meant "that the [...] value _be_ 0". i don't know whether this is
actually wrong, but i usually see this in constructs which are
artificially arcane/wrong. more compliant with my perception of
correctness would be "is".

> > > +    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.
> 
yes, but that's besides the point. for the versioning to actually work,
the version field itself needs to be version-independent. the easiest
way to achieve this is by putting it first.

> I'm not a CPU cache expert but shouldn't access speed be based on
> vicinity to other commonly-accessed variables?
> 
there are indeed a few rules to follow to get optimal performance, but
i'd argue that these are thouroughly insignificant in the bigger context
of this class, even if they were violated.

> > > +        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.
> 
yes, but when it is corrupt, as small delay doesn't matter. it's even
kinda good, as it may induce the user to look into the log.

> By 10 seconds with exponential growth, do you mean "no more than 10
> seconds total, but make the waits between lock attempts increase
> exponentially?"
> 
yes