Beefy Boxes and Bandwidth Generously Provided by pair Networks
good chemistry is complicated,
and a little bit messy -LW
 
PerlMonks  

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

by edan (Curate)
on Jul 10, 2006 at 18:03 UTC ( #560190=note: print w/ replies, xml ) Need Help??


in reply to Re^3: 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

Tried that. Leaks like a madman. Nope, I think the HV and the SVpv need to be mortal. I'll stick with my current incantation, since empirically it's correct, even though I still don't know why, and am still plagued by the thought that maybe it only seems right, but isn't.

Any more help is most welcome.

--
edan


Comment on Re^4: perlembed: mortalize an AV, get "Attempt to free unreferenced scalar" - don't mortalize, leaks memory
Re^5: perlembed: mortalize an AV, get "Attempt to free unreferenced scalar" - don't mortalize, leaks memory
by demerphq (Chancellor) on Jul 10, 2006 at 19:49 UTC

    Tried that. Leaks like a madman.

    Well, thanks for letting me know. I guess i need to book up on this stuff a little better :-)

    I think at this point if some internals guru hasnt popped up to explain things (i havent checked as of writing this) that you should ask on p5p.

    Sorry I wasnt more helpful. :-(

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

Re^5: perlembed: mortalize an AV or not (misc)
by tye (Cardinal) on Jul 10, 2006 at 21:22 UTC

    Possible "mistakes":

    1. Ref count set too high: Leaks memory
    2. Ref count set too low: Throws up, hopefully not too cryptically
    3. Ref count set exactly 1 "too high" and mortalized when it doesn't need to be: Nothing goes wrong

    So you've probably got a case of (3) now. You usually don't have to make mortal something that you aren't putting directly onto the Perl stack, but sometimes it is convient to do so.

    For example, you could use newRV_noinc() since you are making a reference to an AV that you just created (and thus that already has a reference count of 1). Then you wouldn't need to make the AV mortal. Or, you could decide that you like the slightly less efficient method of immediately making it mortal such that it will get freed even if something goes wrong with (or before) your call to newRV().

    I'd try to avoid calling SvREFCNT_inc() / SvREFCNT_dec(), there's usually a clearer way (that is less likely to be done incorrectly). You should call croak() not printf() and exit().

    Why not use call_pv() instead of call_sv() and making yet another thing that you have to make mortal?

    So your original problem was two mistakes. First, av_push() doesn't increment the refcount so creating a new SV and av_push()ing it onto one array is fine (refcount starts out as and stays 1). But av_push()ing it onto two arrays would be bad (refcount still 1 when needs to be 2). Mortalizing the SV means the refcount drops to 0 later so it tries to get free()d twice.

    Second, you use newRV() [which is newRV_inc()] so the AV's ref count was too high. So you had one ref-count too high and a different ref count too low. So your two choices were leaking an AV such that the AV didn't try to free up the SV a second time or properly destroy the AV such that it noticed that the SV had too low of a ref count.

    - tye        

      Thanks tye - this is extremely helpful. So I just want to make sure that I understood properly what you recommend to do. Does the following code give a decent example of how to push an array ref and a hash ref onto the stack?

      dSP; ENTER; SAVETMPS; PUSHMARK(SP); AV *arr = newAV(); av_push( arr, newSVpv("string", 0)); XPUSHs(sv_2mortal(newRV_noinc((SV *)arr))); HV *hash = newHV(); SV *type = newSVpv("string", 0); hv_store( hash, "type", strlen("type"), type, 0); XPUSHs(sv_2mortal(newRV_noinc((SV *)hash))); PUTBACK; call_sv(sv_2mortal(newSVpv("TestPM::test_leak", 0)), G_SCALAR); SPAGAIN; SV *retval = POPs; PUTBACK; FREETMPS; LEAVE;

      I'd really appreciate it if you could validate this approach, which is my understanding of what you explained above.

      Regarding your other questions:

      1. The printf/exit was just an example - my real code is actually throwing an exception there. Actually, it's also doing a FREETMPS/LEAVE - is that the right thing to do if I'm expecting the process to stay alive, and I want everything cleaned up? Will that work okay with the non-mortal approach you're advocating?
      2. I was calling SvREFCNT_inc() since it seemed to me from the hv_store() docs that I needed to. But you're saying that I don't, right? I have to say that I'm still left feeling like the above code might work, but isn't really handling refcounts correctly. But I trust you more than I trust my gut feelings here...
      3. call_sv() was also just an example - my real code is calling call_method().

      So wouldn't it be better if I do mortalize everything, and just SvREFCNT_inc whenever I either av_push() or hv_store() the SV, as well as using newRV_inc()? Will that not work and be correct?

      --
      edan

        Yes, that code looks like it handles ref counts correctly.

        So wouldn't it be better if I do mortalize everything, and just SvREFCNT_inc whenever I either av_push() or hv_store() the SV, as well as using newRV_inc()? Will that not work and be correct?

        Yes, I was coming to the same conclusion.

        I hadn't bothered to grok this much about the ref-count nature of these Perlish macros before (I prefer very simple XS code for a lot of reasons). Now I see more why the Parrot designers think ref counting is so dang hard to get right. Perl's macros make it extra hard (and I found a memory leak in the Perl source already). I think this is mostly motivated by efficiency concerns, and I bet those concerns are overblown, but those are guesses on my part.

        I think I'd much rather have a system where ref counts start at 0 and most things increment the ref count (including sv2mortal, av_push). That is, a system where an "extra" increment and decrement will often get done but you don't have to add things up and decide between *_inc and *_noinc and you don't have to consider inc-/decrementing ref counts directly.

        Yes, you don't need to call SvREFCNT_inc() in the above code. You could choose to instead only create mortals and SvREFCNT_inc() each time you av_push() or such.

        Sorry, I don't have FREETMPS grokked ATM and can't study that just now.

        - tye        

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

    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

      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

Log In?
Username:
Password:

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

How do I use this? | Other CB clients
Other Users?
Others surveying the Monastery: (8)
As of 2014-07-11 00:53 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    When choosing user names for websites, I prefer to use:








    Results (217 votes), past polls