Store GvGP in the SV head union. For all the common lookups [eg GvCV()]

this avoids 1 pointer dereference and the associated risk of a CPU
cache miss. Although this patch looks deceptively small, I fear its
CBV(*) might be rather high.
(* Crack By Volume)

p4raw-id: //depot/perl@27323

diff --git a/dump.c b/dump.c
index b63ad5c..2c6aa23 100644 (file)
--- a/dump.c
+++ b/dump.c
@@ -1322,7 +1322,7 @@ Perl_do_sv_dump(pTHX_ I32 level, PerlIO *file, SV *sv, I32 nest, I32 maxnest, bo
        SvREFCNT_dec(d);
        return;
     }
-    if (type <= SVt_PVLV) {
+    if (type <= SVt_PVLV && !isGV_with_GP(sv)) {
        if (SvPVX_const(sv)) {
            Perl_dump_indent(aTHX_ level, file,"  PV = 0x%"UVxf" ", PTR2UV(SvPVX_const(sv)));
            if (SvOOK(sv))
@@ -1550,6 +1550,8 @@ Perl_do_sv_dump(pTHX_ I32 level, PerlIO *file, SV *sv, I32 nest, I32 maxnest, bo
        Perl_dump_indent(aTHX_ level, file, "  NAME = \"%s\"\n", GvNAME(sv));
        Perl_dump_indent(aTHX_ level, file, "  NAMELEN = %"IVdf"\n", (IV)GvNAMELEN(sv));
        do_hv_dump (level, file, "  GvSTASH", GvSTASH(sv));
+       if (!isGV_with_GP(sv))
+           break;
        Perl_dump_indent(aTHX_ level, file, "  GP = 0x%"UVxf"\n", PTR2UV(GvGP(sv)));
        if (!GvGP(sv))
            break;

diff --git a/ext/Devel/Peek/t/Peek.t b/ext/Devel/Peek/t/Peek.t
index f21ca6c..fdf57a9 100644 (file)
--- a/ext/Devel/Peek/t/Peek.t
+++ b/ext/Devel/Peek/t/Peek.t
@@ -308,7 +308,6 @@ do_test(17,
   FLAGS = \\(SCREAM,MULTI(?:,IN_PAD)?\\)
   IV = 0
   NV = 0
-  PV = 0
   NAME = "a"
   NAMELEN = 1
   GvSTASH = $ADDR\\t"main"

diff --git a/gv.c b/gv.c
index 0ec33ef..be5aeb7 100644 (file)
--- a/gv.c
+++ b/gv.c
@@ -192,6 +192,7 @@ Perl_gv_init(pTHX_ GV *gv, HV *stash, const char *name, STRLEN len, int multi)
            Safefree(SvPVX_mutable(gv));
     }
     Newxz(gp, 1, GP);
+    SvSCREAM_on(gv);
     GvGP(gv) = gp_ref(gp);
 #ifdef PERL_DONT_CREATE_GVSV
     GvSV(gv) = NULL;
@@ -204,7 +205,6 @@ Perl_gv_init(pTHX_ GV *gv, HV *stash, const char *name, STRLEN len, int multi)
     GvFILE(gv) = CopFILE(PL_curcop) ? CopFILE(PL_curcop) : (char *) "";
     GvCVGEN(gv) = 0;
     GvEGV(gv) = gv;
-    SvSCREAM_on(gv);
     GvSTASH(gv) = stash;
     if (stash)
        Perl_sv_add_backref(aTHX_ (SV*)stash, (SV*)gv);
@@ -1374,7 +1374,7 @@ Perl_gp_free(pTHX_ GV *gv)
     dVAR;
     GP* gp;
 
-    if (!gv || !(gp = GvGP(gv)))
+    if (!gv || !isGV_with_GP(gv) || !(gp = GvGP(gv)))
        return;
     if (gp->gp_refcnt == 0) {
        if (ckWARN_d(WARN_INTERNAL))

diff --git a/gv.h b/gv.h
index da9da4a..1e4797a 100644 (file)
--- a/gv.h
+++ b/gv.h
@@ -31,9 +31,10 @@ struct gp {
     ((!defined(_MSC_VER) || _MSC_VER > 1200) && !defined(__BORLANDC__))
 #  define GvGP(gv)     (*(assert(SvTYPE(gv) == SVt_PVGV || \
                                  SvTYPE(gv) == SVt_PVLV), \
-                          &(GvXPVGV(gv)->xgv_gp)))
+                       assert(isGV_with_GP(gv)),       \
+                          &((gv)->sv_u.svu_gp)))
 #else
-#  define GvGP(gv)     (GvXPVGV(gv)->xgv_gp)
+#  define GvGP(gv)     ((gv)->sv_u.svu_gp)
 #endif
 
 #define GvNAME(gv)     (GvXPVGV(gv)->xgv_name)

diff --git a/pp.c b/pp.c
index d63f039..1724ab0 100644 (file)
--- a/pp.c
+++ b/pp.c
@@ -3016,7 +3016,9 @@ PP(pp_substr)
                        Perl_warner(aTHX_ packWARN(WARN_SUBSTR),
                                "Attempt to use reference as lvalue in substr");
                }
-               if (SvOK(sv))           /* is it defined ? */
+               if (isGV_with_GP(sv))
+                   SvPV_force_nolen(sv);
+               else if (SvOK(sv))      /* is it defined ? */
                    (void)SvPOK_only_UTF8(sv);
                else
                    sv_setpvn(sv,"",0); /* avoid lexical reincarnation */

diff --git a/pp_hot.c b/pp_hot.c
index 8d590ca..d26d8f0 100644 (file)
--- a/pp_hot.c
+++ b/pp_hot.c
@@ -1611,6 +1611,9 @@ Perl_do_readline(pTHX)
        sv = TARG;
        if (SvROK(sv))
            sv_unref(sv);
+       else if (isGV_with_GP(sv)) {
+           SvPV_force_nolen(sv);
+       }
        SvUPGRADE(sv, SVt_PV);
        tmplen = SvLEN(sv);     /* remember if already alloced */
        if (!tmplen && !SvREADONLY(sv))

diff --git a/sv.c b/sv.c
index 462dc9e..6d550d9 100644 (file)
--- a/sv.c
+++ b/sv.c
@@ -472,7 +472,7 @@ static void
 do_clean_named_objs(pTHX_ SV *sv)
 {
     dVAR;
-    if (SvTYPE(sv) == SVt_PVGV && GvGP(sv)) {
+    if (SvTYPE(sv) == SVt_PVGV && isGV_with_GP(sv) && GvGP(sv)) {
        if ((
 #ifdef PERL_DONT_CREATE_GVSV
             GvSV(sv) &&
@@ -2117,12 +2117,9 @@ S_sv_2iuv_common(pTHX_ SV *sv) {
        }
     }
     else  {
-       if (((SvFLAGS(sv) & (SVp_POK|SVp_SCREAM)) == SVp_SCREAM)
-           && (SvTYPE(sv) == SVt_PVGV || SvTYPE(sv) == SVt_PVLV)) {
+       if (isGV_with_GP(sv)) {
            return PTR2IV(glob_2inpuv((GV *)sv, NULL, TRUE));
        }
-       if (SvTYPE(sv) == SVt_PVGV)
-           sv_dump(sv);
 
        if (!(SvFLAGS(sv) & SVs_PADTMP)) {
            if (!PL_localizing && ckWARN(WARN_UNINITIALIZED))
@@ -2471,8 +2468,7 @@ Perl_sv_2nv(pTHX_ register SV *sv)
 #endif /* NV_PRESERVES_UV */
     }
     else  {
-       if (((SvFLAGS(sv) & (SVp_POK|SVp_SCREAM)) == SVp_SCREAM)
-           && (SvTYPE(sv) == SVt_PVGV || SvTYPE(sv) == SVt_PVLV)) {
+       if (isGV_with_GP(sv)) {
            glob_2inpuv((GV *)sv, NULL, TRUE);
            return 0.0;
        }
@@ -2809,8 +2805,7 @@ Perl_sv_2pv_flags(pTHX_ register SV *sv, STRLEN *lp, I32 flags)
 #endif
     }
     else {
-       if (((SvFLAGS(sv) & (SVp_POK|SVp_SCREAM)) == SVp_SCREAM)
-           && (SvTYPE(sv) == SVt_PVGV || SvTYPE(sv) == SVt_PVLV)) {
+       if (isGV_with_GP(sv)) {
            return glob_2inpuv((GV *)sv, lp, FALSE);
        }
 
@@ -2945,8 +2940,7 @@ Perl_sv_2bool(pTHX_ register SV *sv)
            if (SvNOKp(sv))
                return SvNVX(sv) != 0.0;
            else {
-               if ((SvFLAGS(sv) & SVp_SCREAM)
-                   && (SvTYPE(sv) == (SVt_PVGV) || SvTYPE(sv) == (SVt_PVLV)))
+               if (isGV_with_GP(sv))
                    return TRUE;
                else
                    return FALSE;
@@ -3189,8 +3183,15 @@ S_glob_assign_glob(pTHX_ SV *dstr, SV *sstr, const int dtype)
        const char * const name = GvNAME(sstr);
        const STRLEN len = GvNAMELEN(sstr);
        /* don't upgrade SVt_PVLV: it can hold a glob */
-       if (dtype != SVt_PVLV)
+       if (dtype != SVt_PVLV) {
+           if (dtype >= SVt_PV) {
+               SvPV_free(dstr);
+               SvPV_set(dstr, 0);
+               SvLEN_set(dstr, 0);
+               SvCUR_set(dstr, 0);
+           }
            sv_upgrade(dstr, SVt_PVGV);
+       }
        GvSTASH(dstr) = GvSTASH(sstr);
        if (GvSTASH(dstr))
            Perl_sv_add_backref(aTHX_ (SV*)GvSTASH(dstr), dstr);
@@ -3205,10 +3206,11 @@ S_glob_assign_glob(pTHX_ SV *dstr, SV *sstr, const int dtype)
     }
 #endif
 
+    gp_free((GV*)dstr);
+    SvSCREAM_off(dstr);
     (void)SvOK_off(dstr);
-    SvSCREAM_on(dstr);
     GvINTRO_off(dstr);         /* one-shot flag */
-    gp_free((GV*)dstr);
+    SvSCREAM_on(dstr);
     GvGP(dstr) = gp_ref(GvGP(sstr));
     if (SvTAINTED(sstr))
        SvTAINT(dstr);
@@ -3685,8 +3687,7 @@ Perl_sv_setsv_flags(pTHX_ SV *dstr, register SV *sstr, I32 flags)
        }
     }
     else {
-       if ((stype == SVt_PVGV || stype == SVt_PVLV)
-                && (sflags & SVp_SCREAM)) {
+       if (isGV_with_GP(sstr)) {
            /* This stringification rule for globs is spread in 3 places.
               This feels bad. FIXME.  */
            const U32 wasfake = sflags & SVf_FAKE;
@@ -7692,8 +7693,9 @@ S_sv_unglob(pTHX_ SV *sv)
     SvFAKE_off(sv);
     gv_efullname3(temp, (GV *) sv, "*");
 
-    if (GvGP(sv))
+    if (GvGP(sv)) {
        gp_free((GV*)sv);
+    }
     if (GvSTASH(sv)) {
        sv_del_backref((SV*)GvSTASH(sv), sv);
        GvSTASH(sv) = NULL;
@@ -9630,7 +9632,10 @@ Perl_rvpv_dup(pTHX_ SV *dstr, const SV *sstr, CLONE_PARAMS* param)
        }
        else {
            /* Special case - not normally malloced for some reason */
-           if ((SvREADONLY(sstr) && SvFAKE(sstr))) {
+           if (isGV_with_GP(sstr)) {
+               /* Don't need to do anything here.  */
+           }
+           else if ((SvREADONLY(sstr) && SvFAKE(sstr))) {
                /* A "shared" PV - clone it as "shared" PV */
                SvPV_set(dstr,
                         HEK_KEY(hek_dup(SvSHARED_HEK_FROM_PV(SvPVX_const(sstr)),
@@ -9774,7 +9779,8 @@ Perl_sv_dup(pTHX_ const SV *sstr, CLONE_PARAMS* param)
                 sv_type_details->body_size + sv_type_details->offset, char);
 #endif
 
-           if (sv_type != SVt_PVAV && sv_type != SVt_PVHV)
+           if (sv_type != SVt_PVAV && sv_type != SVt_PVHV
+               && !isGV_with_GP(dstr))
                Perl_rvpv_dup(aTHX_ dstr, sstr, param);
 
            /* The Copy above means that all the source (unduplicated) pointers
@@ -9815,11 +9821,16 @@ Perl_sv_dup(pTHX_ const SV *sstr, CLONE_PARAMS* param)
                break;
            case SVt_PVGV:
                GvNAME(dstr)    = SAVEPVN(GvNAME(dstr), GvNAMELEN(dstr));
-               GvSTASH(dstr)   = hv_dup(GvSTASH(dstr), param);
                /* Don't call sv_add_backref here as it's going to be created
                   as part of the magic cloning of the symbol table.  */
-               GvGP(dstr)      = gp_dup(GvGP(dstr), param);
-               (void)GpREFCNT_inc(GvGP(dstr));
+               GvSTASH(dstr)   = hv_dup(GvSTASH(dstr), param);
+               if(isGV_with_GP(sstr)) {
+                   /* Danger Will Robinson - GvGP(dstr) isn't initialised
+                      at the point of this comment.  */
+                   GvGP(dstr)  = gp_dup(GvGP(sstr), param);
+                   (void)GpREFCNT_inc(GvGP(dstr));
+               } else
+                   Perl_rvpv_dup(aTHX_ dstr, sstr, param);
                break;
            case SVt_PVIO:
                IoIFP(dstr)     = fp_dup(IoIFP(dstr), IoTYPE(dstr), param);

diff --git a/sv.h b/sv.h
index 102c790..c509ac6 100644 (file)
--- a/sv.h
+++ b/sv.h
@@ -97,6 +97,7 @@ typedef struct hek HEK;
        char*   svu_pv;         /* pointer to malloced string */        \
        SV**    svu_array;              \
        HE**    svu_hash;               \
+       GP*     svu_gp;                 \
     }  sv_u
 
 
@@ -403,7 +404,6 @@ struct xpvlv {
     HV*                xmg_stash;      /* class package */
 
     /* a full glob fits into this */
-    GP*                xgv_gp;
     char*      xgv_name;
     STRLEN     xgv_namelen;
     HV*                xgv_stash;
@@ -432,7 +432,6 @@ struct xpvgv {
     } xmg_u;
     HV*                xmg_stash;      /* class package */
 
-    GP*                xgv_gp;
     char*      xgv_name;
     STRLEN     xgv_namelen;
     HV*                xgv_stash;
@@ -763,12 +762,14 @@ Set the actual length of the string which is in the SV.  See C<SvIV_set>.
 
 #if defined(__GNUC__) && !defined(PERL_GCC_BRACE_GROUPS_FORBIDDEN)
 #define assert_not_ROK(sv)     ({assert(!SvROK(sv) || !SvRV(sv));}),
+#define assert_not_glob(sv)    ({assert(!isGV_with_GP(sv));}),
 #else
 #define assert_not_ROK(sv)     
+#define assert_not_glob(sv)    
 #endif
 
 #define SvOK(sv)               (SvFLAGS(sv) & SVf_OK)
-#define SvOK_off(sv)           (assert_not_ROK(sv)                     \
+#define SvOK_off(sv)           (assert_not_ROK(sv) assert_not_glob(sv) \
                                 SvFLAGS(sv) &= ~(SVf_OK|SVf_AMAGIC|    \
                                                  SVf_IVisUV|SVf_UTF8), \
                                                        SvOOK_off(sv))
@@ -779,21 +780,21 @@ Set the actual length of the string which is in the SV.  See C<SvIV_set>.
 
 #define SvOKp(sv)              (SvFLAGS(sv) & (SVp_IOK|SVp_NOK|SVp_POK))
 #define SvIOKp(sv)             (SvFLAGS(sv) & SVp_IOK)
-#define SvIOKp_on(sv)          (SvRELEASE_IVX(sv), \
+#define SvIOKp_on(sv)          (assert_not_glob(sv) SvRELEASE_IVX(sv), \
                                    SvFLAGS(sv) |= SVp_IOK)
 #define SvNOKp(sv)             (SvFLAGS(sv) & SVp_NOK)
-#define SvNOKp_on(sv)          (SvFLAGS(sv) |= SVp_NOK)
+#define SvNOKp_on(sv)          (assert_not_glob(sv) SvFLAGS(sv) |= SVp_NOK)
 #define SvPOKp(sv)             (SvFLAGS(sv) & SVp_POK)
-#define SvPOKp_on(sv)          (assert_not_ROK(sv)                     \
+#define SvPOKp_on(sv)          (assert_not_ROK(sv) assert_not_glob(sv) \
                                 SvFLAGS(sv) |= SVp_POK)
 
 #define SvIOK(sv)              (SvFLAGS(sv) & SVf_IOK)
-#define SvIOK_on(sv)           (SvRELEASE_IVX(sv), \
+#define SvIOK_on(sv)           (assert_not_glob(sv) SvRELEASE_IVX(sv), \
                                    SvFLAGS(sv) |= (SVf_IOK|SVp_IOK))
 #define SvIOK_off(sv)          (SvFLAGS(sv) &= ~(SVf_IOK|SVp_IOK|SVf_IVisUV))
 #define SvIOK_only(sv)         (SvOK_off(sv), \
                                    SvFLAGS(sv) |= (SVf_IOK|SVp_IOK))
-#define SvIOK_only_UV(sv)      (SvOK_off_exc_UV(sv), \
+#define SvIOK_only_UV(sv)      (assert_not_glob(sv) SvOK_off_exc_UV(sv), \
                                    SvFLAGS(sv) |= (SVf_IOK|SVp_IOK))
 
 #define SvIOK_UV(sv)           ((SvFLAGS(sv) & (SVf_IOK|SVf_IVisUV))   \
@@ -807,7 +808,8 @@ Set the actual length of the string which is in the SV.  See C<SvIV_set>.
 #define SvIsUV_off(sv)         (SvFLAGS(sv) &= ~SVf_IVisUV)
 
 #define SvNOK(sv)              (SvFLAGS(sv) & SVf_NOK)
-#define SvNOK_on(sv)           (SvFLAGS(sv) |= (SVf_NOK|SVp_NOK))
+#define SvNOK_on(sv)           (assert_not_glob(sv) \
+                                SvFLAGS(sv) |= (SVf_NOK|SVp_NOK))
 #define SvNOK_off(sv)          (SvFLAGS(sv) &= ~(SVf_NOK|SVp_NOK))
 #define SvNOK_only(sv)         (SvOK_off(sv), \
                                    SvFLAGS(sv) |= (SVf_NOK|SVp_NOK))
@@ -837,14 +839,14 @@ in gv.h: */
 #define SvUTF8_off(sv)         (SvFLAGS(sv) &= ~(SVf_UTF8))
 
 #define SvPOK(sv)              (SvFLAGS(sv) & SVf_POK)
-#define SvPOK_on(sv)           (assert_not_ROK(sv)                     \
+#define SvPOK_on(sv)           (assert_not_ROK(sv) assert_not_glob(sv) \
                                 SvFLAGS(sv) |= (SVf_POK|SVp_POK))
 #define SvPOK_off(sv)          (SvFLAGS(sv) &= ~(SVf_POK|SVp_POK))
-#define SvPOK_only(sv)         (assert_not_ROK(sv)                     \
+#define SvPOK_only(sv)         (assert_not_ROK(sv) assert_not_glob(sv) \
                                 SvFLAGS(sv) &= ~(SVf_OK|SVf_AMAGIC|    \
                                                  SVf_IVisUV|SVf_UTF8), \
                                    SvFLAGS(sv) |= (SVf_POK|SVp_POK))
-#define SvPOK_only_UTF8(sv)    (assert_not_ROK(sv)                     \
+#define SvPOK_only_UTF8(sv)    (assert_not_ROK(sv) assert_not_glob(sv) \
                                 SvFLAGS(sv) &= ~(SVf_OK|SVf_AMAGIC|    \
                                                  SVf_IVisUV),          \
                                    SvFLAGS(sv) |= (SVf_POK|SVp_POK))
@@ -997,13 +999,20 @@ in gv.h: */
 #    define SvSTASH(sv)        (0 + ((XPVMG*)  SvANY(sv))->xmg_stash)
 #  endif
 #else
-#  define SvPVX(sv) ((sv)->sv_u.svu_pv)
 #  define SvCUR(sv) ((XPV*) SvANY(sv))->xpv_cur
 #  define SvLEN(sv) ((XPV*) SvANY(sv))->xpv_len
 #  define SvEND(sv) ((sv)->sv_u.svu_pv + ((XPV*)SvANY(sv))->xpv_cur)
 
 #  if defined (DEBUGGING) && defined(__GNUC__) && !defined(PERL_GCC_BRACE_GROUPS_FORBIDDEN)
 /* These get expanded inside other macros that already use a variable _sv  */
+#    define SvPVX(sv)                                                  \
+       (*({ SV *const _svi = (SV *) sv;                                \
+           assert(SvTYPE(_svi) >= SVt_PV);                             \
+           assert(SvTYPE(_svi) != SVt_PVAV);                           \
+           assert(SvTYPE(_svi) != SVt_PVHV);                           \
+           assert(!isGV_with_GP(_svi));                                \
+           &((_svi)->sv_u.svu_pv);                                     \
+        }))
 #    define SvIVX(sv)                                                  \
        (*({ SV *const _svi = (SV *) sv;                                \
            assert(SvTYPE(_svi) == SVt_IV || SvTYPE(_svi) >= SVt_PVIV); \
@@ -1039,6 +1048,7 @@ in gv.h: */
            &(((XPVMG*) SvANY(_svi))->xmg_stash);                       \
          }))
 #  else
+#   define SvPVX(sv) ((sv)->sv_u.svu_pv)
 #    define SvIVX(sv) ((XPVIV*) SvANY(sv))->xiv_iv
 #    define SvUVX(sv) ((XPVUV*) SvANY(sv))->xuv_uv
 #    define SvNVX(sv) ((XPVNV*) SvANY(sv))->xnv_nv
@@ -1100,6 +1110,7 @@ in gv.h: */
                (((XPV*)  SvANY(sv))->xpv_cur = (val)); } STMT_END
 #define SvLEN_set(sv, val) \
        STMT_START { assert(SvTYPE(sv) >= SVt_PV); \
+               assert(!isGV_with_GP(sv));      \
                (((XPV*)  SvANY(sv))->xpv_len = (val)); } STMT_END
 #define SvEND_set(sv, val) \
        STMT_START { assert(SvTYPE(sv) >= SVt_PV); \
@@ -1681,6 +1692,11 @@ Returns a pointer to the character buffer.
 #define boolSV(b) ((b) ? &PL_sv_yes : &PL_sv_no)
 
 #define isGV(sv) (SvTYPE(sv) == SVt_PVGV)
+/* If I give every macro argument a different name, then there won't be bugs
+   where nested macros get confused. Been there, done that.  */
+#define isGV_with_GP(pwadak) \
+       (((SvFLAGS(pwadak) & (SVp_POK|SVp_SCREAM)) == SVp_SCREAM)       \
+       && (SvTYPE(pwadak) == SVt_PVGV || SvTYPE(pwadak) == SVt_PVLV))
 
 #define SvGROW(sv,len) (SvLEN(sv) < (len) ? sv_grow(sv,len) : SvPVX(sv))
 #define SvGROW_mutable(sv,len) \