[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kfm-devel
Subject:    Re: FW: Bug#1812: KFM memory leak: updated patch for khtmlw.
From:       Lars Knoll <knoll () mpi-hd ! mpg ! de>
Date:       1999-08-31 13:17:26
[Download RAW message or body]

On Tue, 31 Aug 1999, Waldo Bastian wrote:
> On Tue, 31 Aug 1999, Bjarni R. Einarsson wrote:
> > On Tue, Aug 31, 1999 at 10:51:16AM +0000, Bjarni R. Einarsson wrote:
> > > On Tue, Aug 31, 1999 at 11:44:58AM +0100, David Faure wrote:
> > > > Some more on the subject.
> > > > 
> > > > A main khtmlw developer (Waldo) told me this about the cache :
> > > > 
> > > > > > > The cache in khtmlw is for icons only. It is intended that this cache
> > > > > > > is never deleted/cleared. A normal web-page does not load any
> > > > > > > icons and does not use this cache at all.
> > 
> > One more thing I'd like to point out here.
> > 
> > I can guarantee that the (unpatched) icon cache WILL leak memory.  
> > 
> > Why?
> > 
> > The cache is based on QDict, and QDict's manpage explicitly states
> > that if two different entries have the same hash value, then only
> > one will be visible - the other will be stored invisibly in a
> > linked list until the entry on top is removed. 
> 
> My documentation says:
> 
>    Items with equal keys are allowed. When inserting two items with the
>    same key, only the last inserted item will be visible (last in, first
>    out) until it is removed. 
> 
> So the items need to have the same _KEY_, not the same _HASH VALUE_.
> So inserting the same image twice in the cache would indeed duplicate
> it. But as far as I know, the cache is always looked up first and only
> if the image is not present already, it is added.

OK, there's a difference between an KEY and a hashValue... But what's important,
and there I must agree with Bjarni, is, that items might get stored twice. I
just did a short check, where the cacheImage method is used. It doesn't
check if the icon is already in the cache! (kbind.cpp and kfmman.cpp in kfm)

So the pCache->insert() call has to be replaced by pCache->replace().
Also I think it's a good programming practice to set the cache to autodelete.
One can argue if the cache clearing is really needed, but if we set the
threshhold for clearing to a high enough number (higher than in the patch), it
won't hurt, if the cache doesn't fill up, and it helps in case it fills up. So
where's the problem?

Cheers,
Lars
  
> >  It's a stack.  This
> > means that if two commonly used icons get the same hash value, they
> > will alternately be inserted into the cache quite frequently,
> > growing this stack indefinately.  This could happen alot,
> > especially as time goes by and people create more and more icons
> > for different file types - hash value collisions will only increase.
> 
> That is why each hash value has a list of buckets.
> 
> Cheers,
> Waldo

[prev in list] [next in list] [prev in thread] [next in thread]