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

[edan]

in reply to Re^5: perlembed: mortalize an AV or not (misc)
in thread perlembed: mortalize an AV, get "Attempt to free unreferenced scalar" - don't mortalize, leaks memory

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?

[tye]

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.

[edan]

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.

[demerphq]

No it really doesnt look right to me. Have a look through the sources for similar code. I dont think youll see so many calls to sv_2mortal. I suggest using something like the XS that comes with DBI, or the XS that comes with List::Util for examples.

I realize that given my earlier mistaken reply you probably arent inclined to rate my opinion highly, but I strongly encourage you to look into this further. Its sufficiently different looking from XS and core code ive seen before that it rings alarm bells in my head. Especially given that you arent using newRV_noinc() anywhere, despite perlguts specifically saying you should be.

[tye]

I'd probably only SvREFCNT_inc() if the "insert" code succeeded. That also means that you don't need to SvREFCNT_dec().

I think you'd be better off not having to remember to FREETMPS; LEAVE; if you return(), so I'd go for a simpler XS sub that lets the framework insert such stuff for you and call a regular C sub that can return() when it feels like it and the calling XS sub will do the proper clean-up.

demerphq is surely correct that this style isn't typical. But I beleive it is correct in its handling of ref counts and also understand the motivation for the style.

[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. :)