Re: Having memory leak issues with perl-c
On Wed, 27 Jul 2022 at 17:46, Mark Murawski
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_ksp
Re: Having memory leak issues with perl-c
On 7/27/22 13:20, demerphq wrote:
On Wed, 27 Jul 2022 at 17:46, Mark Murawski
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
