Re: Am I leaking memory?

Desree Sowers Wed, 10 Dec 2003 04:01:24 -0800

Hi Tassilo, and thanks for the prompt reply,

I tried your changes and they seemed to work better than what I had.

My only problem is that I have 65+Mb files that I can read in a couple of seconds, but I have 200Mb files that take over 4 minutes(!). I suspect the problem is in the fread statements. I seem to be spending more time on those than anything else.

Do you have any thoughts?

-Thanks for the help,

Doug

From: Tassilo von Parseval <[EMAIL PROTECTED]>
Reply-To: [EMAIL PROTECTED]
To: Desree Sowers <[EMAIL PROTECTED]>
CC: [EMAIL PROTECTED]
Subject: Re: Am I leaking memory?
Date: Tue, 09 Dec 2003 20:39:21 +0100

On Tue, Dec 09, 2003 at 04:09:25AM -0700 Desree Sowers wrote:

> I have written (with some pointers from Nick Ing-Simmons -- thanks Nick), > some > code intended to read in large (~200+Mb)files. The relevant part of the XS > of > the code read like this:

> AV *AoA; /* Array of strings */
> int param_size;
> int paramater_count = 0;
> char *bufferX, *bufferY;
> while (!feof(file)){
>  /* read in paramater name */
>  if(!feof(file)) {
>    fread(&param_name, PARAM_NAME, 1, file); /*PARAM_NAME is define'd.*/
>    /* now get size of array */
>    fread(&param_size, sizeof(int),1,file);
>    AoA = newAV();
>    av_extend(AoA, 3);
>    av_store(AoA, 0, newSViv(param_size);
>    bufferX = (char*)malloc(sizeof(double) *param_size);
>    bufferY = (char*)malloc(sizeof(double) *param_size);

Better use the macros provided by the perlapi here:

    New(0, bufferX, param_size, double);
    New(0, bufferY, param_size, double);

Besides, why don't you make bufferX and bufferY a pointer to a double if
that's what they are going to store?

>    numbersX = newSVpv("",0);
>    numbersY = newSVpv("",0);
>
>    SvGROW(numbersX, sizeof(double)*param_size);
>    SvGROW(numbersY, sizeof(double)*param_size);

This looks cumbersome. I think you can scratch those four lines
altogether.

>    /* read in the array of doubles */
>    fread(bufferX, sizeof(double)*param_size, 1,file);
>    fread(bufferY, sizeof(double)*param_size, 1,file);
>
>    /* copy them to the SV -- this means they will need to be*/
>    /* unpacked later */
>    sv_catpvn(numbersX, bufferX, sizeof(double)*param_size);
>    sv_catpvn(numbersY, bufferY, sizeof(double)*param_size);

I'd do the whole filling of numbers(X|Y) like this:

    numbersX = sv_newmortal();
    numbersY = sv_newmortal();
    sv_usepvn(numbersX, (char*)bufferX, sizeof(double) * param_size);
    sv_usepvn(numbersY, (char*)bufferY, sizeof(double) * param_size);

sv_usepvn() will use an existing buffer instead of copying the data.
This laters saves you freeing the original buffer (which you didn't do,
btw, and there was your mem-leak).

>    av_store(AoA, 1, numbersX);
>    av_store(AoA, 2, numbersY);

numbersX and numbersY are now mortals, so you need to increment their
ref-count:

    if (av_store(AoA, 1, numbersX))
        SvREFCNT_inc(numbersX);
    if (av_store(AoA, 2, numbersY))
        SvREFCNT_inc(numbersY);

> XPUSHs(sv_2mortal(newSVpv,param_name,strlen(param_name)));

How's param_name allocated? The above is ok when you use static memory
for it. But when you dynamically allocate it, you need to free it after
this XPUSHs() because newSVpv() copies its buffer:

Safefree(param_name);

Use Safefree when you allocate memory with New, otherwise use free().

>    XPUSHs(sv_2mortal(newRV_noinc(SV*)AoA));
>    paramater_count++;
>  }
> }
> fclose(file);
> XSRETURN((paramater_count *2));

Tassilo
--
$_=q#",}])!JAPH!qq(tsuJ[{@"tnirp}3..0}_$;//::niam/s~=)]3[))_$-3(rellac(=_$({
pam{rekcahbus})(rekcah{lrePbus})(lreP{rehtonabus})!JAPH!qq(rehtona{tsuJbus#;
$_=reverse,s+(?<=sub).+q#q!'"qq.\t$&."'!#+sexisexiixesixeseg;y~\n~~dddd;eval

_________________________________________________________________ Take advantage of our best MSN Dial-up offer of the year � six months @$9.95/month. Sign up now! http://join.msn.com/?page=dept/dialup

Re: Am I leaking memory?

Reply via email to