Front page | perl.qa |
Postings from July 2022
Re: Having memory leak issues with perl-c
Thread Previous
|
Thread Next
From:
Mark Murawski
Date:
July 27, 2022 20:41
Subject:
Re: Having memory leak issues with perl-c
Message ID:
c159c861-4550-6536-c6ca-8d32ba1addce@intellasoft.net
On 7/27/22 13:20, demerphq wrote:
> On Wed, 27 Jul 2022 at 17:46, Mark Murawski
> <markm-lists@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!
Thread Previous
|
Thread Next