Re: why is imp_xxh stored in an SV?

Dave Mitchell <[email protected]> Wed, 18 Apr 2012 11:55:45 +0100

Newsgroups	gmane.comp.lang.perl.modules.dbi.sybase.devel
Message-ID	<[email protected]>

On Mon, Apr 16, 2012 at 03:12:07PM +0100, Tim Bunce wrote:
> On Fri, Feb 24, 2012 at 05:18:14PM +0000, Dave Mitchell wrote:
> > (I confess I haven't looked closely at this yet, but...)
> 
> [And I confess I forgot about this message - sorry for the delay.]
> 
> > I seems that the imp_xxh_t structure is stored in the PVX of an SV
> > pointed to by mg_obj.
> 
> The DBI is very old. I suspect it's was done that way because mg_obj had
> to point to an SV in very old versions of perl. git blame says Larry
> added MGf_REFCOUNTED to mg.h on 1994-05-04. The DBI in a very
> rudamentary form back then. So it might have been that.
> 
> I also recall problems with global destruction order, in the early days,

To keep things as simple as possible, I've left mg_obj pointing to
the SV containing the struct, but added using mg_ptr (with mg_len == 0)
that caches a direct pointer to the structure, skipping the need to
indirect via the SV and PVX(sv).

I've also updated dbih_getcom() to directly handle the common code path
when retrieving the handle, rather than doing a dTHX and punting to
dbih_getcom2().

This gives a very slight performance boost, and doesn't have any binary or
backwards compatibility issues (hopefully).

Note that this and the just-submitted DBIS patch represent the end of
my work on improving performance on DBI, (not counting any remedial work
that may be required).

Regards,

Dave

-- 
Diplomacy is telling someone to go to hell in such a way that they'll
look forward to the trip

0001-cache-imp_xxh-in-mg_ptr.patch (text/plain, 3.6 KB)

From 48dc1fae374896cf1de0c4e650f66d5d2eb44bce Mon Sep 17 00:00:00 2001
From: David Mitchell <[email protected]>
Date: Wed, 18 Apr 2012 10:25:12 +0100
Subject: [PATCH] cache imp_xxh in mg_ptr

Currently, the imp_xxh_t structure is embedded in the PVX of an SV.
This SV is pointed to by mg->obj.

Make it so that a direct pointer to the imp_xxh is also cached in the
mg_ptr field (with mg_len set to 0 so that it won't be freed), thus
skipping a level or two of indirection when retrieving it.

At the same time, make dbih_getcom() short-circuit the common case, only
declaring dTHX and calling out to dbih_getcom2() in "unusual"
circumstances.

Together, this makes things infinitesimally quicker, especially on
threaded builds.
---
 DBI.xs |   41 ++++++++++++++++++++++++++---------------
 1 files changed, 26 insertions(+), 15 deletions(-)

diff --git a/DBI.xs b/DBI.xs
index a9e9000..66ca5da 100644
--- a/DBI.xs
+++ b/DBI.xs
@@ -1096,17 +1096,31 @@ dbih_inner(pTHX_ SV *orv, const char *what)
 static imp_xxh_t *
 dbih_getcom(SV *hrv) /* used by drivers via DBIS func ptr */
 {
-    dTHX;
-    imp_xxh_t *imp_xxh = dbih_getcom2(aTHX_ hrv, 0);
-    if (!imp_xxh)       /* eg after take_imp_data */
-        croak("Invalid DBI handle %s, has no dbi_imp_data", neatsvpv(hrv,0));
-    return imp_xxh;
+    MAGIC *mg;
+    SV *sv;
+
+    /* short-cut common case */
+    if (   SvROK(hrv)
+        && (sv = SvRV(hrv))
+        && SvRMAGICAL(sv)
+        && (mg = SvMAGIC(sv))
+        && mg->mg_type == DBI_MAGIC
+        && mg->mg_ptr
+    )
+        return (imp_xxh_t *) mg->mg_ptr;
+
+    {
+        dTHX;
+        imp_xxh_t *imp_xxh = dbih_getcom2(aTHX_ hrv, 0);
+        if (!imp_xxh)       /* eg after take_imp_data */
+            croak("Invalid DBI handle %s, has no dbi_imp_data", neatsvpv(hrv,0));
+        return imp_xxh;
+    }
 }
 
 static imp_xxh_t *
 dbih_getcom2(pTHX_ SV *hrv, MAGIC **mgp) /* Get com struct for handle. Must be fast.    */
 {
-    imp_xxh_t *imp_xxh;
     MAGIC *mg;
     SV *sv;
 
@@ -1141,14 +1155,7 @@ dbih_getcom2(pTHX_ SV *hrv, MAGIC **mgp) /* Get com struct for handle. Must be f
     if (mgp)    /* let caller pickup magic struct for this handle */
         *mgp = mg;
 
-    if (!mg->mg_obj)    /* eg after take_imp_data */
-        return 0;
-
-    /* ignore 'cast increases required alignment' warning       */
-    /* not a problem since we created the pointers anyway.      */
-    imp_xxh = (imp_xxh_t*)(void*)SvPVX(mg->mg_obj);
-
-    return imp_xxh;
+    return (imp_xxh_t *) mg->mg_ptr;
 }
 
 
@@ -1485,7 +1492,10 @@ dbih_setup_handle(pTHX_ SV *orv, char *imp_class, SV *parent, SV *imp_datasv)
     }
 
     /* Use DBI magic on inner handle to carry handle attributes         */
-    sv_magic(SvRV(h), dbih_imp_sv, DBI_MAGIC, Nullch, 0);
+    /* Note that we store the imp_sv in mg_obj, but as a shortcut,      */
+    /* also store a direct pointer to imp, aka PVX(dbih_imp_sv),        */
+    /* in mg_ptr (with mg_len set to null, so it wont be freed)         */
+    sv_magic(SvRV(h), dbih_imp_sv, DBI_MAGIC, (char*)imp, 0);
     SvREFCNT_dec(dbih_imp_sv);  /* since sv_magic() incremented it      */
     SvRMAGICAL_on(SvRV(h));     /* so DBI magic gets sv_clear'd ok      */
 
@@ -5026,6 +5036,7 @@ take_imp_data(h)
     dbih_getcom2(aTHX_ h, &mg); /* get the MAGIC so we can change it    */
     imp_xxh_sv = mg->mg_obj;    /* take local copy of the imp_data pointer */
     mg->mg_obj = Nullsv;        /* sever the link from handle to imp_xxh */
+    mg->mg_ptr = NULL;          /* and sever the shortcut too */
     if (DBIc_TRACE_LEVEL(imp_xxh) >= 9)
         sv_dump(imp_xxh_sv);
     /* --- housekeeping */
-- 
1.7.4.4