[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 <Lars.Knoll () mpi-hd ! mpg ! de>
Date: 1999-08-31 16:44:18
[Download RAW message or body]
On Tue, 31 Aug 1999, Waldo Bastian wrote:
> On Tue, 31 Aug 1999, Bjarni R. Einarsson wrote:
> > Sorry if this doesn't thread correctly, I'm pasting from
> > http://www.lists.kde.org/, since I'm not actually subscribed to kfm-devel
> > (I'm just a power user, not a KDE developer). :-)
> >
> > Waldo wrote:
> > > Would you care to tell what these numbers you mentioned mean? I guess
> > > it is some kind of memory usage. But of what? If it is the size of the
> > > process you are just looking at noise.
> >
> > The test results I'm referring to may be seen at:
> >
> > http://www.mmedia.is/~bre/programs/khtmlw.memleak.patch.txt
> >
> > You are correct that I only measured the process size (using top) -
> > but if you read how I did it, you'll see that I took steps to minimize
> > noise - I used a fixed test sequence and logged out between each test
> > (necessary anyway to switch libs). I normalized the size of all
> > windows, and had very few programs open (always the same ones).
> >
> > Also I was measuring differences - not absolute sizes, I just
> > displayed the absolute sizes as well so people could verify my
> > calculations and do comparisons of their own.
> >
> > I have added results that show how my patch does after Lars' fix has
> > been applied (patch.7) and how things turn out if only the memoryCache
> > is deleted (one line patch left as an excersize for the reader).
> >
> > In short, I disagree with you about me seeing "noise". I did my
> > homework, and the numbers I'm seeing reflect directly the changes I've
> > made to the code. The only "weird" result was (unfortunately) the
> > testing of patch.5 - but patch.7 is giving very stable, very nice
> > numbers to me.
>
> Assuming that all leaks have been fixed in patch 7, the 'Leaked' column
> shows the noise.
>
> You can see that it takes about 4 reloads to 'stabilize' the memory usage and
> that you keep having fluctuations of about 50-100 bytes.
>
> So I think the difference between your measurments on the last two patches
> is quite clear. But in the first set of measurments you can't tell whether the
> differences are noise or structural.
>
> > My patches, and a description of the testing done with them may all
> > be found at http://www.mmedia.is/~bre/programs/.
> >
> >
> > > If the number of icons does increase wildly we need to find the cause
> > > of that instead of clearing the cache once in a while. This cache does
> > > what it is supposed to do. It might not be the best design to have an
> > > an ever increasing cache but 1.1.2 is not about redesigning.
> >
> > Good code isn't just bug-free it should also be bug-tolerant. My patch
> > makes this piece of code more bug-tolerant, in addition to fixing real
> > bugs (my other message explains this better, as does one of Lars').
>
> Bug-tolerant code is really good but 4 days before a release is not the time
> to switch to bug-tolerant coding. So let's concentrate on real bugs. It is
> obvious that there is a leak somewhere. The movie one-liner was one.
> There must be another line like this somewhere in patch 7.
There are leaks somewhere, but I'm afraid, you will have to dig deep into
kfm code to find, why and when the cacheImage function is called.
I thin, we should apply the patch I attached. It has all the fixes found
except for the clearing of the cache (and I removed some changes, which
don't affect anything), and as it looks to me at the moment, we all agree,
that these changes won't do any harm.
The clearing of the cache might make problems with overlays not being
displayed, so I would propose to leave it out for the moment. I could (if
we don't find the real reason for the problem) think of another method,
were we don't add any images to the cache anymore if it gets bigger than a
certain size.
Cheers,
Lars
>
> The HTMLCache behaves very stange as well but it shuldn't be used during
> your tests, so it can't be responsible for any of the leaks you see. Before we
> change anything about it, I want to have a situation where you can clearly
> see that it is fucking up, and that after applying a patch it is behaving as
> expected.
>
> > And I still think my measurements speak for themselves. If they're just
> > noise, please explain why the noise is so consistantly increasing the
> > memory usage, instead of displaying fluctuations such as can be seen
> > near the end of my test of patch.7.
>
> Your latest measurements give insight in the signal/noise ratio of your
> measurements.
["htmlobj.cpp.patch" (TEXT/PLAIN)]
Index: htmlobj.cpp
===================================================================
RCS file: /home/kde/kdelibs/khtmlw/Attic/htmlobj.cpp,v
retrieving revision 1.69
diff -u -r1.69 htmlobj.cpp
--- htmlobj.cpp 1999/01/18 10:57:54 1.69
+++ htmlobj.cpp 1999/08/31 16:37:43
@@ -50,6 +50,12 @@
QDict<HTMLCachedImage>* HTMLImage::pCache = 0L;
int HTMLObject::objCount = 0;
+// Testing implied that this number was just fine - impact on
+// performance was negligable. Conserving memory is good (tm).
+// It would be best if this could be tuned via. some advanced
+// configuration dialog.
+#define PCACHESIZE 73
+
//-----------------------------------------------------------------------------
HTMLObject::HTMLObject()
@@ -1239,12 +1245,19 @@
//-----------------------------------------------------------------------------
+
HTMLCachedImage::HTMLCachedImage( const char *_filename )
{
pixmap = 0;
filename = _filename;
}
+HTMLCachedImage::~HTMLCachedImage( )
+{
+ if ( pixmap ) delete pixmap;
+ pixmap = 0;
+}
+
QPixmap* HTMLCachedImage::getPixmap()
{
if ( !pixmap )
@@ -1262,7 +1275,8 @@
// yet been initialized. Better be careful.
if( !pCache )
{
- pCache = new QDict<HTMLCachedImage>( 503, true, false );
+ pCache = new QDict<HTMLCachedImage>( PCACHESIZE, true, false );
+ pCache->setAutoDelete(TRUE);
return 0l;
}
@@ -1279,10 +1293,12 @@
{
// Since this method is static, it is possible that pCache has not
// yet been initialized. Better be careful.
- if( !pCache )
- pCache = new QDict<HTMLCachedImage>( 503, true, false );
+ if( !pCache ) {
+ pCache = new QDict<HTMLCachedImage>( PCACHESIZE, true, false );
+ pCache->setAutoDelete(TRUE);
+ }
- pCache->insert( _filename, new HTMLCachedImage( _filename ) );
+ pCache->replace( _filename, new HTMLCachedImage( _filename ) );
}
HTMLImage::HTMLImage( KHTMLWidget *widget, const char *_filename,
@@ -1290,8 +1306,10 @@
int _max_width, int _width, int _height, int _percent, int bdr )
: QObject(), HTMLObject()
{
- if ( pCache == 0 )
- pCache = new QDict<HTMLCachedImage>( 503, true, false );;
+ if ( pCache == 0 ) {
+ pCache = new QDict<HTMLCachedImage>( PCACHESIZE, true, false );;
+ pCache->setAutoDelete(TRUE);
+ }
pixmap = 0;
movieCache = 0;
@@ -1427,6 +1445,7 @@
if ( p )
{
*pixmap = *p;
+ cached = true;
}
else
{
@@ -1469,14 +1488,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
@@ -1485,7 +1505,7 @@
}
else
{
- pixmap = new QPixmap();
+ if ( !pixmap ) pixmap = new QPixmap();
pixmap->loadFromData( _buffer.buffer() );
cached = false;
@@ -1543,6 +1563,7 @@
fclose( f );
QByteArray arr;
arr.assign( p, size );
+ if (movie) delete movie;
movie = new QMovie( arr, 8192 );
// End Workaround
// movie = new QMovie( _filename, 8192 );
@@ -1554,7 +1575,7 @@
}
else
{
- pixmap = new QPixmap();
+ if(!pixmap) pixmap = new QPixmap();
pixmap->load( _filename );
cached = false;
@@ -1815,9 +1836,10 @@
// if ( !imageURL.isEmpty() && !pixmap )
// htmlWidget->cancelRequestFile( this );
+
+ if ( pixmap ) delete pixmap;
+ if ( movieCache ) delete movieCache;
- if ( pixmap && !cached )
- delete pixmap;
#ifdef USE_QMOVIE
if ( movie )
{
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic