Re: Having memory leak issues with perl-c

--000000000000bfa11505e4cca484
Content-Type: text/plain; charset="UTF-8"

On Wed, 27 Jul 2022 at 17:46, Mark Murawski <[email protected]>
wrote:

> Hi All!
>
> I'm working on a new feature for plperl in postgresql to populate some
> metadata in main::_FN for running plperl functions from postgres sql.
>
> I've snipped out some extra code that's unrelated to focus on the issue
> at hand.   If the section labeled 'Leaking Section' is entirely
> commented out (and of course the related SvREFCNT_dec_current is
> commented as well), then there's no memory issue at all.  If I do use
> this section of code, something is not freed and I'm not sure what that
> is, since I'm very new to perl-c
>
>
>
> /*
>   * Decrement the refcount of the given SV within the active Perl
> interpreter
>   *
>   * This is handy because it reloads the active-interpreter pointer, saving
>   * some notation in callers that switch the active interpreter.
>   */
> static inline void
> SvREFCNT_dec_current(SV *sv)
> {
>      dTHX;
>
>      SvREFCNT_dec(sv);
> }
>
> static SV  *
> plperl_call_perl_func(plperl_proc_desc *desc, FunctionCallInfo fcinfo)
> {
>      dTHX;
>      dSP;
>
>      HV                 *hv;                  // hash
>      SV                 *FNsv;                // scalar reference to the
> hash
>      SV                 *svFN;                // local reference to the
> hash?
>
>      ENTER;
>      SAVETMPS;
>
>      /* Give functions some metadata about what's going on in $_FN
> (Similar to $_TD for triggers) */
>
>      // Leaking Section {
>      FNsv = get_sv("main::_FN", GV_ADD);
>      save_item(FNsv);                        /* local $_FN */
>
>      hv = newHV(); // create new hash
>      hv_ksplit(hv, 12);                      /* pre-grow the hash */
>      hv_store_string(hv, "name", cstr2sv(desc->proname));
>
>      svFN = newRV_noinc((SV *) hv); // reference to the new hash
>      sv_setsv(FNsv, svFN);
>

So at this point FNsv and svFN both are references to the same HV. But you
dont free svFN.

Likely a simple SvREFCNT_dec(svFN) would do it.

Basically you are nearly doing this:

for my $FNsv ($main::FN) {
    my %hash;
    my $svFN= \%hash; push @LEAK, \$svFN;
    $FNsv= $svFN;
}

If you put a sv_2mortal(svFN) or an explicit refcount decrement on svFN
then I expect the leak would go away. Note the reason i use a for loop here
is I am trying to emphasize that $FNsv IS $main::FN, as the topic of a for
loop is an alias which is as close as you can get in Perl to the C level
operation of copying a pointer to an SV structure. (The difference being
that in Perl aliases are refcounted copies and C they are not refcounted).
Compare this to $svFN which is clearly a new variable.

What you should be doing is this:

my %hash;
$main::FN= \%hash;

Which would be something like this:


sv_upgrade(FNsv,SVtRV);
SvRV_set(FNsv,(SV*)newHV());
SvROK_on(FNsv);


     // Leaking Section }
>
>      // dostuff
>
>      SvREFCNT_dec_current(hv);
>
>      PUTBACK;
>      FREETMPS;
>      LEAVE;
>
>      ...snip...
> }
>
>
> If anyone would like to see the full context, I've attached the entire
> file.  My additions are between the 'New..........' sections
>
> My question is... the perl-c api docs do not make it clear for which
> allocations or accesses that you need to decrement the ref count.
>

I would say the docs actually do explain this. Most functions will specify
that you need to do the refcount incrementing yourself if it is relevant.

A general rule of thumb however is that any item you create yourself which
is not "owned by perl" by being attached to some data structure it exposes
is your problem to deal with. So for instance this:

     FNsv = get_sv("main::_FN", GV_ADD);

is getting you a local pointer to the structure representing $main::_FN
which is a global var. Thus it is stored in the global stash, and thus Perl
knows about it and will clean it up, you don't need to worry about its
refcount unless you store it in something else that is refcount managed.

On the other hand

     hv = newHV(); // create new hash

is a pointer to a new HV structure which is not stored in any place that
perl knows about. So if you dont arrange for it to be recount decremented
it will leak. However you then did this:

     svFN = newRV_noinc((SV *) hv); // reference to the new hash

this is creating a new reference to the hash. The combination of the two
basically is equivalent to creating an anonymous hashref. The "noinc" is
there because the hv starts off with a refcount of 1, and the new reference
is the thing that will "own" that refcount. So at this point you no longer
need to manage 'hv' provided you correctly manage svFN.

You then do this:

sv_setsv(FNsv, svFN);

This results in the refcount of 'hv' being incremented, as there are now
two RV's pointing at it. You need to free up the temporary, or even better,
simply dont use it. You dont need it, you can simply turn FNsv into an RV,
and then set its RV field appropriately, the leak will go away and the code
will be more efficient.

Another comment is regarding the hv_ksplit() which seems redundant. The
initial number of buckets is 8, the new size up is 12 or 16. Setting it to
12 is likely just a waste. Either set it much larger, or dont use it at all.

Yves



-- 
perl -Mre=debug -e "/just|another|perl|hacker/"

--000000000000bfa11505e4cca484
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr"><div dir=3D"ltr">On Wed, 27 Jul 2022 at 17:46, Mark Muraws=
ki &lt;<a href=3D"mailto:[email protected]">markm-lists@intellaso=
ft.net</a>&gt; wrote:<br></div><div class=3D"gmail_quote"><blockquote class=
=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rg=
b(204,204,204);padding-left:1ex">Hi All!<br>
<br>
I&#39;m working on a new feature for plperl in postgresql to populate some =
<br>
metadata in main::_FN for running plperl functions from postgres sql.<br>
<br>
I&#39;ve snipped out some extra code that&#39;s unrelated to focus on the i=
ssue <br>
at hand.=C2=A0=C2=A0 If the section labeled &#39;Leaking Section&#39; is en=
tirely <br>
commented out (and of course the related SvREFCNT_dec_current is <br>
commented as well), then there&#39;s no memory issue at all.=C2=A0 If I do =
use <br>
this section of code, something is not freed and I&#39;m not sure what that=
 <br>
is, since I&#39;m very new to perl-c<br>
<br>
<br>
<br>
/*<br>
=C2=A0=C2=A0* Decrement the refcount of the given SV within the active Perl=
 <br>
interpreter<br>
=C2=A0=C2=A0*<br>
=C2=A0=C2=A0* This is handy because it reloads the active-interpreter point=
er, saving<br>
=C2=A0=C2=A0* some notation in callers that switch the active interpreter.<=
br>
=C2=A0=C2=A0*/<br>
static inline void<br>
SvREFCNT_dec_current(SV *sv)<br>
{<br>
=C2=A0=C2=A0=C2=A0=C2=A0 dTHX;<br>
<br>
=C2=A0=C2=A0=C2=A0=C2=A0 SvREFCNT_dec(sv);<br>
}<br>
<br>
static SV=C2=A0 *<br>
plperl_call_perl_func(plperl_proc_desc *desc, FunctionCallInfo fcinfo)<br>
{<br>
=C2=A0=C2=A0=C2=A0=C2=A0 dTHX;<br>
=C2=A0=C2=A0=C2=A0=C2=A0 dSP;<br>
<br>
=C2=A0=C2=A0=C2=A0=C2=A0 HV=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 *hv;=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0 // hash<br>
=C2=A0=C2=A0=C2=A0=C2=A0 SV=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 *FNsv;=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 //=
 scalar reference to the <br>
hash<br>
=C2=A0=C2=A0=C2=A0=C2=A0 SV=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 *svFN;=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 //=
 local reference to the <br>
hash?<br>
<br>
=C2=A0=C2=A0=C2=A0=C2=A0 ENTER;<br>
=C2=A0=C2=A0=C2=A0=C2=A0 SAVETMPS;<br>
<br>
=C2=A0=C2=A0=C2=A0=C2=A0 /* Give functions some metadata about what&#39;s g=
oing on in $_FN <br>
(Similar to $_TD for triggers) */<br>
<br>
=C2=A0=C2=A0=C2=A0=C2=A0 // Leaking Section {<br>
=C2=A0=C2=A0=C2=A0=C2=A0 FNsv =3D get_sv(&quot;main::_FN&quot;, GV_ADD);<br=
>
=C2=A0=C2=A0=C2=A0=C2=A0 save_item(FNsv);=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /* local $_FN */<br>
<br>
=C2=A0=C2=A0=C2=A0=C2=A0 hv =3D newHV(); // create new hash<br>
=C2=A0=C2=A0=C2=A0=C2=A0 hv_ksplit(hv, 12);=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0 /* pre-grow the hash */<br>
=C2=A0=C2=A0=C2=A0=C2=A0 hv_store_string(hv, &quot;name&quot;, cstr2sv(desc=
-&gt;proname));<br>
<br>
=C2=A0=C2=A0=C2=A0=C2=A0 svFN =3D newRV_noinc((SV *) hv); // reference to t=
he new hash<br>
=C2=A0=C2=A0=C2=A0=C2=A0 sv_setsv(FNsv, svFN);<br></blockquote><div><br></d=
iv><div>So at this point FNsv and svFN both are references to the same HV. =
But you dont free svFN.</div><div><br></div><div>Likely a simple SvREFCNT_d=
ec(svFN) would do it.</div><div><br></div><div>Basically you are nearly doi=
ng this:</div><div><br></div><div>for my $FNsv ($main::FN) {</div><div>=C2=
=A0 =C2=A0 my %hash;</div><div>=C2=A0 =C2=A0 my $svFN=3D \%hash; push=C2=A0=
@LEAK, \$svFN;</div><div>=C2=A0 =C2=A0 $FNsv=3D $svFN;</div><div>}</div><di=
v><br></div><div>If you put a sv_2mortal(svFN) or an explicit refcount decr=
ement on svFN then I expect the leak would go away. Note the reason i use a=
 for loop here is I am trying to emphasize that $FNsv IS $main::FN, as the =
topic of a for loop is an alias which is as close as you can get in Perl to=
 the C level operation of copying a pointer to an SV structure. (The differ=
ence being that in Perl aliases are refcounted copies and C they are not re=
fcounted). Compare this to $svFN which is clearly a new variable.=C2=A0<br>=
</div><div><br></div><div><div>What you should be doing is this:</div><div>=
<br></div><div>my %hash;</div><div>$main::FN=3D \%hash;</div><div><br></div=
></div><div>Which would be something like this:</div><div><br></div><div><b=
r></div><div>sv_upgrade(FNsv,SVtRV);</div><div>SvRV_set(FNsv,(SV*)newHV());=
</div><div>SvROK_on(FNsv);</div><div><br></div><div><br></div><blockquote c=
lass=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px soli=
d rgb(204,204,204);padding-left:1ex">
=C2=A0=C2=A0=C2=A0=C2=A0 // Leaking Section }<br>
<br>
=C2=A0=C2=A0=C2=A0=C2=A0 // dostuff<br>
<br>
=C2=A0=C2=A0=C2=A0=C2=A0 SvREFCNT_dec_current(hv);<br>
<br>
=C2=A0=C2=A0=C2=A0=C2=A0 PUTBACK;<br>
=C2=A0=C2=A0=C2=A0=C2=A0 FREETMPS;<br>
=C2=A0=C2=A0=C2=A0=C2=A0 LEAVE;<br>
<br>
=C2=A0=C2=A0=C2=A0=C2=A0 ...snip...<br>
}<br>
<br>
<br>
If anyone would like to see the full context, I&#39;ve attached the entire =
<br>
file.=C2=A0 My additions are between the &#39;New..........&#39; sections<b=
r>
<br>
My question is... the perl-c api docs do not make it clear for which <br>
allocations or accesses that you need to decrement the ref count.<br></bloc=
kquote><div><br></div><div>I would say the docs actually do explain this. M=
ost functions will specify that you need to do the refcount incrementing yo=
urself if it is relevant.=C2=A0</div><div><br></div><div>A general rule of =
thumb however is that any item you create yourself which is not &quot;owned=
 by perl&quot; by being attached to some data structure it exposes is your =
problem to deal with. So for instance this:</div><div><br></div><div>=C2=A0=
 =C2=A0 =C2=A0FNsv =3D get_sv(&quot;main::_FN&quot;, GV_ADD);<br></div><div=
><br></div><div>is getting you a local pointer to the structure representin=
g $main::_FN which is a global var. Thus it is stored in the global stash, =
and thus Perl knows about it and will clean it up, you don&#39;t need to wo=
rry about its refcount unless you store it in something else that is refcou=
nt managed.</div><div><br></div><div>On the other hand</div><div><br></div>=
<div>=C2=A0 =C2=A0 =C2=A0hv =3D newHV(); // create new hash=C2=A0<br></div>=
<div><br></div><div>is a pointer to a new HV structure which is not stored =
in any place that perl knows about. So if you dont arrange for it to be rec=
ount decremented it will leak. However you then did this:</div><div><br></d=
iv><div>=C2=A0 =C2=A0 =C2=A0svFN =3D newRV_noinc((SV *) hv); // reference t=
o the new hash<br></div><div><br></div><div>this is creating a new referenc=
e to the hash. The combination of the two basically is equivalent to creati=
ng an anonymous hashref. The &quot;noinc&quot; is there because the hv star=
ts off with a refcount of 1, and the new reference is the thing that will &=
quot;own&quot; that refcount. So at this point you no longer need to manage=
 &#39;hv&#39; provided you correctly manage svFN.=C2=A0</div><div><br></div=
><div>You then do this:</div><div><br></div><div>sv_setsv(FNsv, svFN);</div=
><div><div><br></div></div><div>This results in the refcount of &#39;hv&#39=
; being incremented, as there are now two RV&#39;s pointing at it. You need=
 to free up the temporary, or even better, simply dont use it. You dont nee=
d it, you can simply turn FNsv into an RV, and then set its RV field approp=
riately, the leak will go away and the code will be more efficient.=C2=A0</=
div><div><br></div><div>Another comment is regarding the hv_ksplit() which =
seems redundant. The initial number of buckets is 8, the new size up is 12 =
or 16. Setting it to 12 is likely just a waste. Either=C2=A0set it much lar=
ger, or dont use it at all.</div><div><br></div><div>Yves</div><div><br></d=
iv><div><br></div><div><br></div></div>-- <br><div dir=3D"ltr" class=3D"gma=
il_signature">perl -Mre=3Ddebug -e &quot;/just|another|perl|hacker/&quot;</=
div></div>

--000000000000bfa11505e4cca484--