Forums > Dev

memory leak – what's wrong with this code

June 1, 2006 | 10:10 am

Hi,

I’m having trouble with a circular message buffer in Max 4.5.7 under
Windows. With every call to my code the memory usage of Max is rising a
few bytes and after a lot of calls Max crashes.

Below is the code I’m using. It takes the t_atoms from a message box and
places them into a buffer. Only the first argument gets ignored and the
whole this is prepended with one A_SYM and another A_LONG.

The code below is a simplified version, of course I’m checking whether
there’s enough space in the buffer before writing to it.

// this is my circ. buffer:
x->x_msgbuf = (t_atom *)sysmem_newptr(MSGBUFSIZE * sizeof(t_atom));

// here’s the ‘bad’ code, argc and argv are directly passed on
// from Max

// first in buffer is the symbol "send"
SETSYM(x->x_msgbuf + msghead, ps_send);
if (++msghead >= MSGBUFSIZE)
msghead = 0;

// set client number (long int)
SETLONG(x->x_msgbuf + msghead, client);
if (++msghead >= MSGBUFSIZE)
msghead = 0;

// copy arguments we got from user (except the first one)
for (i = 1; i < argc; i++)
{
x->x_msgbuf[msghead] = argv[i];
if (++msghead >= MSGBUFSIZE) // check for buffer boundaries
msghead = 0;
}

// append a ‘;’ at the end
SETSEMI(x->x_msgbuf + msghead);
if (++msghead >= MSGBUFSIZE)
msghead = 0;

If I just remove this code the increase in memory usage goes away. Is
there anything wrong with this code?

Another related question: when I use defer_low() to call my function,
the memory increase seems to be higher. In the SDK docs it says that
defer_low() passes on copies of (t_atom *)argv, do I have to free it at
the end of my function? And if yes, which of the memory freeing routines
should I use for that?

Olaf


June 1, 2006 | 3:18 pm

Hi Olaf,

There’s nothing wrong with your code that I can see, except that you
didn’t list where you call sysmem_freeptr to free the memory that you
claimed with your sysmem_newptr call. If you don’t ever call
sysmem_freeptr, that’s your problem.

Ben


June 1, 2006 | 4:17 pm

Hi Ben,

I call sysmem_freeptr() in the objects free routine. I just included
that sysmem_newptr() call into the email to show which routine I use for
memory allocation. It does not get called every time something gets
added to the circ. buffer, only once at object creation. So what gets
called over and over again (and seems to be causing the inclrease in
memopry usage finally leading to the crash) is the SET… functions and
the copying of t_atoms into my buffer.

Olaf


June 1, 2006 | 6:21 pm

This wouldn’t make sense. Nothing is alloc’ed in these macros.

#define SETSYM(ap, x) ((ap)->a_type = A_SYM, (ap)->a_w.w_sym = (x))

Problem must be elsewhere. Somewhere an allocation is taking place
which is not being freed. Perhaps send us more code to scrutinize.

-Joshua


June 1, 2006 | 6:58 pm

What OS are you running? What tool are you using to detect the leak?

If you’re running on OS X, you might want to try running max like

env MallocGuardEdges=1 MallocScribble=1 open

from the terminal.

Perhaps what you’re doing is indexing past the end of your buffer? In
OS X vm pages are allocated lazily, so you might see an increase in
size when you try to write to the next page.

_Mark


June 2, 2006 | 7:47 am

Hi Joshua,

I found the bug… As I pointed outed in me previous mails I was using
symem_newptr() to allocate the memory for the circular buffer. After
always using a size of 4096 t_atoms I once increased it to 65536 and
that’s where the trouble starts. Below you’ll find the code I used to
find the bug. MSGBUFSIZE sets the size of the buffer. From a certain
size on it seems not to alloc all the memory I want. But doing the check
whether it returned NULL also fails, so it seems sysmem_newptr() just
returns less memory than I wanted.

Olaf

/* memleak.c — demonstrates the memory leak porblem ——- */

#include "ext.h"

#define MSGBUFSIZE 65536 // this will crash, 4096 works

typedef struct memleak
{
t_object x_obj;
t_atom *x_msgbuf;
long x_msghead;
} t_memleak;

void *memleak_class;

t_symbol *ps_send;

static void memleak_dosend(t_memleak *x, t_symbol *s, short argc, t_atom
*argv);
static void memleak_send(t_memleak *x, t_symbol *s, short argc, t_atom
*argv);
void memleak_free(t_memleak *x);
void *memleak_new();

void main()
{
setup((t_messlist **)&memleak_class,
(method)memleak_new,(method)memleak_free, (short)sizeof(t_memleak), 0L, 0);
addmess((method)memleak_send,"send",A_GIMME,0);

ps_send = gensym("send");
}

static void memleak_dosend(t_memleak *x, t_symbol *s, short argc, t_atom
*argv)
{
long msghead = x->x_msghead;
int i, client;

if (argv[0].a_type == A_LONG)
{
client = argv[0].a_w.w_long – 1;
}
else
{
error("no client specified");
return;
}

// first in buffer is the symbol "send"
SETSYM(x->x_msgbuf + msghead, ps_send);
if (++msghead >= MSGBUFSIZE)
msghead = 0;

// set client number (long int)
SETLONG(x->x_msgbuf + msghead, client);
if (++msghead >= MSGBUFSIZE)
msghead = 0;

// copy arguments we got from user (except the first one)
for (i = 1; i < argc; i++)
{
x->x_msgbuf[msghead] = argv[i];
if (++msghead >= MSGBUFSIZE) // check for buffer boundaries
msghead = 0;
}

// append a ‘;’ at the end
SETSEMI(x->x_msgbuf + msghead);
if (++msghead >= MSGBUFSIZE)
msghead = 0;

x->x_msghead = msghead;
}

static void memleak_send(t_memleak *x, t_symbol *s, short argc, t_atom
*argv)
{
memleak_dosend(x, s, argc, argv);
}

void memleak_free(t_memleak *x)
{
sysmem_freeptr(x->x_msgbuf);
}

void *memleak_new()
{
t_memleak *x;

x = (t_memleak *)newobject(memleak_class);

// this is my circ. buffer:
x->x_msgbuf = (t_atom *)sysmem_newptr(MSGBUFSIZE * sizeof(t_atom));
if (!x->x_msgbuf)
{
error("oops, out of memory");
return NULL;
}
x->x_msghead = 0;

return x;
}


June 2, 2006 | 6:20 pm

Hmmm. Upon initial inspection, I see no problems with your provided
code, and we’re able to allocate very large jitter matrix data
backing memory with sysmem_newptr(), so I don’t think that’s broken.
This would definitely be a problem if you were using getbytes(), but
that doesn’t seem to be the case. Your provided code also has no way
possible to introduce a memory leak, but it sounds like there is some
memory corruption taking place, leading to a crash.

Can you also let us know things like platform version, which compiler
you are using, etc. If you run your patch in the debugger (using
MaxRuntime), where exactly is it crashing?

Can you provide us with the patch you are using to generate crashes
with specific reproduction steps?

-Joshua


June 2, 2006 | 6:50 pm

Joshua Kit Clayton wrote:

> Hmmm. Upon initial inspection, I see no problems with your provided
> code, and we’re able to allocate very large jitter matrix data backing
> memory with sysmem_newptr(), so I don’t think that’s broken. This would
> definitely be a problem if you were using getbytes(), but that doesn’t
> seem to be the case.

Yes, I switched to sysmem_newptr because getbytes was having it’s memory
limits very low.

> Your provided code also has no way possible to
> introduce a memory leak, but it sounds like there is some memory
> corruption taking place, leading to a crash.
>
> Can you also let us know things like platform version, which compiler
> you are using, etc.

WinXP SP2, latest updates, Max/MSP 4.5.7 on a P4 1.8GHz with 512MB Ram.
Compiler is VC++ 6.0 (I know, it’s old). In case it turns out to be the
compiler I finally have a reason to update…

> If you run your patch in the debugger (using
> MaxRuntime), where exactly is it crashing?

It does not crash in Runtime… :-(
Memory usage (indicated by Task Manager) is still rising in the
beginning but it then stops to rise and does not crash.

> Can you provide us with the patch you are using to generate crashes
> with specific reproduction steps?

It’s the one below. Just turn on the metro and wait for the crash. The
faster the metro the sooner it crashes, looks like it needs a certain
(fixed) number of function calls to crash it.

Olaf

#P window setfont "Sans Serif" 9.;
#P window linecount 1;
#P message 134 171 104 9109513 send 1 hello number $1;
#P newex 134 124 71 9109513 random 5000;
#P toggle 134 63 15 0;
#P newex 134 93 40 9109513 metro 1;
#P newex 134 223 133 9109513 memleak;
#P connect 4 0 0 0;
#P connect 3 0 4 0;
#P connect 1 0 3 0;
#P connect 2 0 1 0;
#P window clipboard copycount 5;


June 2, 2006 | 8:42 pm

This sounds like scheduler backlog to me–i.e. you are generating
events faster than they can be serviced from the scheduler. This
would explain both the appearance of a memory leak and crashing (once
your backlog exceeds a certain threshold).

Let us know if it crashes with qmetro (low priority which will not
backlog at all).

-Joshua


June 2, 2006 | 9:03 pm

Yes, same effect (i.e. crash) with qmetro.

And since it doesn’t crash when I use malloc() instead of
sysmem_newptr() this can’t be backloging… It also crashes when I set
the metro (or qmetro) really slow, it just takes longer then.

Olaf


June 2, 2006 | 10:21 pm

On Jun 2, 2006, at 2:03 PM, Olaf Matthes wrote:

> Yes, same effect (i.e. crash) with qmetro.
>
> And since it doesn’t crash when I use malloc() instead of
> sysmem_newptr() this can’t be backloging… It also crashes when I
> set the metro (or qmetro) really slow, it just takes longer then.

Okay, thanks or the detailed info.

However, here’s the source for sysmem_newptr:

t_ptr sysmem_newptr(unsigned long size)
{
#ifdef WIN_VERSION
return (t_ptr) malloc(size);
#else
return (t_ptr) NewPtr(size);
#endif
}

Probably not crashing in the debug version since memory is a tiny bit
more padded. And perhaps your malloc call just happens to not be
close in memory with something important (using the VS6 c runtime,
and thus from a different c runtime memory pool than the one we use
in the kernel). In the absence of the scheduler backlog possibility,
I assume your write head is in fact exceeding the circular buffer
bounds somehow, though the code looks correct to me, and I can’t
reproduce the crash on OS X. I’ve verified that msghead is never >65535.

Since it is on windows, perhaps there is some inconsistency w/r/t
struct packing for the t_atom type? If you add post("atom size is %d,
bytes allocated are %d",sizeof(t_atom), MSGBUFSIZE * sizeof(t_atom));
to your constructor, what does it say? On Windows, atom size should
be 6, since 2 byte struct packing is defined.

-Joshua


June 3, 2006 | 8:46 am

Joshua Kit Clayton wrote:
>
> In the absence of the scheduler backlog possibility, I assume
> your write head is in fact exceeding the circular buffer bounds
> somehow, though the code looks correct to me, and I can’t reproduce the
> crash on OS X. I’ve verified that msghead is never >65535.

For me it was also never crashing on OS X, just Windows. Other people
also reported crashes (on Windows) when using my external, so the
problem seems to be compiled into the external already. I sent these
people the new version which calls malloc() directly and so far no
crashes any more.

> Since it is on windows, perhaps there is some inconsistency w/r/t
> struct packing for the t_atom type? If you add post("atom size is %d,
> bytes allocated are %d",sizeof(t_atom), MSGBUFSIZE * sizeof(t_atom));
> to your constructor, what does it say? On Windows, atom size should be
> 6, since 2 byte struct packing is defined.

It says "atom size is 6, bytes allocated are 393216" which seems to be
pretty correct.

Olaf


June 3, 2006 | 8:50 am

And what does sysmem_ptrsize() return, when called on your pointer?

jb


June 3, 2006 | 8:52 am

Hey Olaf,
I had this problem as well with the t_atom struct changing byte sizes
based on compiler settings. the Max SDK counts on structs being 4
byte aligned. Make your compiler do the 4byte alignment thing i.e. a
sizeof(t_atom) = 8 and all should be good.

best,
wes


June 3, 2006 | 8:59 am

Jeremy Bernstein wrote:

> And what does sysmem_ptrsize() return, when called on your pointer?

It also says 393216!

Olaf


June 10, 2006 | 8:05 am

Olaf, did you ever figure out the problem here?

jb


June 10, 2006 | 11:37 am

Jeremy Bernstein wrote:
> Olaf, did you ever figure out the problem here?

No, not yet. – I’ve been away to NIME at IRCAM, I think you were also there?

I’ll probably install VC++ 7.0 on another machine to see whetehr it’s a
compiler issue.

Olaf


June 10, 2006 | 11:57 am

Nope, Richard was there. I’m about 1000km to the northeast.

jb


June 10, 2006 | 12:00 pm

Jeremy Bernstein wrote:

> Nope, Richard was there.

Ah, okay, I just got told that some C74 guy was there.

> I’m about 1000km to the northeast.

Hhm, that’s about where I am at the moment. :-)
No, maybe I’m about 1200km northeast.

Olaf


June 10, 2006 | 3:56 pm

If you still haven’t figured it out, I’d be willing to bet it’s a
struct packing problem. Luke was at NIME.

wes


June 13, 2006 | 6:42 pm

Jeremy Bernstein wrote:

> Olaf, did you ever figure out the problem here?

I just tried on another machine with Visual Studio NET 2003 instead of
VC++ 6.0: same behaviour of the compiled external. As project settings I
used the .dsp file from the minimum example from the latest SDK, so I
don’t think it’s a wrong compiler setting (unless that example is wrong
as well).

Olaf


June 13, 2006 | 7:55 pm

Sorry, wrong observation (and silly me)… I forgot to remove the object
compiled with VC++ 6.0, so I was still loading the old one. With the
VS.NET compiled one memory usage increases in the beginning and then
stays constant, no more crashes. The increase in the beginning is
probably due to the fact that some memory to store t_symbols inside the
t_atoms gets allocated.

So, final result: compiler issue with VC++ 6.0, everything working okay
with C++.NET 2003.

Olaf


June 13, 2006 | 8:06 pm

Thanks for following up on this.

-Joshua


Viewing 23 posts - 1 through 23 (of 23 total)