Re: why is imp_xxh stored in an SV?

2012-04-18 Thread Dave Mitchell 
On Mon, Apr 16, 2012 at 03:12:07PM +0100, Tim Bunce wrote:
 On Fri, Feb 24, 2012 at 05:18:14PM +, 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;

Re: why is imp_xxh stored in an SV?

2012-04-18 Thread Tim Bunce 
On Wed, Apr 18, 2012 at 11:55:45AM +0100, Dave Mitchell wrote:
 
 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).

Looks good. I've uploadded DBI-1.618_901.tar.gz for cpantesters to chew on.

 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).

Many thanks again for your contributions to the DBI, Dave.
They are very much appreciated.

Tim.

Re: why is imp_xxh stored in an SV?

2012-04-18 Thread Martin J. Evans 

Thanks Dave. I believe your work was sponsored but all the same I'd like you to 
know I very much appreciate your work on these changes.

Martin
--
Martin J. Evans
Easysoft Limited
http://www.easysoft.com

Re: why is imp_xxh stored in an SV?

2012-04-18 Thread H.Merijn Brand 
On Wed, 18 Apr 2012 12:59:25 +0100, Tim Bunce tim.bu...@pobox.com
wrote:

 On Wed, Apr 18, 2012 at 11:55:45AM +0100, Dave Mitchell wrote:
  
  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).

*\o/*

 Looks good. I've uploadded DBI-1.618_901.tar.gz for cpantesters to chew on.

Built, tested, and installed on my (development) laptop with 
perl 5.014001 on linux (i686-linux-64int-ld)
Linux 3.1.9-1.4-desktop  i386 Core(TM) i7-2620M CPU @ 2.70GHz/800(4) i686  7951 
Mb

#

DBD::CSV-0.34:

# Using DBIversion 1.619
# Using DBD::File  version 0.40
# Using SQL::Statement version 1.33
# Using Text::CSV_XS   version 0.88
# DBD::CSV 0.34 using Text::CSV_XS (0.88)
#   DBD::File  0.40 using IO::File (1.15)
# DBI::DBD::SqlEngine  0.03 using SQL::Statement 1.33
# DBI  1.619
# OS   linux (2.6.37.6-31-desktop)
# Perl 5.014001 (i686-linux-64int-ld)

All tests successful.
Files=23, Tests=709,  2 wallclock secs ( 0.13 usr  0.03 sys +  1.80 cusr  0.17 
csys =  2.13 CPU)
Result: PASS

#

DBD::Pg-2.19.2:

Configuring DBD::Pg 2.19.2
PostgreSQL version: 90103 (default port: 5432)

# DBI Version 1.619
# DBD::Pg Version 2.19.2
# PerlVersion 5.14.1
# OS  linux
# PostgreSQL (compiled)   90103
# PostgreSQL (target) 90103
# PostgreSQL (reported)   PostgreSQL 9.1.3 on i586-suse-linux-gnu, compiled 
by gcc (SUSE Linux) 4.6.2, 32-bit
# Default port5432
# DBI_DSN dbi:Pg:
# DBI_USERpostgres
# Test schema dbd_pg_testschema
# LANGen_US.UTF-8
# array_nulls on
# backslash_quote safe_encoding
# client_encoding UTF8
# server_encoding UTF8
# standard_conforming_strings on
# Adjusted:   DBI_DSN

All tests successful.
Files=15, Tests=1677, 22 wallclock secs ( 0.29 usr  0.02 sys +  1.07 cusr  0.16 
csys =  1.54 CPU)
Result: PASS

#

DBD::Oracle-1.42:

Client:
Installing on a linux, Ver#2.6
Using Oracle in /usr/lib/oracle/11.2/client
DEFINE _SQLPLUS_RELEASE = 1102000200 (CHAR)
Oracle version 11.2.0.2 (11.2)

Server
Oracle Database 11g Enterprise Edition Release 11.2.0.3.0 - 64bit Production
With the Partitioning, OLAP, Data Mining and Real Application Testing options

# Carp version is 1.25
# Config version is undefined
# DBI version is 1.619
# Data::Dumper version is 2.131
# Devel::Peek version is 1.07
# DynaLoader version is 1.13
# Encode version is 2.44
# Exporter version is 5.66
# ExtUtils::MakeMaker version is 6.62
# Math::BigInt version is 1.997
# Oraperl version is 1.44
# Scalar::Util version is 1.25
# Test::More version is 0.98
# Thread::Semaphore version is 2.12
# strict version is 1.04
# utf8 version is 1.09
# vars version is 1.02
# warnings version is 1.12

All tests successful.
Files=36, Tests=2548, 30 wallclock secs ( 0.75 usr  0.09 sys +  4.31 cusr  0.75 
csys =  5.90 CPU)
Result: PASS

#

DBD::mysql-4.020:

mysql  Ver 15.1 Distrib 5.3.6-MariaDB, for suse-linux-gnu (i686) using readline 
5.1

All tests successful.
Files=40, Tests=890,  7 wallclock secs ( 0.21 usr  0.04 sys +  1.57 cusr  0.24 
csys =  2.06 CPU)
Result: PASS

#

DBD::Unify-0.83:

on HP-UX 11.23/64 U  rx1620/64 Itanium 2/1600(2) ia64  2037Mb
using perl5.14.2 IA64.ARCHREV_0-LP64-ld

System:   perl5.014002 hpux
UNIFY:/pro/asql/v83I
Database Version: 38
Revision: 8.3I
Nap Option:   SELECT
Operating System: HP-UX B.11.23 U ia64

Using DBI 1.619 (for perl 5.014002 on IA64.ARCHREV_0-LP64-ld) installed in 
/u/usr/merijn/.cpan/build/DBI-1.618_901/blib/arch/auto/DBI/

All tests successful.
Files=21, Tests=3721, 34 wallclock secs ( 1.14 usr  0.14 sys + 20.17 cusr  3.80 
csys = 25.25 CPU)
Result: PASS


IMPRESSIVE!

  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

Re: why is imp_xxh stored in an SV?

2012-04-18 Thread Dave Mitchell 
 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).

PS - here are some final timings, comparing my
'while ($sth-fetch()) {$c++}'
test loop (with unchanged DBD-mysql-4.020), against
(a) DBI r15098, just before the start of my changes;
(b) DBI r15266 plus my recent DBIS and mg_ptr tweaks:

Times are in CPU seconds, lower is better:

 unthreaded|  threaded 
5.8.1   5.8.9  5.15.6  |  5.8.1   5.8.9  5.15.7
-   -  -- -+  -   -  --
(a) 15.28   14.84   15.11  |  42.86   48.92   47.72
(b) 13.01   12.44   12.68  |  27.81   30.19   17.67


Note especially how on modern threaded perls (i.e. = 5.10), the timing
is approaching that of unthreaded. (I have a separate patch pending
against DBD-mysql which brings that 17.67 down by another 2.5 seconds)

All very satisfying :-)


-- 
Standards (n). Battle insignia or tribal totems.

Re: why is imp_xxh stored in an SV?

2012-04-18 Thread Tim Bunce 
On Wed, Apr 18, 2012 at 04:35:07PM +0100, Dave Mitchell wrote:
  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).
 
 PS - here are some final timings, comparing my
 'while ($sth-fetch()) {$c++}'
 test loop (with unchanged DBD-mysql-4.020), against
 (a) DBI r15098, just before the start of my changes;
 (b) DBI r15266 plus my recent DBIS and mg_ptr tweaks:
 
 Times are in CPU seconds, lower is better:
 
unthreaded|  threaded 
 5.8.1   5.8.9  5.15.6  |  5.8.1   5.8.9  5.15.7
   -   -  -- -+  -   -  --
 (a) 15.28   14.84   15.11  |  42.86   48.92   47.72
 (b) 13.01   12.44   12.68  |  27.81   30.19   17.67
 
 
 Note especially how on modern threaded perls (i.e. = 5.10), the timing
 is approaching that of unthreaded. (I have a separate patch pending
 against DBD-mysql which brings that 17.67 down by another 2.5 seconds)
 
 All very satisfying :-)

Indeed!  Great work.  Many thanks again Dave.

Tim.

Re: why is imp_xxh stored in an SV?

2012-04-16 Thread Tim Bunce 
On Fri, Feb 24, 2012 at 05:18:14PM +, 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,
that led to other oddities like the DBD::_mem::{dr,db,st} classes that
the imp_xxh_t struct gets blessed into:

strcpy(imp_mem_name, imp_class);
strcat(imp_mem_name, _mem);
imp_mem_stash = gv_stashpv(imp_mem_name, FALSE);
...
dbih_imp_rv = newRV_inc(dbih_imp_sv);   /* just needed for sv_bless */
sv_bless(dbih_imp_rv, imp_mem_stash);
sv_free(dbih_imp_rv);

I hope that's ancient history now, but it might also be a dark corner
hiding a dragon.

 Is there any reason why it can't be just directly malloced and pointed
 to from mg_ptr instead?

Other than the issue above I can't think of any, except that it would
require drivers to be re-compiled.

Normally I'd say that requiring users to undergo the pain of a driver
recompile probably wouldn't be worth the (currently only presumed) gains.
But since we're already looking at a binary-incompatible change for the
DBIS macro, which I've also been trying to avoid, it looks more tempting.

If you can show a useful speed gain from the change (and no global
destruction issues on 5.8.3+) then I'll say we should require a
driver-compile and roll the new DBIS changes into that.

Sound good?

Tim.