On 7/27/22 13:20, demerphq wrote:
On Wed, 27 Jul 2022 at 17:46, Mark Murawski
<markm-li...@intellasoft.net> 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/"
Spectacular. I'll be reviewing and making changes that you're
suggesting and this helps my understanding for sure.
Thanks!
