From: Nicholas Clark Date: Thu, 21 Apr 2011 11:19:08 +0000 (+0100) Subject: Move the check_new() test to the start of sv_size() X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?p=p5sagit%2FDevel-Size.git;a=commitdiff_plain;h=81f1c0187fa7bb773d2f2c2e30bc57de03bad30a Move the check_new() test to the start of sv_size() For now, this requires sv_size() to return new-or-not, but it prepares the way to move all structure recursion into sv_size(). Currently it's in two places - sv_size() and total_size(). This resolves two long-standing bugs in typeglobs - total_size() was double counting entries in typeglobs, and sv_size() was double-counting the PVGV size if GvEGV() looped back. --- diff --git a/CHANGES b/CHANGES index 862d384..542e843 100644 --- a/CHANGES +++ b/CHANGES @@ -2,6 +2,8 @@ Revision history for Perl extension Devel::Size. 0.74_51 2011-04-20 nicholas * Don't count PL_sv_{undef,no,yes} in the size returned + * total_size() was double-counting entries in typeglobs + * sv_size() was double-counting the PVGV size if GvEGV() looped back 0.74_50 2011-04-19 nicholas * Ensure that size() doesn't add the referent's size for non SVt_RV references diff --git a/Size.xs b/Size.xs index c533942..1c8a0f3 100644 --- a/Size.xs +++ b/Size.xs @@ -160,7 +160,7 @@ free_state(struct state *st) Safefree(st); } -static void sv_size(pTHX_ struct state *, const SV *const, bool recurse); +static bool sv_size(pTHX_ struct state *, const SV *const, bool recurse); typedef enum { OPc_NULL, /* 0 */ @@ -406,12 +406,10 @@ op_size(pTHX_ const OP * const baseop, struct state *st) regex_size(cPMOPx(baseop)->op_pmregexp, st); #endif TAG;break; - case OPc_SVOP: TAG; - st->total_size += sizeof(struct pmop); - if (check_new(st, cSVOPx(baseop)->op_sv)) { + case OPc_SVOP: TAG; + st->total_size += sizeof(struct pmop); sv_size(aTHX_ st, cSVOPx(baseop)->op_sv, TRUE); - } - TAG;break; + TAG;break; case OPc_PADOP: TAG; st->total_size += sizeof(struct padop); TAG;break; @@ -445,12 +443,8 @@ op_size(pTHX_ const OP * const baseop, struct state *st) check_new_and_strlen(st, basecop->cop_file); check_new_and_strlen(st, basecop->cop_stashpv); #else - if (check_new(st, basecop->cop_stash)) { - sv_size(aTHX_ st, (SV *)basecop->cop_stash, TRUE); - } - if (check_new(st, basecop->cop_filegv)) { - sv_size(aTHX_ st, (SV *)basecop->cop_filegv, TRUE); - } + sv_size(aTHX_ st, (SV *)basecop->cop_stash, TRUE); + sv_size(aTHX_ st, (SV *)basecop->cop_filegv, TRUE); #endif } @@ -469,11 +463,14 @@ op_size(pTHX_ const OP * const baseop, struct state *st) # define NEW_HEAD_LAYOUT #endif -static void +static bool sv_size(pTHX_ struct state *const st, const SV * const orig_thing, const bool recurse) { const SV *thing = orig_thing; + if(!check_new(st, thing)) + return FALSE; + st->total_size += sizeof(SV); switch (SvTYPE(thing)) { @@ -490,6 +487,8 @@ sv_size(pTHX_ struct state *const st, const SV * const orig_thing, st->total_size += sizeof(IV); # endif #endif + if(recurse && SvROK(thing)) + sv_size(aTHX_ st, SvRV_const(thing), TRUE); TAG;break; /* Is it a float? Like the int, it depends on purify */ case SVt_NV: TAG; @@ -505,6 +504,8 @@ sv_size(pTHX_ struct state *const st, const SV * const orig_thing, #ifndef NEW_HEAD_LAYOUT st->total_size += sizeof(XRV); #endif + if(recurse && SvROK(thing)) + sv_size(aTHX_ st, SvRV_const(thing), TRUE); TAG;break; #endif /* How about a plain string? In which case we need to add in how @@ -586,11 +587,7 @@ sv_size(pTHX_ struct state *const st, const SV * const orig_thing, Post 5.9.something this is stored in magic, so will be found there, and Perl_av_arylen_p() takes a non-const AV*, hence compilers rightly complain about AvARYLEN() passing thing to it. */ - if (AvARYLEN(thing)) { - if (check_new(st, AvARYLEN(thing))) { - sv_size(aTHX_ st, AvARYLEN(thing), TRUE); - } - } + sv_size(aTHX_ st, AvARYLEN(thing), TRUE); #endif magic_size(thing, st); TAG;break; @@ -624,26 +621,13 @@ sv_size(pTHX_ struct state *const st, const SV * const orig_thing, magic_size(thing, st); st->total_size += ((XPVIO *) SvANY(thing))->xpv_len; - if (check_new(st, CvSTASH(thing))) { - sv_size(aTHX_ st, (SV *)CvSTASH(thing), TRUE); - } - if (check_new(st, SvSTASH(thing))) { - sv_size(aTHX_ st, (SV *)SvSTASH(thing), TRUE); - } - if (check_new(st, CvGV(thing))) { - sv_size(aTHX_ st, (SV *)CvGV(thing), TRUE); - } - if (check_new(st, CvPADLIST(thing))) { - sv_size(aTHX_ st, (SV *)CvPADLIST(thing), TRUE); - } - if (check_new(st, CvOUTSIDE(thing))) { - sv_size(aTHX_ st, (SV *)CvOUTSIDE(thing), TRUE); - } + sv_size(aTHX_ st, (SV *)CvSTASH(thing), TRUE); + sv_size(aTHX_ st, (SV *)SvSTASH(thing), TRUE); + sv_size(aTHX_ st, (SV *)CvGV(thing), TRUE); + sv_size(aTHX_ st, (SV *)CvPADLIST(thing), TRUE); + sv_size(aTHX_ st, (SV *)CvOUTSIDE(thing), TRUE); if (CvISXSUB(thing)) { - SV *sv = cv_const_sv((CV *)thing); - if (sv) { - sv_size(aTHX_ st, sv, TRUE); - } + sv_size(aTHX_ st, cv_const_sv((CV *)thing), TRUE); } else { op_size(aTHX_ CvSTART(thing), st); op_size(aTHX_ CvROOT(thing), st); @@ -661,28 +645,13 @@ sv_size(pTHX_ struct state *const st, const SV * const orig_thing, /* Is there something hanging off the glob? */ if (GvGP(thing)) { if (check_new(st, GvGP(thing))) { - st->total_size += sizeof(GP); - { - SV *generic_thing; - if ((generic_thing = (SV *)(GvGP(thing)->gp_sv))) { - sv_size(aTHX_ st, generic_thing, TRUE); - } - if ((generic_thing = (SV *)(GvGP(thing)->gp_form))) { - sv_size(aTHX_ st, generic_thing, TRUE); - } - if ((generic_thing = (SV *)(GvGP(thing)->gp_av))) { - sv_size(aTHX_ st, generic_thing, TRUE); - } - if ((generic_thing = (SV *)(GvGP(thing)->gp_hv))) { - sv_size(aTHX_ st, generic_thing, TRUE); - } - if ((generic_thing = (SV *)(GvGP(thing)->gp_egv))) { - sv_size(aTHX_ st, generic_thing, TRUE); - } - if ((generic_thing = (SV *)(GvGP(thing)->gp_cv))) { - sv_size(aTHX_ st, generic_thing, TRUE); - } - } + st->total_size += sizeof(GP); + sv_size(aTHX_ st, (SV *)(GvGP(thing)->gp_sv), TRUE); + sv_size(aTHX_ st, (SV *)(GvGP(thing)->gp_form), TRUE); + sv_size(aTHX_ st, (SV *)(GvGP(thing)->gp_av), TRUE); + sv_size(aTHX_ st, (SV *)(GvGP(thing)->gp_hv), TRUE); + sv_size(aTHX_ st, (SV *)(GvGP(thing)->gp_egv), TRUE); + sv_size(aTHX_ st, (SV *)(GvGP(thing)->gp_cv), TRUE); } } TAG;break; @@ -690,12 +659,8 @@ sv_size(pTHX_ struct state *const st, const SV * const orig_thing, st->total_size += sizeof(XPVFM); magic_size(thing, st); st->total_size += ((XPVIO *) SvANY(thing))->xpv_len; - if (check_new(st, CvPADLIST(thing))) { - sv_size(aTHX_ st, (SV *)CvPADLIST(thing), TRUE); - } - if (check_new(st, CvOUTSIDE(thing))) { - sv_size(aTHX_ st, (SV *)CvOUTSIDE(thing), TRUE); - } + sv_size(aTHX_ st, (SV *)CvPADLIST(thing), TRUE); + sv_size(aTHX_ st, (SV *)CvOUTSIDE(thing), TRUE); if (st->go_yell && !st->fm_whine) { carp("Devel::Size: Calculated sizes for FMs are incomplete"); @@ -713,15 +678,9 @@ sv_size(pTHX_ struct state *const st, const SV * const orig_thing, check_new_and_strlen(st, ((XPVIO *) SvANY(thing))->xio_fmt_name); check_new_and_strlen(st, ((XPVIO *) SvANY(thing))->xio_bottom_name); /* Throw the GVs on the list to be walked if they're not-null */ - if (((XPVIO *) SvANY(thing))->xio_top_gv) { - sv_size(aTHX_ st, (SV *)((XPVIO *) SvANY(thing))->xio_top_gv, TRUE); - } - if (((XPVIO *) SvANY(thing))->xio_bottom_gv) { - sv_size(aTHX_ st, (SV *)((XPVIO *) SvANY(thing))->xio_bottom_gv, TRUE); - } - if (((XPVIO *) SvANY(thing))->xio_fmt_gv) { - sv_size(aTHX_ st, (SV *)((XPVIO *) SvANY(thing))->xio_fmt_gv, TRUE); - } + sv_size(aTHX_ st, (SV *)((XPVIO *) SvANY(thing))->xio_top_gv, TRUE); + sv_size(aTHX_ st, (SV *)((XPVIO *) SvANY(thing))->xio_bottom_gv, TRUE); + sv_size(aTHX_ st, (SV *)((XPVIO *) SvANY(thing))->xio_fmt_gv, TRUE); /* Only go trotting through the IO structures if they're really trottable. If USE_PERLIO is defined we can do this. If @@ -734,6 +693,7 @@ sv_size(pTHX_ struct state *const st, const SV * const orig_thing, default: warn("Devel::Size: Unknown variable type: %d encountered\n", SvTYPE(thing) ); } + return TRUE; } static struct state * @@ -816,11 +776,8 @@ CODE: while (av_len(pending_array) >= 0) { thing = av_pop(pending_array); /* Process it if we've not seen it */ - if (check_new(st, thing)) { + if (sv_size(aTHX_ st, thing, TRUE)) { dbg_printf(("# Found type %i at %p\n", SvTYPE(thing), thing)); - /* Is it valid? */ - if (thing) { - /* Yes, it is. So let's check the type */ switch (SvTYPE(thing)) { /* fix for bug #24846 (Does not correctly recurse into references in a PVNV-type scalar) */ case SVt_PVNV: TAG; @@ -899,10 +856,7 @@ CODE: TAG;break; default: TAG;break; - } } - - sv_size(aTHX_ st, thing, TRUE); } else { /* check_new() returned false: */ #ifdef DEVEL_SIZE_DEBUGGING diff --git a/t/globs.t b/t/globs.t index 6fdeb5f..0fecd46 100644 --- a/t/globs.t +++ b/t/globs.t @@ -61,8 +61,6 @@ $SIG{__WARN__} = sub { my $copy_gv_size = total_size($copy); # GV copies point back to the real GV through GvEGV. They share the same GP # and GvFILE - local $TODO = 'EGV is double counted. GV - GP == ' - . ($incremental_gv_size - $gp_size); is($copy_gv_size, $real_gv_size + $incremental_gv_size - $gp_size, 'GV copies point back to the real GV'); } @@ -108,7 +106,6 @@ sub gv_grew { unless $Config{usemultiplicity}; is($io_now_size, $io_was_size, "IO doesn't grow as GV has SCALAR"); is($gv_now_size, $gv_was_size, 'GV size unchanged as GV has SCALAR'); - local $TODO = 'total_size double counts GP entries'; is($gv_now_total_size, $gv_was_total_size, 'GV total size unchanged as GV has SCALAR'); } elsif ($type eq 'CODE' || $type eq 'FORMAT') { @@ -130,7 +127,6 @@ sub gv_grew { "IO total_size grew by expected amount for $type"); is($gv_now_size, $gv_was_size + $new_thing_size, "GV size grew by expected amount for $type"); - local $TODO = 'total_size double counts GP entries'; is($gv_now_total_size, $gv_was_total_size + $new_thing_size, "GV total_size grew by expected amount for $type"); }