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
>From 48dc1fae374896cf1de0c4e650f66d5d2eb44bce Mon Sep 17 00:00:00 2001
From: David Mitchell <da...@iabyn.com>
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
