Try again to improve method caching

Subject: Re: Autoloading broken?!

Chip Salzenberg writes:
>
> According to Ilya Zakharevich:
> >
>
> Well, I can only guess what your message was going to say...  But if
> you build stock _14, you'll find that MakeMaker doesn't work, because
> SelfLoader doesn't work.
>
> I think it has something to do with your patch finding completely
> empty functions (no XSUB and no code) and ignoring -- or even removing
> -- them, under the assumption they're bad cache entries.  But that
> approach can make declarations like "sub Foo::bar;" evaporate into
> nothingness, when such declarations are sometimes used to force a call
> to Foo::AUTOLOAD().

In a correct package - FOO. I think it would call some AUTOLOAD
anyway, this is why this case slipped through my testing.

> That's my understanding, anyway.

Thanks, I found this too (and fixed it). I think it should work better
now. So far only other places which I found broken by my previous
patch are "overloading + AUTOLOADing", and "->can + AUTOLOAD".

These 3 cases work now (after correcting a bug in overload.t's AUTOLOAD).

p5p-msgid: <199612240113.UAA09487@monk.mps.ohio-state.edu>

diff --git a/gv.c b/gv.c
index fed7eca..6dd8ad0 100644 (file)
--- a/gv.c
+++ b/gv.c
@@ -143,19 +143,20 @@ I32 level;
     if (SvTYPE(topgv) != SVt_PVGV)
        gv_init(topgv, stash, name, len, TRUE);
 
-    if (cv=GvCV(topgv)) {
-       if (GvCVGEN(topgv) >= sub_generation)
-           return topgv;       /* valid cached inheritance */
-       if (!GvCVGEN(topgv)) {  /* not an inheritance cache */
-           return topgv;
-       }
-       else {
-           /* stale cached entry, just junk it */
-           GvCV(topgv) = cv = 0;
-           GvCVGEN(topgv) = 0;
+    if (cv = GvCV(topgv)) {
+       if (CvXSUB(cv) || CvROOT(cv) || CvGV(cv)) { /* Not deleted, possibly autoloaded. */
+           if (GvCVGEN(topgv) >= sub_generation)
+               return topgv;   /* valid cached inheritance */
+           if (!GvCVGEN(topgv)) {      /* not an inheritance cache */
+               return topgv;
+           }
        }
+       /* stale cached entry, just junk it */
+       SvREFCNT_dec(cv);
+       GvCV(topgv) = cv = 0;
+       GvCVGEN(topgv) = 0;
     }
-    /* if cv is still set, we have to free it if we find something to cache */
+    /* Now cv = 0, and there is no cv in topgv. */
 
     gvp = (GV**)hv_fetch(stash,"ISA",3,FALSE);
     if (gvp && (gv = *gvp) != (GV*)&sv_undef && (av = GvAV(gv))) {
@@ -172,13 +173,9 @@ I32 level;
            }
            gv = gv_fetchmeth(basestash, name, len, level + 1);
            if (gv) {
-               if (cv) {                               /* junk old undef */
-                   assert(SvREFCNT(topgv) > 1);
-                   SvREFCNT_dec(topgv);
-                   SvREFCNT_dec(cv);
-               }
                GvCV(topgv) = GvCV(gv);                 /* cache the CV */
                GvCVGEN(topgv) = sub_generation;        /* valid for now */
+               SvREFCNT_inc(GvCV(gv));
                return gv;
            }
        }
@@ -187,13 +184,9 @@ I32 level;
     if (!level) {
        if (lastchance = gv_stashpvn("UNIVERSAL", 9, FALSE)) {
            if (gv = gv_fetchmeth(lastchance, name, len, level + 1)) {
-               if (cv) {                               /* junk old undef */
-                   assert(SvREFCNT(topgv) > 1);
-                   SvREFCNT_dec(topgv);
-                   SvREFCNT_dec(cv);
-               }
                GvCV(topgv) = GvCV(gv);                 /* cache the CV */
                GvCVGEN(topgv) = sub_generation;        /* valid for now */
+               SvREFCNT_inc(GvCV(gv));
                return gv;
            }
        }

diff --git a/sv.c b/sv.c
index 87a1a2d..47869b1 100644 (file)
--- a/sv.c
+++ b/sv.c
@@ -1949,11 +1949,14 @@ register SV *sstr;
                                    (CvROOT(cv) || CvXSUB(cv)) )
                                warn("Subroutine %s redefined",
                                    GvENAME((GV*)dstr));
-                           SvFAKE_on(cv);
+                           if (SvREFCNT(cv) == 1)
+                               SvFAKE_on(cv);
                        }
                    }
+                   sub_generation++;
                    if (GvCV(dstr) != (CV*)sref) {
                        GvCV(dstr) = (CV*)sref;
+                       GvCVGEN(dstr) = 0; /* Switch off cacheness. */
                        GvASSUMECV_on(dstr);
                    }
                    if (curcop->cop_stash != GvSTASH(dstr))