Re: Having memory leak issues with perl-c
[email protected] (demerphq) Thu, 4 Aug 2022 08:50:51 +0200
| Newsgroups | perl.qa |
|---|---|
| Message-ID | <CANgJU+U0B9-2LN0QS1w7r6CR+VdFYhSMdzidy2cq4aqSzWv6qA@mail.gmail.com> |
--000000000000d7746305e564c580 Content-Type: text/plain; charset="UTF-8" On Thu, 4 Aug 2022 at 01:58, Mark Murawski <[email protected]> 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/" --000000000000d7746305e564c580 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable <div dir=3D"ltr"><div dir=3D"ltr">On Thu, 4 Aug 2022 at 01:58, Mark Murawsk= i <<a href=3D"mailto:[email protected]">markm-lists@intellasof= t.net</a>> wrote:<br></div><div class=3D"gmail_quote"><blockquote class= =3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rg= b(204,204,204);padding-left:1ex"> =20 =20 =20 <div> <div>I'm still not getting something... if I want to fix the code-a= s-is and do this:<br></div> <br> =C2=A0=C2=A0=C2=A0 FNsv =3D get_sv("main::_FN", GV_ADD);<br> =C2=A0=C2=A0=C2=A0 if (!FNsv)<br> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ereport(ERROR,<br> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0 (errcode(ERRCODE_EXTERNAL_ROUTINE_EXCEPTION),<br> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0 errmsg("couldn't fetch $_FN")));<= br> <br> =C2=A0=C2=A0=C2=A0 save_item(FNsv);=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /* local $_FN */<br></div></blockquote><div>= <br></div><div>I dont get the sequence here. You take the old value of $mai= n::_FN and then you localize it after you fetch it? That seems weird.</div>= <div>=C2=A0</div><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px = 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div> <br> =C2=A0=C2=A0=C2=A0 hv =3D newHV(); // create new hash<br> =C2=A0=C2=A0=C2=A0 hv_store_string(hv, "name", cstr2sv(desc-&= gt;proname));<br></div></blockquote><div><br></div><div>Really you shouldnt= do this until you have safely managed the refcounts of all your newly crea= ted objects so that if this die's nothing leaks.</div><div>=C2=A0</div>= <blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-= left:1px solid rgb(204,204,204);padding-left:1ex"><div> <br> =C2=A0=C2=A0=C2=A0 svFN =3D newRV_noinc((SV *) hv); // reference to the= new hash<br> =C2=A0=C2=A0=C2=A0 sv_setsv(FNsv, svFN);<br> <br> =C2=A0=C2=A0=C2=A0 // dostuff<br> <br> =C2=A0=C2=A0=C2=A0 SvREFCNT_dec_current(svFN);<br> =C2=A0=C2=A0=C2=A0 SvREFCNT_dec_current((SV *) hv);<br> <br> <br> You're saying that the =C2=A0 sv_setsv(FNsv, svFN); creates a secon= d ref... so in theory I can unref it and then all else would be equal<br> but I get this:<br> <br> WARNING:=C2=A0 Attempt to free unreferenced scalar: SV 0x55d5b1cf6480, Perl interpreter: 0x55d5b17226c0<br></div></blockquote><div><br></div><= div>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 r= efcount anymore.</div><div><br></div><div>HV *hv=3D newHV(); /* until this = is attached to something that will get cleaned up you need to deal with its= refcnt */</div><div>svFN =3D newRV_noinc((SV *) hv); /* now the hv is owne= d by svFN, we no longer have to worry about hv, just svFN */<br></div><div>= sv_setsv(FNsv, svFN);=C2=A0 /* now FNsv is a copy of the ref that is in svF= N */<br></div><div><br></div><div>So at this point we have two SV's whi= ch reference hv, svFN and FNsv, hv will have a refcount of 2, svFN should h= ave a refcount of 1.</div><div><br></div><div>SvREFCNT_dec(svFN);<br></div>= <div><br></div><div>This will decrement svFN to 0, which will cause it to b= e freed, but not before Perl walks the reference tree decrementing the thin= gs 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 o= wned by FNsv only.</div><div><br></div><div>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.</div>= <div>=C2=A0</div><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px = 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div> Also.. something I didn't follow was this:<br> "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. "<br> <br> How do you turn an SV into an RV without creating this extra reference..<br> Aren't I doing this already, with: <br> =C2=A0=C2=A0 svFN =3D newRV_noinc((SV *) hv);<br></div></blockquote><di= v><br></div><div>No. That=C2=A0 creates a new SV which is a reference to a = hv.</div><div><br></div><div>Your code does this basically:</div><div><br><= /div><div>my %hash;</div><div>my $ref=3D \%hash;</div><div>$main::_FN=3D $r= ef;</div><div><br></div><div>Obviously in perl we can write:</div><div><br>= </div><div>my %hash;</div><div>$main::_FN=3D \%hash;</div><div><br></div><d= iv>And in XS we can do the same thing. Unfortunately there isn't=C2=A0a= utility sub to do this currently, it has=C2=A0been on my TODO list to add = one for some time but lack of round=C2=A0tuits and all that.=C2=A0</div><di= v><br></div><div>You want code something like this:</div><div><br></div><di= v>sv_clear(FNsv); /* undef the sv */</div><div>sv_upgrade(FNsv,SVt_RV);</di= v><div>SvRV_set(FNsv, (SV*)hv);</div><div>SvROK_on(FNsv);</div><div><br></d= iv><div>and you are done. It's possible that the sv_upgrade() is unnece= ssary as I believe=C2=A0every SV is big enough to handle SVr_RV, try with a= nd 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 t= hus every SV can turn into one without allocating a new body and the sv_upg= rade should be superfluous.=C2=A0</div><div><br></div><div>Again, make libe= ral use of sv_dump() it is the XS version of Data::Dumper more or less.</di= v><div><br></div><div>Yves</div><div><br></div><div><br></div><div><br></di= v><div><br></div></div><div><br></div>-- <br><div dir=3D"ltr" class=3D"gmai= l_signature">perl -Mre=3Ddebug -e "/just|another|perl|hacker/"</d= iv></div> --000000000000d7746305e564c580--