Re: Having memory leak issues with perl-c
[email protected] (Mark Murawski) Wed, 3 Aug 2022 19:58:26 -0400
| Newsgroups | perl.qa |
|---|---|
| Message-ID | <[email protected]> |
--------------7tsh7wS5LSFoQJGhF9MiD080
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
On 7/27/22 13:20, demerphq wrote:
>
> 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
>
>
I'm still not getting something... if I want to fix the code-as-is and
do this:
FNsv = get_sv("main::_FN", GV_ADD);
if (!FNsv)
ereport(ERROR,
(errcode(ERRCODE_EXTERNAL_ROUTINE_EXCEPTION),
errmsg("couldn't fetch $_FN")));
save_item(FNsv); /* local $_FN */
hv = newHV(); // create new hash
hv_store_string(hv, "name", cstr2sv(desc->proname));
svFN = newRV_noinc((SV *) hv); // reference to the new hash
sv_setsv(FNsv, svFN);
// dostuff
SvREFCNT_dec_current(svFN);
SvREFCNT_dec_current((SV *) hv);
You're saying that the sv_setsv(FNsv, svFN); creates a second ref...
so in theory I can unref it and then all else would be equal
but I get this:
WARNING: Attempt to free unreferenced scalar: SV 0x55d5b1cf6480, Perl
interpreter: 0x55d5b17226c0.
Also.. something I didn't follow was this:
"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. "
How do you turn an SV into an RV without creating this extra reference..
Aren't I doing this already, with:
svFN = newRV_noinc((SV *) hv);
Thanks.
--------------7tsh7wS5LSFoQJGhF9MiD080
Content-Type: text/html; charset=UTF-8
Content-Transfer-Encoding: 8bit
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<div class="moz-cite-prefix">On 7/27/22 13:20, demerphq wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CANgJU+XSXiUG0W5wyU-B11k4YD+WootT2LiXwTWYkZTHW7hp+w@mail.gmail.com">
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
<div dir="ltr"><br>
<div class="gmail_quote">
<div>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:</div>
<div><br>
</div>
<div> FNsv = get_sv("main::_FN", GV_ADD);<br>
</div>
<div><br>
</div>
<div>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.</div>
<div><br>
</div>
<div>On the other hand</div>
<div><br>
</div>
<div> hv = newHV(); // create new hash <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 recount decremented it will leak. However you then
did this:</div>
<div><br>
</div>
<div> svFN = newRV_noinc((SV *) hv); // reference to the
new hash<br>
</div>
<div><br>
</div>
<div>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. </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 '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. </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 set it much larger, or dont use it at all.</div>
<div><br>
</div>
<div>Yves</div>
<div><br>
</div>
<br>
</div>
</div>
</blockquote>
<br>
<br>
I'm still not getting something... if I want to fix the code-as-is
and do this:<br>
<br>
FNsv = get_sv("main::_FN", GV_ADD);<br>
if (!FNsv)<br>
ereport(ERROR,<br>
(errcode(ERRCODE_EXTERNAL_ROUTINE_EXCEPTION),<br>
errmsg("couldn't fetch $_FN")));<br>
<br>
save_item(FNsv); /* local $_FN */<br>
<br>
hv = newHV(); // create new hash<br>
hv_store_string(hv, "name", cstr2sv(desc->proname));<br>
<br>
svFN = newRV_noinc((SV *) hv); // reference to the new hash<br>
sv_setsv(FNsv, svFN);<br>
<br>
// dostuff<br>
<br>
SvREFCNT_dec_current(svFN);<br>
SvREFCNT_dec_current((SV *) hv);<br>
<br>
<br>
You're saying that the sv_setsv(FNsv, svFN); creates a second
ref... so in theory I can unref it and then all else would be equal<br>
but I get this:<br>
<br>
WARNING: Attempt to free unreferenced scalar: SV 0x55d5b1cf6480,
Perl interpreter: 0x55d5b17226c0.<br>
<br>
<br>
Also.. something I didn't follow was this:<br>
"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. "<br>
<br>
How do you turn an SV into an RV without creating this extra
reference..<br>
Aren't I doing this already, with: <br>
svFN = newRV_noinc((SV *) hv);<br>
<br>
<br>
Thanks.<br>
<br>
</body>
</html>
--------------7tsh7wS5LSFoQJGhF9MiD080--