[PATCH] Request review for KSharedDataCache (Round 2)

[Michael Pyne]

On Wednesday, April 28, 2010 06:20:37 Oswald Buddenhagen wrote:
> 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.

Yes, fredrikh IIRC was talking with me about how to do this more properly, 
luckily Qt 4.6 supports creating QPixmaps based on an existing X11 Picture, 
but we'd need to force the pixmap to stay alive, so probably a kded module 
would be needed.

> > 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. :)

Yeah, more information is always better, especially something as useful as the 
mean and standard deviation. As it stands I agree that the standard deviation 
would be very hard to use for the end developer.

Anyways I'll go ahead and work on that.

> > > > +    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.

Well it's certainly no difference to the implementation whether it reads 20 
bytes and checks the last 1 to determine the version or whether it reads the 
first byte as the version. I've already bumped the version once in my internal 
testing. I'll move it though, all I'd need to do is unlink my .kcache files.

> > > > +        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.

That's a good point.

I'll work on the changes then. Please let me know if you have any other 
concerns, or if you have problems with me committing after I make those 
changes.

Regards,
 - Michael Pyne
-------------- 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/20100428/daaeb6a7/attachment.sig>