Re: Having memory leak issues with perl-c
On 8/12/22 18:26, Mark Murawski wrote:
On 8/12/22 18:11, Mark Murawski wrote:
On 8/4/22 15:10, demerphq wrote:
On Thu, 4 Aug 2022 at 17:04, Mark Murawski
wrote:
On 8/4/22 02:50, demerphq wrote:
On Thu, 4 Aug 2022 at 01:58, Mark Murawski
wrote:
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 */
I dont get the sequence here. You take the old value of
$main::_FN and then you localize it after you fetch it? That
seems weird.
You did not respond to this comment ^^
The reason for the save_item*( was because I was modeling this code
on another section that uses save_item() to accomplish something
similar (to send internal postgres details to some perl). I don't
know why the original author uses save_item() for this purpose.
Oh... I know why save_item is used. It's because this code can be
executed multiple times in the same perl process. So each one needs
it's own _FN
After a serious amount of trial and error... I got it to stop crashing
and I fixed the leak
HV *hv; // hash
SV *FNsv; // scalar reference to the hash
SV *SVhv;
ENTER;
SAVETMPS;
FNsv = get_sv("main::_FN", GV_ADD);
save_item(FNsv); /* local $_FN */
hv = newHV(); // create new hash
SVhv = newRV_noinc((SV *) hv);
sv_setsv(FNsv, SVhv);
hv_store_string(hv, "name", cstr2sv(desc->proname));
PUTBACK;
FREETMPS;
LEAVE;
SvREFCNT_dec_current(SVhv);
The fix I found was to put SvREFCNT_dec_current *after*
PUTBACK/FREETMPS/LEAVE
if you do the SvREFCNT_dec_current prior.. then it leaks
If the trailing end of the code looks like this... you get a leak
SvREFCNT_dec_current(SVhv);
PUTBACK;
FREETMPS;
LEAVE;
Re: Having memory leak issues with perl-c
On 8/12/22 18:11, Mark Murawski wrote:
On 8/4/22 15:10, demerphq wrote:
On Thu, 4 Aug 2022 at 17:04, Mark Murawski
wrote:
On 8/4/22 02:50, demerphq wrote:
On Thu, 4 Aug 2022 at 01:58, Mark Murawski
wrote:
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 */
I dont get the sequence here. You take the old value of
$main::_FN and then you localize it after you fetch it? That
seems weird.
You did not respond to this comment ^^
The reason for the save_item*( was because I was modeling this code on
another section that uses save_item() to accomplish something similar
(to send internal postgres details to some perl). I don't know why
the original author uses save_item() for this purpose.
Oh... I know why save_item is used. It's because this code can be
executed multiple times in the same perl process. So each one needs
it's own _FN
Re: Having memory leak issues with perl-c
On 8/4/22 15:10, demerphq wrote:
On Thu, 4 Aug 2022 at 17:04, Mark Murawski
wrote:
On 8/4/22 02:50, demerphq wrote:
On Thu, 4 Aug 2022 at 01:58, Mark Murawski
wrote:
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 */
I dont get the sequence here. You take the old value of
$main::_FN and then you localize it after you fetch it? That
seems weird.
You did not respond to this comment ^^
The reason for the save_item*( was because I was modeling this code on
another section that uses save_item() to accomplish something similar
(to send internal postgres details to some perl). I don't know why the
original author uses save_item() for this purpose.
No, you shouldn't do anything after creating the hash that might die
until you have arranged for hv to be freed. Storing into the hash
might die.
Got it.
Obviously in perl we can write:
my %hash;
$main::_FN= \%hash;
And in XS we can do the same thing. Unfortunately there isn't a
utility sub to do this currently, it has been on my TODO list to
add one for some time but lack of round tuits and all that.
You want code something like this:
sv_clear(FNsv); /* undef the sv */
sv_upgrade(FNsv,SVt_RV);
SvRV_set(FNsv, (SV*)hv);
SvROK_on(FNsv);
Again, make liberal use of sv_dump() it is the XS version of
Data::Dumper more or less.
Also... i get a crash when I use sv_clear(FNsv); right away like this.
And what does FNsv look like immediately before you call sv_clear()?
I'm on vacation and I'm working on this as I get the time I wasn't
able to attach gdb and get anything meaningful, but based on the newly
added debugs you can tell where it's crashing
Can you please show a reduced version of the code? And explain why you
are doing save_item(FNsv)? And provide some of the output of sv_dump()?
save_item... details above..
And trimmed new code:
// New .. {
FNsv = get_sv("main::_FN", GV_ADD);
if (!FNsv) {
ereport(ERROR,
(errcode(ERRCODE_EXTERNAL_ROUTINE_EXCEPTION),
errmsg("couldn't fetch $_FN")));
}
fprintf(stderr, "POST get_sv\n");
sv_dump(FNsv);
save_item(FNsv); /* local $_FN */
hv = newHV(); // create new hash
fprintf(stderr, "PRE sv_clear/sv_upgrade/SvRV_set/SvROK_on\n");
fprintf(stderr, "PRE sv_clear\n");
sv_clear(FNsv); /* undef the sv */
fprintf(stderr, "POST sv_clear\n");
sv_dump(FNsv);
fprintf(stderr, "PRE sv_upgrade svtype: %d, %d %d\n", SvTYPE(FNsv),
SVt_NULL, SVt_PVIO);
sv_upgrade(FNsv,SVt_RV);
//SvUPGRADE(FNsv, SVt_RV);
fprintf(stderr, "POST sv_upgrade\n");
sv_dump(FNsv);
fprintf(stderr, "PRE sv_set/on\n");
SvRV_set(FNsv, (SV*)hv);
SvROK_on(FNsv);
fprintf(stderr, "POST sv_set/on\n");
sv_dump(FNsv);
fprintf(stderr, "POST sv_clear/sb_upgrade/SvRV_set/SvROK_on\n");
sv_dump(FNsv);
// Anything below might die().. so we do all our setup above
hv_store_string(hv, "name", cstr2sv(desc->proname));
// new .. }
Output:
POST get_sv
SV = NULL(0x0) at 0x55adeea14e40
REFCNT = 1
FLAGS = ()
PRE sv_clear/sv_upgrade/SvRV_set/SvROK_on
PRE sv_clear
POST sv_clear
SV = UNKNOWN(0xff) (0x0) at 0x55adeea14e40
REFCNT = 1
FLAGS = ()
PRE sv_upgrade svtype: 255, 0 15
sv_upgrade from type 255 down to type 1.
2022-08-12 17:56:57 EDT - - - - 7387 - - 0 - LOG: server process
(PID 11614) was terminated by signal 11: Segmentation fault
Basically my gdb session is useless and results in this:
Program terminated with signal SIGSEGV, Segmentation fault.
#0 0x7fcf6ae24c7f in ?? ()
(gdb) bt
#0 0x7fcf6ae24c7f in ?? ()
Backtrace stopped: Cannot access memory at address 0x7ffce563e4b0
Thanks!
Re: Having memory leak issues with perl-c
On Thu, 4 Aug 2022 at 17:04, Mark Murawski
wrote:
> On 8/4/22 02:50, demerphq wrote:
>
> On Thu, 4 Aug 2022 at 01:58, Mark Murawski
> wrote:
>
>> 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 */
>>
>
> I dont get the sequence here. You take the old value of $main::_FN and
> then you localize it after you fetch it? That seems weird.
>
>
You did not respond to this comment ^^
>
>
>>
>> hv = newHV(); // create new hash
>> hv_store_string(hv, "name", cstr2sv(desc->proname));
>>
>
> Really you shouldnt do this until you have safely managed the refcounts of
> all your newly created objects so that if this die's nothing leaks.
>
>
> I take this to mean , setting up FNsv first, and then allocating hv? But
> in this case we seem to have a chicken/egg problem? How can you set up
> FNsv to point to hv without first setting up hv?
>
No, you shouldn't do anything after creating the hash that might die until
you have arranged for hv to be freed. Storing into the hash might die.
>
> WARNING: Attempt to free unreferenced scalar: SV 0x55d5b1cf6480, Perl
>> interpreter: 0x55d5b17226c0
>>
>
> Why are you decrementing hv? You dont own hv anymore, it's owned by svFN
> and after the sv_setsv() call also FNsv. You shouldnt mess with its
> refcount anymore.
>
>
> The ownership aspect is making more sense now, thanks for clarifying.
>
YW.
>
> Obviously in perl we can write:
>
> my %hash;
> $main::_FN= \%hash;
>
> And in XS we can do the same thing. Unfortunately there isn't a utility
> sub to do this currently, it has been on my TODO list to add one for some
> time but lack of round tuits and all that.
>
> You want code something like this:
>
> sv_clear(FNsv); /* undef the sv */
> sv_upgrade(FNsv,SVt_RV);
> SvRV_set(FNsv, (SV*)hv);
> SvROK_on(FNsv);
>
> Again, make liberal use of sv_dump() it is the XS version of Data::Dumper
> more or less.
>
>
> I have been playing with sv_dump()... At the end of this flow, the
> refcount to FNsv is 1 and should get automatically cleaned up by Perl,
> right?
>
Well I dont know for sure as I dont know the latest state of your code. But
my thinking is that you asked for $main::_FN which is in the package table,
so perl should clean it up. But I dont understand why you are using
save_item(FNsv).
> I still have a leak here, using the above code.
>
Probably I would be able to help you better if you posted the latest state
of your code, with sv_dump() calls liberally sprinkled through the code,
and post the output as well.
>
> Also... i get a crash when I use sv_clear(FNsv); right away like this.
>
And what does FNsv look like immediately before you call sv_clear()?
> If I take it out, the code seems to all run correctly, but I have a leak
> and the hash or the hash reference is not being cleaned up.
>
Can you please show a reduced version of the code? And explain why you are
doing save_item(FNsv)? And provide some of the output of sv_dump()?
cheers,
Yves
--
perl -Mre=debug -e "/just|another|perl|hacker/"
Re: Having memory leak issues with perl-c
On 8/4/22 02:50, demerphq wrote:
On Thu, 4 Aug 2022 at 01:58, Mark Murawski
wrote:
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 */
I dont get the sequence here. You take the old value of $main::_FN and
then you localize it after you fetch it? That seems weird.
hv = newHV(); // create new hash
hv_store_string(hv, "name", cstr2sv(desc->proname));
Really you shouldnt do this until you have safely managed the
refcounts of all your newly created objects so that if this die's
nothing leaks.
I take this to mean , setting up FNsv first, and then allocating hv?
But in this case we seem to have a chicken/egg problem? How can you set
up FNsv to point to hv without first setting up hv?
WARNING: Attempt to free unreferenced scalar: SV 0x55d5b1cf6480,
Perl interpreter: 0x55d5b17226c0
Why are you decrementing hv? You dont own hv anymore, it's owned by
svFN and after the sv_setsv() call also FNsv. You shouldnt mess with
its refcount anymore.
The ownership aspect is making more sense now, thanks for clarifying.
Obviously in perl we can write:
my %hash;
$main::_FN= \%hash;
And in XS we can do the same thing. Unfortunately there isn't a
utility sub to do this currently, it has been on my TODO list to add
one for some time but lack of round tuits and all that.
You want code something like this:
sv_clear(FNsv); /* undef the sv */
sv_upgrade(FNsv,SVt_RV);
SvRV_set(FNsv, (SV*)hv);
SvROK_on(FNsv);
Again, make liberal use of sv_dump() it is the XS version of
Data::Dumper more or less.
I have been playing with sv_dump()... At the end of this flow, the
refcount to FNsv is 1 and should get automatically cleaned up by Perl,
right? I still have a leak here, using the above code.
Also... i get a crash when I use sv_clear(FNsv); right away like this.
If I take it out, the code seems to all run correctly, but I have a leak
and the hash or the hash reference is not being cleaned up.
Thanks.
Re: Having memory leak issues with perl-c
On Thu, 4 Aug 2022 at 01:58, Mark Murawski
wrote:
> 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 */
>
I dont get the sequence here. You take the old value of $main::_FN and then
you localize it after you fetch it? That seems weird.
>
> hv = newHV(); // create new hash
> hv_store_string(hv, "name", cstr2sv(desc->proname));
>
Really you shouldnt do this until you have safely managed the refcounts of
all your newly created objects so that if this die's nothing leaks.
>
> 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
>
Why are you decrementing hv? You dont own hv anymore, it's owned by svFN
and after the sv_setsv() call also FNsv. You shouldnt mess with its
refcount anymore.
HV *hv= newHV(); /* until this is attached to something that will get
cleaned up you need to deal with its refcnt */
svFN = newRV_noinc((SV *) hv); /* now the hv is owned by svFN, we no longer
have to worry about hv, just svFN */
sv_setsv(FNsv, svFN); /* now FNsv is a copy of the ref that is in svFN */
So at this point we have two SV's which reference hv, svFN and FNsv, hv
will have a refcount of 2, svFN should have a refcount of 1.
SvREFCNT_dec(svFN);
This will decrement svFN to 0, which will cause it to be freed, but not
before Perl walks the reference tree decrementing the things the reference
contains, so this will trigger an automatic SvREFCNT_dec() on hv. So after
this line the refcount of hv would be 1, and it would be owned by FNsv only.
Btw, if you liberally added sv_dump(svFN); sv_dump(hv); and sv_dump(FNsv);
to your code you would see what it is happening here. It will show you the
refcounts of each object.
> 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);
>
No. That creates a new SV which is a reference to a hv.
Your code does this basically:
my %hash;
my $ref= \%hash;
$main::_FN= $ref;
Obviously in perl we can write:
my %hash;
$main::_FN= \%hash;
And in XS we can do the same thing. Unfortunately there isn't a utility sub
to do this currently, it has been on my TODO list to add one for some time
but lack of round tuits and all that.
You want code something like this:
sv_clear(FNsv); /* undef the sv */
sv_upgrade(FNsv,SVt_RV);
SvRV_set(FNsv, (SV*)hv);
SvROK_on(FNsv);
and you are done. It's possible that the sv_upgrade() is unnecessary as I
believe every SV is big enough to handle SVr_RV, try with and without,
keeping will make your code more backwards compatible to really old
versions of perl. But in anything modern an RV is a bodyless SV, and thus
every SV can turn into one without allocating a new body and the sv_upgrade
should be superfluous.
Again, make liberal use of sv_dump() it is the XS version of Data::Dumper
more or less.
Yves
--
perl -Mre=debug -e "/just|another|perl|hacker/"
Re: Having memory leak issues with perl-c
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.
Re: Having memory leak issues with perl-c
On Thu, 28 Jul 2022 at 10:35, demerphq wrote: > On Wed, 27 Jul 2022 at 22:41, Mark Murawski > wrote: > >> >> >> On 7/27/22 13:20, demerphq wrote: >> >> Spectacular. I'll be reviewing and making changes that you're suggesting >> and this helps my understanding for sure. >> > > I was a bit off my game yesterday. I should have told you to make liberal > use of sv_dump() when you are debugging. That will help you visualize what > is going on. It's the XS equivalent to the Dump function from Devel::Peek > (or more accurately it is what the Dump functions uses under the hood). > > You can pass any SV like structure (Eg, HV, AV, etc) into it with casting > and it will dump out all the useful details, including refcounts. > > Good luck! > Last comment. You might find the code in Sereal::Encoder and Sereal::Decoder helpful. Sereal is a serialization package I maintain, and as such it contains all the code for serializing perl data structures and then reconstituting them. It was written in a style where it avoids mortal values as much as possible, where it mortalizes the root of the data structure being deserialized and then does not mortalize anything else (barring state data), and passes down the variables being "filled in" (usually called the SV *into). So for instance if it is deserializing an array ref, it first creates an SV with refcount 1, it then passes that into the code that parses array refs, which turns the into into an SvRV pointing at a newAV, then for each element in the array it creates a newSV, pushes it into the array, and then calls the parse logic with that new element as the into argument, etc. Thus it bypasses all of the recount twiddling. If the code dies in the middle the root is mortal so Perl will free it later. A mortal pattern might look like this: HV *sv= newSV(0); sv_2mortal(sv); /* if we die this will get freed when perl cleans up later! */ /* do stuff that could die */ ... /* Yay, we didn't die! increment sv's refcount to demortalize it and then return it */ SvREFCOUNT_inc(sv); return sv; Sorry, just trying to arm you with as much useful info as I can. I happened to be working on very similar code the last day or two for Sereal. :-) Yves -- perl -Mre=debug -e "/just|another|perl|hacker/"
Re: Having memory leak issues with perl-c
On Wed, 27 Jul 2022 at 22:41, Mark Murawski wrote: > > > On 7/27/22 13:20, demerphq wrote: > > Spectacular. I'll be reviewing and making changes that you're suggesting > and this helps my understanding for sure. > I was a bit off my game yesterday. I should have told you to make liberal use of sv_dump() when you are debugging. That will help you visualize what is going on. It's the XS equivalent to the Dump function from Devel::Peek (or more accurately it is what the Dump functions uses under the hood). You can pass any SV like structure (Eg, HV, AV, etc) into it with casting and it will dump out all the useful details, including refcounts. Good luck! cheers, Yves -- perl -Mre=debug -e "/just|another|perl|hacker/"
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
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
