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>

2 files changed, 18 insertions, 22 deletions

diff --git a/gv.c b/gv.c
index fed7eca3dc..6dd8ad0f3c 100644
--- 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 87a1a2d017..47869b1209 100644
--- 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))