Re: Changes to make DBIS more efficient

2012-04-18 Thread Tim Bunce
Perfect! Applied. Many thanks Dave.

Tim.

On Wed, Apr 18, 2012 at 11:48:31AM +0100, Dave Mitchell wrote:
> On Fri, Mar 02, 2012 at 05:04:29PM +, Dave Mitchell wrote:
> > On Fri, Mar 02, 2012 at 03:23:30PM +, Dave Mitchell wrote:
> > > I'd be happy in principle, although some guidance would be welcome
> > 
> > Also in particular, what needs to continue happening across an API
> > change? Is it just that a driver, compiled and installed against an old
> > DBI, must continue to work if the DBI is upgraded?
> 
> I've now attached a revised DBIS patch. The only difference is that
> the new DBI continues to store a pointer to the dbi_state struct in
> $DBI::_dbistate, so that DBD modules compiled against the old DBI can
> continue to retrieve the struct the old way. Once recompiled, they'll
> start to use the new method.
> 
> IIUC, this means that there are no binary or backwards compatibility
> issues, and no API version numbers need bumping.
> 
> -- 
> The optimist believes that he lives in the best of all possible worlds.
> As does the pessimist.

> From 0fb5fd6a7099c5fc40bd0b9d40b8b34d748533b5 Mon Sep 17 00:00:00 2001
> From: David Mitchell 
> Date: Fri, 10 Feb 2012 13:11:13 +
> Subject: [PATCH] under ithreads, make DBIS efficient for DBD::*
> 
> rather than the slow looking up of $DBI::_dbistate on every use of DBIS,
> convert DBIS into a call to a C-level function that returns the address of
> the dbi_state struct.
> 
> Since the C-level function is only directly callable from DBI, store its
> address in an XSUB, so that the DBD:* modules can retrieve the function's
> address and cache it in a static var.
> 
> We continue to store the dbi_state struct address within $DBI::_dbistate
> too, so that DBD modules compiled against an older DBI will continue to
> work if the DBI is upgraded but the DBD not recompiled.
> ---
>  DBI.pm  |4 +---
>  DBI.xs  |   39 +++
>  DBIXS.h |   39 +++
>  3 files changed, 47 insertions(+), 35 deletions(-)
> 
> diff --git a/DBI.pm b/DBI.pm
> index 4e02e8c..614ffae 100644
> --- a/DBI.pm
> +++ b/DBI.pm
> @@ -519,10 +519,8 @@ END {
>  
>  
>  sub CLONE {
> -my $olddbis = $DBI::_dbistate;
>  _clone_dbis() unless $DBI::PurePerl; # clone the DBIS structure
> -DBI->trace_msg(sprintf "CLONE DBI for new thread %s\n",
> - $DBI::PurePerl ? "" : sprintf("(dbis %x -> %x)",$olddbis, 
> $DBI::_dbistate));
> +DBI->trace_msg("CLONE DBI for new thread\n");
>  while ( my ($driver, $drh) = each %DBI::installed_drh) {
>   no strict 'refs';
>   next if defined &{"DBD::${driver}::CLONE"};
> diff --git a/DBI.xs b/DBI.xs
> index 64dc5b1..a9e9000 100644
> --- a/DBI.xs
> +++ b/DBI.xs
> @@ -127,8 +127,6 @@ char *neatsvpv _((SV *sv, STRLEN maxlen));
>  SV * preparse(SV *dbh, const char *statement, IV ps_return, IV ps_accept, 
> void *foo);
>  static meth_types get_meth_type(const char * const name);
>  
> -DBISTATE_DECLARE;
> -
>  struct imp_drh_st { dbih_drc_t com; };
>  struct imp_dbh_st { dbih_dbc_t com; };
>  struct imp_sth_st { dbih_stc_t com; };
> @@ -310,13 +308,20 @@ typedef struct {
>  
>  START_MY_CXT
>  
> -#if defined(MULTIPLICITY) || defined(PERL_OBJECT) || defined(PERL_CAPI)
> -#   undef DBIS
> -#   define DBIS (MY_CXT.dbi_state)
> -#endif
> +#undef DBIS
> +#define DBIS   (MY_CXT.dbi_state)
>  
>  #define g_dbi_last_h(MY_CXT.dbi_last_h)
>  
> +/* allow the 'static' dbi_state struct to be accessed from other files */
> +dbistate_t**
> +_dbi_state_lval(pTHX)
> +{
> +dMY_CXT;
> +return &(MY_CXT.dbi_state);
> +}
> +
> +
>  /* --- */
>  
>  static void *
> @@ -521,15 +526,12 @@ dbi_bootinit(dbistate_t * parent_dbis)
>  dbistate_t* DBISx;
>  
>  DBISx = (struct dbistate_st*)malloc_using_sv(sizeof(struct dbistate_st));
> -
> -/* publish address of dbistate so dynaloaded DBD's can find it,
> - * taking care to store the value in the same way it'll be used
> - * to avoid problems on some architectures, for example see
> - * http://rt.cpan.org/Public/Bug/Display.html?id=32309
> - */
> -sv_setiv(get_sv(DBISTATE_PERLNAME, GV_ADDMULTI), 0); /* force SvIOK */
>  DBIS = DBISx;
> -DBIS_PUBLISHED_LVALUE = DBISx;
> +
> +/* make DBIS available to DBD modules the "old" (<= 1.618) way,
> + * so that unrecompiled DBD's will still work against a newer DBI */
> +sv_setiv(get_sv("DBI::_dbistate", GV_ADDMULTI),
> +PTR2IV(MY_CXT.dbi_state));
>  
>  /* store version and size so we can spot DBI/DBD version mismatch   */
>  DBIS->check_version = check_version;
> @@ -547,12 +549,6 @@ dbi_bootinit(dbistate_t * parent_dbis)
>  DBIS->thr_owner   = PERL_GET_THX;
>  #endif
>  
> -DBISTATE_INIT; /* check DBD code to set DBIS from DBISTATE_PERLNAME */
> -
> -if (DBIS_TRACE_LEVEL > 9) {
> -sv_dump(DBISTATE_ADDRSV);
> -}
> -
>  /* store some function pointers so DBD'

Re: Changes to make DBIS more efficient

2012-04-18 Thread Dave Mitchell
On Fri, Mar 02, 2012 at 05:04:29PM +, Dave Mitchell wrote:
> On Fri, Mar 02, 2012 at 03:23:30PM +, Dave Mitchell wrote:
> > I'd be happy in principle, although some guidance would be welcome
> 
> Also in particular, what needs to continue happening across an API
> change? Is it just that a driver, compiled and installed against an old
> DBI, must continue to work if the DBI is upgraded?

I've now attached a revised DBIS patch. The only difference is that
the new DBI continues to store a pointer to the dbi_state struct in
$DBI::_dbistate, so that DBD modules compiled against the old DBI can
continue to retrieve the struct the old way. Once recompiled, they'll
start to use the new method.

IIUC, this means that there are no binary or backwards compatibility
issues, and no API version numbers need bumping.

-- 
The optimist believes that he lives in the best of all possible worlds.
As does the pessimist.
>From 0fb5fd6a7099c5fc40bd0b9d40b8b34d748533b5 Mon Sep 17 00:00:00 2001
From: David Mitchell 
Date: Fri, 10 Feb 2012 13:11:13 +
Subject: [PATCH] under ithreads, make DBIS efficient for DBD::*

rather than the slow looking up of $DBI::_dbistate on every use of DBIS,
convert DBIS into a call to a C-level function that returns the address of
the dbi_state struct.

Since the C-level function is only directly callable from DBI, store its
address in an XSUB, so that the DBD:* modules can retrieve the function's
address and cache it in a static var.

We continue to store the dbi_state struct address within $DBI::_dbistate
too, so that DBD modules compiled against an older DBI will continue to
work if the DBI is upgraded but the DBD not recompiled.
---
 DBI.pm  |4 +---
 DBI.xs  |   39 +++
 DBIXS.h |   39 +++
 3 files changed, 47 insertions(+), 35 deletions(-)

diff --git a/DBI.pm b/DBI.pm
index 4e02e8c..614ffae 100644
--- a/DBI.pm
+++ b/DBI.pm
@@ -519,10 +519,8 @@ END {
 
 
 sub CLONE {
-my $olddbis = $DBI::_dbistate;
 _clone_dbis() unless $DBI::PurePerl; # clone the DBIS structure
-DBI->trace_msg(sprintf "CLONE DBI for new thread %s\n",
-   $DBI::PurePerl ? "" : sprintf("(dbis %x -> %x)",$olddbis, 
$DBI::_dbistate));
+DBI->trace_msg("CLONE DBI for new thread\n");
 while ( my ($driver, $drh) = each %DBI::installed_drh) {
no strict 'refs';
next if defined &{"DBD::${driver}::CLONE"};
diff --git a/DBI.xs b/DBI.xs
index 64dc5b1..a9e9000 100644
--- a/DBI.xs
+++ b/DBI.xs
@@ -127,8 +127,6 @@ char *neatsvpv _((SV *sv, STRLEN maxlen));
 SV * preparse(SV *dbh, const char *statement, IV ps_return, IV ps_accept, void 
*foo);
 static meth_types get_meth_type(const char * const name);
 
-DBISTATE_DECLARE;
-
 struct imp_drh_st { dbih_drc_t com; };
 struct imp_dbh_st { dbih_dbc_t com; };
 struct imp_sth_st { dbih_stc_t com; };
@@ -310,13 +308,20 @@ typedef struct {
 
 START_MY_CXT
 
-#if defined(MULTIPLICITY) || defined(PERL_OBJECT) || defined(PERL_CAPI)
-#   undef DBIS
-#   define DBIS (MY_CXT.dbi_state)
-#endif
+#undef DBIS
+#define DBIS   (MY_CXT.dbi_state)
 
 #define g_dbi_last_h(MY_CXT.dbi_last_h)
 
+/* allow the 'static' dbi_state struct to be accessed from other files */
+dbistate_t**
+_dbi_state_lval(pTHX)
+{
+dMY_CXT;
+return &(MY_CXT.dbi_state);
+}
+
+
 /* --- */
 
 static void *
@@ -521,15 +526,12 @@ dbi_bootinit(dbistate_t * parent_dbis)
 dbistate_t* DBISx;
 
 DBISx = (struct dbistate_st*)malloc_using_sv(sizeof(struct dbistate_st));
-
-/* publish address of dbistate so dynaloaded DBD's can find it,
- * taking care to store the value in the same way it'll be used
- * to avoid problems on some architectures, for example see
- * http://rt.cpan.org/Public/Bug/Display.html?id=32309
- */
-sv_setiv(get_sv(DBISTATE_PERLNAME, GV_ADDMULTI), 0); /* force SvIOK */
 DBIS = DBISx;
-DBIS_PUBLISHED_LVALUE = DBISx;
+
+/* make DBIS available to DBD modules the "old" (<= 1.618) way,
+ * so that unrecompiled DBD's will still work against a newer DBI */
+sv_setiv(get_sv("DBI::_dbistate", GV_ADDMULTI),
+PTR2IV(MY_CXT.dbi_state));
 
 /* store version and size so we can spot DBI/DBD version mismatch   */
 DBIS->check_version = check_version;
@@ -547,12 +549,6 @@ dbi_bootinit(dbistate_t * parent_dbis)
 DBIS->thr_owner   = PERL_GET_THX;
 #endif
 
-DBISTATE_INIT; /* check DBD code to set DBIS from DBISTATE_PERLNAME */
-
-if (DBIS_TRACE_LEVEL > 9) {
-sv_dump(DBISTATE_ADDRSV);
-}
-
 /* store some function pointers so DBD's can call our functions */
 DBIS->getcom  = dbih_getcom;
 DBIS->clearcom= dbih_clearcom;
@@ -4346,6 +4342,9 @@ BOOT:
 (void)cv;
 (void)items; /* avoid 'unused variable' warning */
 dbi_bootinit(NULL);
+/* make this sub into a fake XS so it can bee seen by DBD::* modules;
+ * never actually call it as an XS sub, or it 

Re: Changes to make DBIS more efficient

2012-03-05 Thread Jens Rehsack
Am 02.03.2012 16:11, schrieb Tim Bunce:
[...]
> However, I'd still like to make the change. So I'm thinking it might be
> best to support *both* mechanisms for at least a few releases before
> removing the old one. The gives end users a "safe zone" where they
> can upgrade the DBI and then later upgrade compiled drivers after
> they've been changed.

Probably Makefile.PL's CheckConflicts() can be used to warn about
old DBD's as we know about them ...

/Jens


Re: Changes to make DBIS more efficient

2012-03-02 Thread Dave Mitchell
On Fri, Mar 02, 2012 at 03:23:30PM +, Dave Mitchell wrote:
> I'd be happy in principle, although some guidance would be welcome

Also in particular, what needs to continue happening across an API
change? Is it just that a driver, compiled and installed against an old
DBI, must continue to work if the DBI is upgraded?

-- 
"You're so sadly neglected, and often ignored.
A poor second to Belgium, When going abroad."
-- Monty Python, "Finland"


Re: Changes to make DBIS more efficient

2012-03-02 Thread Dave Mitchell
On Fri, Mar 02, 2012 at 03:11:17PM +, Tim Bunce wrote:
> On Fri, Feb 10, 2012 at 03:27:24PM +, Dave Mitchell wrote:
> I'm concerned that changing the API, and thus forcing all compiled
> drivers to be recompiled at the same time the DBI is installed,
> isn't worth the gain. Especially as drivers shouldn't be using DBIS in
> any hot code anyway.
> 
> However, I'd still like to make the change. So I'm thinking it might be
> best to support *both* mechanisms for at least a few releases before
> removing the old one. The gives end users a "safe zone" where they
> can upgrade the DBI and then later upgrade compiled drivers after
> they've been changed.
> 
> Would you be okay with reworking the change along those lines?

I'd be happy in principle, although some guidance would be welcome, as I'm
not currently clear what within my patch does (and doesn't) constitute an
API change.  Is it the addition of an extra XS function?

Is the fix as easy as something along the lines of

#if DBIXS_REVISION > NNN
#  define DBIS the-new-way
   ... and other stuff
#else
#  define DBIS the-old-way
   ... and other stuff
#endif

???

-- 
Britain, Britain, Britain! Discovered by Sir Henry Britain in
sixteen-oh-ten. Sold to Germany a year later for a pfennig and the promise
of a kiss. Destroyed in eighteen thirty-forty two, and rebuilt a week
later by a man. This we know. Hello. But what of the people of Britain?
Who they? What do? And why?   -- "Little Britain"


Changes to make DBIS more efficient

2012-03-02 Thread Tim Bunce
On Fri, Feb 10, 2012 at 03:27:24PM +, Dave Mitchell wrote:
> 
> The second patch, which makes DBIS more efficient, is a lot more complex,
> and more likely to break things (especially as it's changing a bunch of
> macros that are directly #included by the DBD drivers. You may need to
> bump API version numbers; I don't understand that bit.

I'm concerned that changing the API, and thus forcing all compiled
drivers to be recompiled at the same time the DBI is installed,
isn't worth the gain. Especially as drivers shouldn't be using DBIS in
any hot code anyway.

However, I'd still like to make the change. So I'm thinking it might be
best to support *both* mechanisms for at least a few releases before
removing the old one. The gives end users a "safe zone" where they
can upgrade the DBI and then later upgrade compiled drivers after
they've been changed.

Would you be okay with reworking the change along those lines?

Meanwhile, I'll apply your other more recent patches.

> The big saving between (2) and (3) is due to DBD::mysql still using DBIS;
> in particular, for every fetch call.

It turns out DBD::Pg does as well! I hope to fix that shortly.

Tim.