Beefy Boxes and Bandwidth Generously Provided by pair Networks
Pathologically Eclectic Rubbish Lister

Re^5: perlembed: mortalize an AV or not (misc)

by tye (Sage)
on Jul 10, 2006 at 21:22 UTC ( #560244=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

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        

  • Comment on Re^5: perlembed: mortalize an AV or not (misc)

Replies are listed 'Best First'.
Re^6: perlembed: mortalize an AV or not (misc)
by edan (Curate) on Jul 11, 2006 at 05:25 UTC
    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?


      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        

        Someone asked about the Perl memory leak I spotted. Rather than try to put code into a /msg, I'll just include it here, that way anyone can point out that I'm mistaken. From line 3113 of op.c, Perl v5.8.0:

        repointer = newSViv(0); av_push(PL_regex_padav,SvREFCNT_inc(repointer));

        newSViv() gives a ref count of 1. av_push() doesn't change ref count so the SvREFCNT_inc() leaves a ref count of 2 but repointer is not subsequently used so we've made one reference to it so we've got a memory leak.

        I'm hoping the person who asked will perlbug this and save me the bother (not talking to p5p is always an advantage, IMO), but we'll see what happens. :)

        - tye        

        Okay! Last try, I hope. Does this look right?

        void test(char *str) { dSP; ENTER; SAVETMPS; PUSHMARK(SP); AV *arr = (AV *)sv_2mortal((SV *) newAV()); SV *arr_value = sv_2mortal(newSVpv("test", 0)); av_push( arr, arr_value); SvREFCNT_inc(arr_value); XPUSHs(sv_2mortal(newRV_inc((SV *)arr))); HV *hash = (HV *)sv_2mortal((SV *)newHV()); SV *hash_value = sv_2mortal(newSVpv("test", 0)); SvREFCNT_inc( hash_value ); if ( NULL == hv_store( hash, "key", strlen("key"), hash_value, 0) +) { SvREFCNT_dec( hash_value ); FREETMPS; LEAVE; return; } XPUSHs(sv_2mortal(newRV_inc((SV *)hash))); PUTBACK; call_pv("TestPM::test_leak", G_SCALAR); SPAGAIN; SV *retval = POPs; PUTBACK; FREETMPS; LEAVE; }

        Thanks again, tye.


Log In?

What's my password?
Create A New User
Node Status?
node history
Node Type: note [id://560244]
and all is quiet...

How do I use this? | Other CB clients
Other Users?
Others wandering the Monastery: (11)
As of 2018-02-21 15:30 GMT
Find Nodes?
    Voting Booth?
    When it is dark outside I am happiest to see ...

    Results (282 votes). Check out past polls.