[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 12:08:15
[Download RAW message or body]

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.  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.
> 
> This problem is NOT solved by using QDict::replace (which the
> khtmlw code didn't do anyway, but my patch did) - the replace
> function (probably, this I haven't verified) only checks the
> visible entry, and it does NOT compare hash values, it compares
> actual values (this is according to the man page).

You are right. And it should be fixed before the release. I reviewed the patch,
and (as I already wrote on kde-devel), it looks moslty ok. I found one error:

> @@ -1475,8 +1498,9 @@
>    _buffer.readBlock( buffer, 3 );
>    _buffer.close();
>      
> -  if ( strcmp( buffer, "GIF" ) == 0 )
> +  if ( strcmp( buffer, "GIF89a" ) == 0 )
>    {
> +    if (movie) delete movie;
>      movie = new QMovie( _buffer.buffer() );
>      movie->connectUpdate( this, SLOT( movieUpdated( const QRect &) ) );
>  #if QT_VERSION <= 141

This should rather be:

@@ -1469,14 +1492,15 @@
{
bComplete = true;

-  char buffer[ 4 ];
+  char buffer[ 7 ];
buffer[0] = 0;
_buffer.open( IO_ReadOnly );
-  _buffer.readBlock( buffer, 3 );
+  _buffer.readBlock( buffer, 6 );
_buffer.close();

-  if ( strcmp( buffer, "GIF" ) == 0 )
+  if ( strcmp( buffer, "GIF89a" ) == 0 )
{
+    if (movie) delete movie;
movie = new QMovie( _buffer.buffer() );
movie->connectUpdate( this, SLOT( movieUpdated( const QRect &) ) );
#if QT_VERSION <= 141

Except for this, I would say, we should apply the patch.

> So whether the pCache fixes help with web browsing or not is a moot
> point - they are addressing a real problem with that bit of code.
> 
> Unless there are bugs in my patch, I think people should just raise
> the PCACHESIZE to what they think is reasonable, and use it.

Yes. I would prefer a bigger value for PCACHESIZE. The cache is (mainly) used
for preloading icons needed for kfms iconview. As far as I remember, about
80-100 icons are preloaded. So it would get deleted too fast. This would result
in a performance loss for browsing local directories with iconview enabled.
The cache should get deleted, when it contains more then say about 150 pixmaps.

Cheers,
Lars

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