Beefy Boxes and Bandwidth Generously Provided by pair Networks
Perl Monk, Perl Meditation
 
PerlMonks  

Re^5: perlembed: mortalize an AV, get "Attempt to free unreferenced scalar" - don't mortalize, leaks memory

by demerphq (Chancellor)
on Jul 11, 2006 at 09:17 UTC ( #560367=note: print w/ replies, xml ) Need Help??


in reply to Re^4: perlembed: mortalize an AV, get "Attempt to free unreferenced scalar" - don't mortalize, leaks memory
in thread perlembed: mortalize an AV, get "Attempt to free unreferenced scalar" - don't mortalize, leaks memory

Hmm, i did a little research into this, and i think i know what the problem was with my code. newRV is an alias for newRV_inc(), which means the item being referenced has an implicit SvREFCNT_inc() called on it, thus in the snippet i showed *hash ends up with a refcount of 2 instead of a refcount of 1. Changing it to newRV_noinc() should fix it.

HV *hash = newHV(); SV *type = newSVpv("string", 0); if ( NULL == hv_store( hash, "type", strlen("type"), type, 0) ) { SvREFCNT_dec( type ); /* free the SV */ printf("hv_store failed\n"); exit(1); /*me wonders about this...*/ } XPUSHs(sv_2mortal(newRV_noinc((SV *)hash)));

I really have to get myself set up to check this, but I thought id mention it. BTW, if you havent already read it you should review "Reference Counts and Mortality" in perlguts. Which on a quick reading basically agrees with what i said before, with the minor caveat about newRV_noinc(), which it specifically documents as being a common mistake.

For example, imagine you want to return a reference from an XSUB function. Inside the XSUB routine, you create an SV which initially has a reference count of one. Then you call newRV_inc, passing it the just-created SV. This returns the reference as a new SV, but the reference count of the SV you passed to newRV_inc has been incremented to two. Now you return the reference from the XSUB routine and forget about the SV. But Perl hasn't! Whenever the returned reference is destroyed, the reference count of the original SV is decreased to one and nothing happens. The SV will hang around without any way to access it until Perl itself terminates. This is a memory leak.

Which basically explains why my original snippet "leaks like mad". It leaks like mad because the root of the structure that you want to disappear on its own never goes to 0, so the whole lot stays in memory. Change the newRV() to newRV_noinc() and it should be freed as and when expected and no leaks.

---
$world=~s/war/peace/g


Comment on Re^5: perlembed: mortalize an AV, get "Attempt to free unreferenced scalar" - don't mortalize, leaks memory
Select or Download Code
Re^6: perlembed: mortalize an AV, get "Attempt to free unreferenced scalar" - don't mortalize, leaks memory
by edan (Curate) on Jul 11, 2006 at 11:10 UTC

    Hmm.... thanks for pressing this, demerphq. You appear to be right about not needing so many mortals. I think the trick is to avoid incrementing any refcounts (as I was doing after hv_store()), and only mortalize the RV I am pushing on the stack. This is basically what you said, but the code comes out a bit different from what you said:

    AV *arr = newAV(); av_push( arr, newSVpv("test", 0)); XPUSHs(sv_2mortal(newRV_noinc((SV *)arr))); HV *hash = newHV(); hv_store( hash, "key", strlen("key"), newSVpv("test", 0), 0); XPUSHs(sv_2mortal(newRV_noinc((SV *)hash)));

    So I'm just creating all "intermediate" variables non-mortals, not incrementing any refcounts after av_push or hv_store(), using newRV_noinc(), and mortalizing just the RV that's pushed on the stack.

    Does that seem right? Sure looks cleaner...

    --
    edan

      Hmm.... thanks for pressing this

      No problem. I got it wrong first off, which I felt bad about, and ultimately your quest to educate yourself ended up educating me, which IMO is a victory for us both, so thanks right back atchya :-)

      Anyway, the posted code looks lot closer to what I would expect. The only glaring issue is basically a nit, in that you need to handle the case of the hv_store() failing. Which is why its normally done like

      HV *hash = newHV(); SV *sv= newSVpv("test", 0); if (!hv_store( hash, "key", strlen("key"), sv , 0)) { /* need to explicitly free the sv if its * not successfully stored in the hash */ SvREFCNT_dec(sv); }

      As otherwise if the hv_store fails you will leak....

      I guess that this is just sample code so I should overlook the the overuse of strlen but in that sample I'd supply sizes explicitly. Also, be careful, strlen() is unsafe in the core (especially as regards SV's) as strings are allowed to contain embedded nulls.

      ---
      $world=~s/war/peace/g

        Are you sure about that? Seems to me from perlapi that the caller is responsible for both incrementing and decrementing the stored SV. But since I'm not incrementing, why should I decrement upon failure? I took the example of a bare hv_store() with neither increment nor decrement from DBI.xs, as you suggested...

        --
        edan

Log In?
Username:
Password:

What's my password?
Create A New User
Node Status?
node history
Node Type: note [id://560367]
help
Chatterbox?
and the web crawler heard nothing...

How do I use this? | Other CB clients
Other Users?
Others rifling through the Monastery: (14)
As of 2014-09-30 16:43 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    How do you remember the number of days in each month?











    Results (378 votes), past polls