www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - Debugging memory leak.

reply David Brown <dlang davidb.org> writes:
I've been developing an application on x86_64 (dgcc) without any problems.
Yesterday, I tried building it on x86 to discover that it has a memory leak
on that platform.  The leak is rather significant, and the program quickly
exhausts memory and is killed.

Any suggestions from the list on how to debug/fix this?

Some background as to what the program is doing:

   - It reads large amounts of data into dynamically allocated buffers
     (currently 256K each).

   - These buffers are passed to std.zlib.compress, which returns a new
     buffer.

Some suspicions I have:

   - Because of the larger address space on the x86_64, it is less likely
     that random data will point into one of these buffers, but on the x86,
     it happens more, causing a buffer to be kept around.  Eventually more
     and more of these stick around.

   - It's worse with a gentoo built compiler (USE=d emerge gcc) than with
     the gdc-0.24 binary distribution.  These are both built using gcc
     4.1.2.

Ideas for possibly fixing this:

   - Manually 'delete' these buffers.  In my instance, this wouldn't really
     be all that difficult since I know when they go out of use.

   - Call std.gc.hasNoPointers(void*) on the block.  I would think this is
     the case for a char[], but std.zlib.compress uses a void[], which the
     compiler can't make this assumption about.

   - Try Tango?  Is the GC different there?

David
Oct 08 2007
parent reply Sean Kelly <sean f4.ca> writes:
David Brown wrote:

 
 Ideas for possibly fixing this:
 
   - Manually 'delete' these buffers.  In my instance, this wouldn't really
     be all that difficult since I know when they go out of use.
 
   - Call std.gc.hasNoPointers(void*) on the block.  I would think this is
     the case for a char[], but std.zlib.compress uses a void[], which the
     compiler can't make this assumption about.


std.zlib should likely be changed to use a byte[] array instead.


   - Try Tango?  Is the GC different there?


Somewhat, but void[] arrays are still treated as if they have pointers.


Sean
Oct 08 2007
next sibling parent David Brown <dlang davidb.org> writes:
On Mon, Oct 08, 2007 at 09:22:54AM -0700, Sean Kelly wrote:

 David Brown wrote:

 Ideas for possibly fixing this:
   - Manually 'delete' these buffers.  In my instance, this wouldn't really
     be all that difficult since I know when they go out of use.
   - Call std.gc.hasNoPointers(void*) on the block.  I would think this is
     the case for a char[], but std.zlib.compress uses a void[], which the
     compiler can't make this assumption about.


 std.zlib should likely be changed to use a byte[] array instead.


I called delete on the result coming back from std.zlib.compress and it
seems to have gotten rid of the memory leak.  My guess is that on x86_64,
the address space is large enough that compressed data was unlikely to look
like pointers, but more likely to do so on a 32-bit platform.

So, I would call this a bug in std.zlib.  Even if it still returns the
void[], it should allocate as a byte[].

In my instance, calling delete on the block reduces the amount of work the
garbage collector needs to do, so probably is a good idea anyway.

Thanks,
David
Oct 08 2007
prev sibling parent reply Frits van Bommel <fvbommel REMwOVExCAPSs.nl> writes:
Sean Kelly wrote:

 David Brown wrote:

 Ideas for possibly fixing this:

   - Manually 'delete' these buffers.  In my instance, this wouldn't 
 really
     be all that difficult since I know when they go out of use.

   - Call std.gc.hasNoPointers(void*) on the block.  I would think this is
     the case for a char[], but std.zlib.compress uses a void[], which the
     compiler can't make this assumption about.


 
 std.zlib should likely be changed to use a byte[] array instead.


Yes it should. I created a quick patch for the gphobos version of 
std.zlib (since the OP was using GDC). I attached it to this post (not 
pasted inline due to line wrapping issues).
I haven't tested it other than running it through 'gdc -c', but it's 
such a trivial patch that the fact it compiles should mean it should 
function identically (except for not allocating void[]s, and thus 
hopefully prevent some memory leakage).


   - Try Tango?  Is the GC different there?


 
 Somewhat, but void[] arrays are still treated as if they have pointers.


But AFAICT tango.io.compress.Zlib doesn't allocate any of those, just 
ubyte[] arrays, and exception classes. (The unittest does use a 
MemoryConduit, which internally uses a void[], but nothing else should 
allocate void[]s for that module)
Oct 08 2007
next sibling parent reply Brad Roberts <braddr puremagic.com> writes:
On Mon, 8 Oct 2007, Frits van Bommel wrote:


 Sean Kelly wrote:

 David Brown wrote:

 
 Ideas for possibly fixing this:
 
   - Manually 'delete' these buffers.  In my instance, this wouldn't really
     be all that difficult since I know when they go out of use.
 
   - Call std.gc.hasNoPointers(void*) on the block.  I would think this is
     the case for a char[], but std.zlib.compress uses a void[], which the
     compiler can't make this assumption about.


 
 std.zlib should likely be changed to use a byte[] array instead.


 
 Yes it should. I created a quick patch for the gphobos version of std.zlib
 (since the OP was using GDC). I attached it to this post (not pasted inline
 due to line wrapping issues).
 I haven't tested it other than running it through 'gdc -c', but it's such a
 trivial patch that the fact it compiles should mean it should function
 identically (except for not allocating void[]s, and thus hopefully prevent
 some memory leakage).
 

   - Try Tango?  Is the GC different there?


 
 Somewhat, but void[] arrays are still treated as if they have pointers.


 
 But AFAICT tango.io.compress.Zlib doesn't allocate any of those, just ubyte[]
 arrays, and exception classes. (The unittest does use a MemoryConduit, which
 internally uses a void[], but nothing else should allocate void[]s for that
 module)


Please attach that diff to a bug report so that it doesn't get lost.  I'll 
look at it getting it into the next release.

Thanks,
Brad
Oct 08 2007
parent Frits van Bommel <fvbommel REMwOVExCAPSs.nl> writes:
Brad Roberts wrote:

 On Mon, 8 Oct 2007, Frits van Bommel wrote:

 Yes it should. I created a quick patch for the gphobos version of std.zlib
 (since the OP was using GDC). I attached it to this post (not pasted inline
 due to line wrapping issues).


 
 Please attach that diff to a bug report so that it doesn't get lost.  I'll 
 look at it getting it into the next release.


I recreated it for DMD (the patch wouldn't apply to the DMD std.zlib but 
was trivial to recreate).
I filed a bug report[1]. Given your comment I wasn't sure whether to 
assign it to you or Walter, so I left it assigned to Walter (the 
default) and just added you  in the 'CC' box, figuring you'd at least be 
interested in it :).


[1]: http://d.puremagic.com/issues/show_bug.cgi?id=1557
Oct 08 2007
prev sibling next sibling parent reply Sean Kelly <sean f4.ca> writes:
Frits van Bommel wrote:

 Sean Kelly wrote:
 

   - Try Tango?  Is the GC different there?


 Somewhat, but void[] arrays are still treated as if they have pointers.


 
 But AFAICT tango.io.compress.Zlib doesn't allocate any of those, just 
 ubyte[] arrays, and exception classes. (The unittest does use a 
 MemoryConduit, which internally uses a void[], but nothing else should 
 allocate void[]s for that module)


Yup.  However, an annoying problem still exists with Buffer.  Basically, 
this class maintains a void[] reference to a block of memory allocated 
as a byte[].  However, if the block is resized for any reason, the type 
doing the resizing is used to determine whether the newly allocated 
block contains pointers.  I've been meaning to change the Tango runtime 
and GC to preserve array block attributes across reallocations, but it's 
a somewhat involved process and I haven't gotten to it yet.


Sean
Oct 08 2007
next sibling parent reply Frits van Bommel <fvbommel REMwOVExCAPSs.nl> writes:
Sean Kelly wrote:

 Frits van Bommel wrote:

 Sean Kelly wrote:


   - Try Tango?  Is the GC different there?


 Somewhat, but void[] arrays are still treated as if they have pointers.


 But AFAICT tango.io.compress.Zlib doesn't allocate any of those, just 
 ubyte[] arrays, and exception classes. (The unittest does use a 
 MemoryConduit, which internally uses a void[], but nothing else should 
 allocate void[]s for that module)


 
 Yup.  However, an annoying problem still exists with Buffer.  Basically, 
 this class maintains a void[] reference to a block of memory allocated 
 as a byte[].  However, if the block is resized for any reason, the type 
 doing the resizing is used to determine whether the newly allocated 
 block contains pointers.  I've been meaning to change the Tango runtime 
 and GC to preserve array block attributes across reallocations, but it's 
 a somewhat involved process and I haven't gotten to it yet.


Actually, that bug (or its Phobos equivalent) seems to be partly to 
blame as well. If you look at std.zlib, you'll see that right after 
every "new void[whatever]", std.gc.hasNoPointers is called on the 
result. However, there are some ~/~= operations on those buffers while 
typed as void[]s.
I think if the "has no pointers" bit carried over from the original 
arrays when concatenating (if neither contains pointers[1], the result 
doesn't contain any either) std.zlib might actually be leak-free.


[1]: If an array gc-allocated, use the attribute as set by the 
allocation function or the user. Otherwise, use the default for the 
static type (through TypeInfo).


P.S. I think I'm starting to realize what you meant by that "a somewhat 
involved process" comment ;).
Oct 08 2007
parent Sean Kelly <sean f4.ca> writes:
Frits van Bommel wrote:

 Sean Kelly wrote:

 Frits van Bommel wrote:

 Sean Kelly wrote:


   - Try Tango?  Is the GC different there?


 Somewhat, but void[] arrays are still treated as if they have pointers.


 But AFAICT tango.io.compress.Zlib doesn't allocate any of those, just 
 ubyte[] arrays, and exception classes. (The unittest does use a 
 MemoryConduit, which internally uses a void[], but nothing else 
 should allocate void[]s for that module)


 Yup.  However, an annoying problem still exists with Buffer.  
 Basically, this class maintains a void[] reference to a block of 
 memory allocated as a byte[].  However, if the block is resized for 
 any reason, the type doing the resizing is used to determine whether 
 the newly allocated block contains pointers.  I've been meaning to 
 change the Tango runtime and GC to preserve array block attributes 
 across reallocations, but it's a somewhat involved process and I 
 haven't gotten to it yet.


 
 Actually, that bug (or its Phobos equivalent) seems to be partly to 
 blame as well. If you look at std.zlib, you'll see that right after 
 every "new void[whatever]", std.gc.hasNoPointers is called on the 
 result. However, there are some ~/~= operations on those buffers while 
 typed as void[]s.
 I think if the "has no pointers" bit carried over from the original 
 arrays when concatenating (if neither contains pointers[1], the result 
 doesn't contain any either) std.zlib might actually be leak-free.
 
 
 [1]: If an array gc-allocated, use the attribute as set by the 
 allocation function or the user. Otherwise, use the default for the 
 static type (through TypeInfo).
 
 
 P.S. I think I'm starting to realize what you meant by that "a somewhat 
 involved process" comment ;).


Some of the complication comes from a desire for efficiency.  Currently, 
the array reallocation routines may call two synchronized GC functions: 
gc_sizeOf and gc_malloc.  Preserving block attributes would currently 
require calling gc_getAttr as well, which would mean three mutex locks 
for a single append/resize operation.  What I'd like to do is create an 
aggregate routine called something like gc_describe which returns all 
the relevant information in one go, and replace the call to gc_getAttr 
with a call to gc_describe.  Then the relevant block info can be 
preserved and passed into the call to gc_malloc later on.  The result 
being that the current maximum of two mutex locks would be retained.

The other complication will be a maintenance issue.  If somewhere in the 
runtime performs an array reallocation somehow differently, there will 
be "holes" in the functionality preserving block attributes. 
Fortunately, this portion of the runtime is fairly stable so I don't 
foresee it being a problem.

As an aside, I'd originally considered replacing the whole mess with a 
call to gc_realloc, but currently gc_realloc is allowed to fail of the 
supplied pointer is to the interior of a memory block (ie. a slice).  I 
don't want to change this, because the current scheme allows a GC 
implementation to simply call C malloc/realloc/free, which would not be 
possible if the GC were required to operate correctly on interior pointers.


Sean
Oct 08 2007
prev sibling parent renoX <renosky free.fr> writes:
Sean Kelly a écrit :

 Frits van Bommel wrote:

 Sean Kelly wrote:


   - Try Tango?  Is the GC different there?


 Somewhat, but void[] arrays are still treated as if they have pointers.


 But AFAICT tango.io.compress.Zlib doesn't allocate any of those, just 
 ubyte[] arrays, and exception classes. (The unittest does use a 
 MemoryConduit, which internally uses a void[], but nothing else should 
 allocate void[]s for that module)


 
 Yup.  However, an annoying problem still exists with Buffer.  Basically, 
 this class maintains a void[] reference to a block of memory allocated 
 as a byte[].  However, if the block is resized for any reason, the type 
 doing the resizing is used to determine whether the newly allocated 
 block contains pointers.  I've been meaning to change the Tango runtime 
 and GC to preserve array block attributes across reallocations, but it's 
 a somewhat involved process and I haven't gotten to it yet.
 
 
 Sean


IMHO, that's a different issue: the compress prototype should be 
modified to indicates that it returns byte[] not void[]: after all, no 
pointer is expected in compressed data, so the function signature should 
reflect this.

Regards,
renoX
Oct 09 2007
prev sibling parent Daniel Keep <daniel.keep.lists gmail.com> writes:
Frits van Bommel wrote:

 But AFAICT tango.io.compress.Zlib doesn't allocate any of those, just
 ubyte[] arrays, and exception classes. (The unittest does use a
 MemoryConduit, which internally uses a void[], but nothing else should
 allocate void[]s for that module)


I use ubyte[]s for anything dealing with binary data; I only ever use
void[]s where I'm dealing with typed stuff when I don't care what the
type is.  Incidentally, tango.io.compress.Bzip2 also uses ubyte[]s
internally (really, they're basically the same module, except with "z_"
replaced with "bz_" :P ).

	-- Daniel
Oct 08 2007