Re: Having memory leak issues with perl-c
Mark Murawski <[email protected]> Wed, 27 Jul 2022 16:41:52 -0400
| Newsgroups | gmane.comp.lang.perl.quality-assurance |
|---|---|
| Message-ID | <[email protected]> |
--------------4P8vVSdBRxTDlkVOcXgnF0Rd Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit On 7/27/22 13:20, demerphq wrote: > On Wed, 27 Jul 2022 at 17:46, Mark Murawski > <[email protected]> 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! --------------4P8vVSdBRxTDlkVOcXgnF0Rd 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> <br> <br> <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"> <div dir="ltr"> <div dir="ltr">On Wed, 27 Jul 2022 at 17:46, Mark Murawski <<a href="mailto:[email protected]" moz-do-not-send="true" class="moz-txt-link-freetext">[email protected]</a>> wrote:<br> </div> <div class="gmail_quote"> <blockquote class="gmail_quote">Hi All!<br> <br> I'm working on a new feature for plperl in postgresql to populate some <br> metadata in main::_FN for running plperl functions from postgres sql.<br> <br> I've snipped out some extra code that's unrelated to focus on the issue <br> at hand. If the section labeled 'Leaking Section' is entirely <br> commented out (and of course the related SvREFCNT_dec_current is <br> commented as well), then there's no memory issue at all. If I do use <br> this section of code, something is not freed and I'm not sure what that <br> is, since I'm very new to perl-c<br> <br> <br> <br> /*<br> * Decrement the refcount of the given SV within the active Perl <br> interpreter<br> *<br> * This is handy because it reloads the active-interpreter pointer, saving<br> * some notation in callers that switch the active interpreter.<br> */<br> static inline void<br> SvREFCNT_dec_current(SV *sv)<br> {<br> dTHX;<br> <br> SvREFCNT_dec(sv);<br> }<br> <br> static SV *<br> plperl_call_perl_func(plperl_proc_desc *desc, FunctionCallInfo fcinfo)<br> {<br> dTHX;<br> dSP;<br> <br> HV *hv; // hash<br> SV *FNsv; // scalar reference to the <br> hash<br> SV *svFN; // local reference to the <br> hash?<br> <br> ENTER;<br> SAVETMPS;<br> <br> /* Give functions some metadata about what's going on in $_FN <br> (Similar to $_TD for triggers) */<br> <br> // Leaking Section {<br> FNsv = get_sv("main::_FN", GV_ADD);<br> save_item(FNsv); /* local $_FN */<br> <br> hv = newHV(); // create new hash<br> hv_ksplit(hv, 12); /* pre-grow the 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> </blockquote> <div><br> </div> <div>So at this point FNsv and svFN both are references to the same HV. But you dont free svFN.</div> <div><br> </div> <div>Likely a simple SvREFCNT_dec(svFN) would do it.</div> <div><br> </div> <div>Basically you are nearly doing this:</div> <div><br> </div> <div>for my $FNsv ($main::FN) {</div> <div> my %hash;</div> <div> my $svFN= \%hash; push @LEAK, \$svFN;</div> <div> $FNsv= $svFN;</div> <div>}</div> <div><br> </div> <div>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. <br> </div> <div><br> </div> <div> <div>What you should be doing is this:</div> <div><br> </div> <div>my %hash;</div> <div>$main::FN= \%hash;</div> <div><br> </div> </div> <div>Which would be something like this:</div> <div><br> </div> <div><br> </div> <div>sv_upgrade(FNsv,SVtRV);</div> <div>SvRV_set(FNsv,(SV*)newHV());</div> <div>SvROK_on(FNsv);</div> <div><br> </div> <div><br> </div> <blockquote class="gmail_quote"> // Leaking Section }<br> <br> // dostuff<br> <br> SvREFCNT_dec_current(hv);<br> <br> PUTBACK;<br> FREETMPS;<br> LEAVE;<br> <br> ...snip...<br> }<br> <br> <br> If anyone would like to see the full context, I've attached the entire <br> file. My additions are between the 'New..........' sections<br> <br> My question is... the perl-c api docs do not make it clear for which <br> allocations or accesses that you need to decrement the ref count.<br> </blockquote> <div><br> </div> <div>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. </div> <div><br> </div> <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> <div><br> </div> <div><br> </div> </div> -- <br> <div dir="ltr" class="gmail_signature">perl -Mre=debug -e "/just|another|perl|hacker/"</div> </div> </blockquote> <br> <br> Spectacular. I'll be reviewing and making changes that you're suggesting and this helps my understanding for sure.<br> <br> Thanks!<br> <br> <br> <br> </body> </html> --------------4P8vVSdBRxTDlkVOcXgnF0Rd--