Re: [HACKERS] Optimize PL/Perl function argument passing [PATCH]
On Wed, Feb 02, 2011 at 12:10:59PM -0500, Andrew Dunstan wrote: > > On 02/02/2011 11:45 AM, Tim Bunce wrote: > >>But why are we bothering to keep $prolog at > >>all any more, if all we're going to pass it is&PL_sv_no all the > >>time? Maybe we'll have a use for it in the future, but right now we > >>don't appear to unless I'm missing something. > > > >PostgreSQL::PLPerl::NYTProf would break if it was removed, so I'd rather > >it wasn't. > > > >I could work around that if there's an easy way for perl code to tell > >what version of PostgreSQL. If there isn't I think it would be worth > >adding. > > Not really. It might well be good to add but that needs to wait for > another time. Ok. > I gather you're plugging in a replacement for mkfunc? Yes. > For now I'll add a comment to the code saying why it's there. Thanks. Tim. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Optimize PL/Perl function argument passing [PATCH]
On 02/02/2011 11:45 AM, Tim Bunce wrote: But why are we bothering to keep $prolog at all any more, if all we're going to pass it is&PL_sv_no all the time? Maybe we'll have a use for it in the future, but right now we don't appear to unless I'm missing something. PostgreSQL::PLPerl::NYTProf would break if it was removed, so I'd rather it wasn't. I could work around that if there's an easy way for perl code to tell what version of PostgreSQL. If there isn't I think it would be worth adding. Not really. It might well be good to add but that needs to wait for another time. I gather you're plugging in a replacement for mkfunc? For now I'll add a comment to the code saying why it's there. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Optimize PL/Perl function argument passing [PATCH]
On Mon, Jan 31, 2011 at 02:22:37PM -0500, Andrew Dunstan wrote: > > > On 01/15/2011 12:31 AM, Alex Hunsaker wrote: > >On Tue, Dec 7, 2010 at 07:24, Tim Bunce wrote: > >>Changes: > >> > >>Sets the local $_TD via C instead of passing an extra argument. > >>So functions no longer start with "our $_TD; local $_TD = shift;" > >> > >>Pre-extend stack for trigger arguments for slight performance gain. > >> > >>Passes installcheck. > > >Cool, surprisingly in the non trigger case I saw up to an 18% speedup. That's great. > >The trigger case remained about the same, I suppose im I/O bound. > > > >Find attached a v2 with some minor fixes, If it looks good to you Ill > >mark this as "Ready for Commit". > > > >Changes: > >- move up a declaration to make it c90 safe > >- avoid using tg_trigger before it was initialized > >- only extend the stack to the size we need (there was + 1 which > >unless I am missing something was needed because we used to push $_TD > >on the stack, but we dont any more) > > This looks pretty good. But why are we bothering to keep $prolog at > all any more, if all we're going to pass it is &PL_sv_no all the > time? Maybe we'll have a use for it in the future, but right now we > don't appear to unless I'm missing something. PostgreSQL::PLPerl::NYTProf would break if it was removed, so I'd rather it wasn't. I could work around that if there's an easy way for perl code to tell what version of PostgreSQL. If there isn't I think it would be worth adding. Tim. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Optimize PL/Perl function argument passing [PATCH]
On Mon, Jan 31, 2011 at 12:22, Andrew Dunstan wrote: > This looks pretty good. But why are we bothering to keep $prolog at all any > more, if all we're going to pass it is &PL_sv_no all the time? Maybe we'll > have a use for it in the future, but right now we don't appear to unless I'm > missing something. I don't see any reason to keep it around. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Optimize PL/Perl function argument passing [PATCH]
On 01/15/2011 12:31 AM, Alex Hunsaker wrote: On Tue, Dec 7, 2010 at 07:24, Tim Bunce wrote: Changes: Sets the local $_TD via C instead of passing an extra argument. So functions no longer start with "our $_TD; local $_TD = shift;" Pre-extend stack for trigger arguments for slight performance gain. Passes installcheck. Cool, surprisingly in the non trigger case I saw up to an 18% speedup. The trigger case remained about the same, I suppose im I/O bound. Find attached a v2 with some minor fixes, If it looks good to you Ill mark this as "Ready for Commit". Changes: - move up a declaration to make it c90 safe - avoid using tg_trigger before it was initialized - only extend the stack to the size we need (there was + 1 which unless I am missing something was needed because we used to push $_TD on the stack, but we dont any more) This looks pretty good. But why are we bothering to keep $prolog at all any more, if all we're going to pass it is &PL_sv_no all the time? Maybe we'll have a use for it in the future, but right now we don't appear to unless I'm missing something. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Optimize PL/Perl function argument passing [PATCH]
On Tue, Dec 7, 2010 at 07:24, Tim Bunce wrote: > Changes: > > Sets the local $_TD via C instead of passing an extra argument. > So functions no longer start with "our $_TD; local $_TD = shift;" > > Pre-extend stack for trigger arguments for slight performance gain. > > Passes installcheck. Cool, surprisingly in the non trigger case I saw up to an 18% speedup. The trigger case remained about the same, I suppose im I/O bound. Find attached a v2 with some minor fixes, If it looks good to you Ill mark this as "Ready for Commit". Changes: - move up a declaration to make it c90 safe - avoid using tg_trigger before it was initialized - only extend the stack to the size we need (there was + 1 which unless I am missing something was needed because we used to push $_TD on the stack, but we dont any more) Benchmarks: --- create table t (a int); create or replace function func(int) returns int as $$ return $_[0]; $$ language plperl; create or replace function trig() returns trigger as $$ $_TD->{'new'}{'a'}++; return 'MODIFY'; $$ language plperl; -- pre patch, simple function call, 3 runs SELECT sum(func(1)) from generate_series(1, 1000); Time: 30908.675 ms Time: 30916.995 ms Time: 31173.122 ms -- post patch SELECT sum(func(1)) from generate_series(1, 1000); Time: 26460.987 ms Time: 26465.480 ms Time: 25958.016 ms -- pre patch, trigger insert into t (a) select generate_series(1, 100); Time: 18186.390 ms Time: 21291.721 ms Time: 20782.238 ms -- post insert into t (a) select generate_series(1, 100); Time: 19136.633 ms Time: 21140.095 ms Time: 22062.578 ms diff --git a/src/pl/plperl/plperl.c b/src/pl/plperl/plperl.c index 5595baa..7fb69df 100644 --- a/src/pl/plperl/plperl.c +++ b/src/pl/plperl/plperl.c @@ -1421,7 +1421,7 @@ plperl_create_sub(plperl_proc_desc *prodesc, char *s, Oid fn_oid) EXTEND(SP, 4); PUSHs(sv_2mortal(newSVstring(subname))); PUSHs(sv_2mortal(newRV_noinc((SV *) pragma_hv))); - PUSHs(sv_2mortal(newSVstring("our $_TD; local $_TD=shift;"))); + PUSHs(&PL_sv_no); /* unused */ PUSHs(sv_2mortal(newSVstring(s))); PUTBACK; @@ -1493,9 +1493,7 @@ plperl_call_perl_func(plperl_proc_desc *desc, FunctionCallInfo fcinfo) SAVETMPS; PUSHMARK(SP); - EXTEND(sp, 1 + desc->nargs); - - PUSHs(&PL_sv_undef); /* no trigger data */ + EXTEND(sp, desc->nargs); for (i = 0; i < desc->nargs; i++) { @@ -1575,21 +1573,22 @@ plperl_call_perl_trigger_func(plperl_proc_desc *desc, FunctionCallInfo fcinfo, SV *td) { dSP; - SV *retval; - Trigger*tg_trigger; - int i; - int count; + SV *retval, *TDsv; + int i, count; + Trigger*tg_trigger = ((TriggerData *) fcinfo->context)->tg_trigger; ENTER; SAVETMPS; - PUSHMARK(sp); + TDsv = get_sv("_TD", GV_ADD); + SAVESPTR(TDsv); /* local $_TD */ + sv_setsv(TDsv, td); - XPUSHs(td); + PUSHMARK(sp); + EXTEND(sp, tg_trigger->tgnargs); - tg_trigger = ((TriggerData *) fcinfo->context)->tg_trigger; for (i = 0; i < tg_trigger->tgnargs; i++) - XPUSHs(sv_2mortal(newSVstring(tg_trigger->tgargs[i]))); + PUSHs(sv_2mortal(newSVstring(tg_trigger->tgargs[i]))); PUTBACK; /* Do NOT use G_KEEPERR here */ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Optimize PL/Perl function argument passing [PATCH]
On 12/13/2010 09:42 AM, Alexey Klyukin wrote: I used to work on a patch that converts postgres arrays into perl array references: http://archives.postgresql.org/pgsql-hackers/2009-11/msg01552.php I have a newer patch, which is, however, limited to one-dimensional resulting arrays. If there's an interest in that approach I can update it for the current code base, add support multi-dimensional arrays (I used to implement that, but lost the changes accidentally) and post it for review. Yes please. I had forgotten that you'd done that, so I duplicated some of your work yesterday, but it sounds like you have (or had) the guts of what I am still missing. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Optimize PL/Perl function argument passing [PATCH]
On Dec 9, 2010, at 7:32 PM, Tim Bunce wrote: > On Wed, Dec 08, 2010 at 09:21:05AM -0800, David E. Wheeler wrote: >> On Dec 8, 2010, at 9:14 AM, Tim Bunce wrote: >> Do you have any more improvements in the pipeline? >>> >>> I'd like to add $arrayref = decode_array_literal('{2,3}') and >>> maybe $hashref = decode_hstore_literal('x=>1, y=>2'). >>> I don't know how much works would be involved in those though. >> >> Those would be handy, but for arrays, at least, I'd rather have a GUC >> to turn on so that arrays are passed to PL/perl functions as array >> references. > > Understood. At this stage I don't know what the issues are so I'm > nervous of over promising (plus I don't know how much time I'll have). > It's possible a blessed ref with string overloading would avoid > backwards compatibility issues. I used to work on a patch that converts postgres arrays into perl array references: http://archives.postgresql.org/pgsql-hackers/2009-11/msg01552.php I have a newer patch, which is, however, limited to one-dimensional resulting arrays. If there's an interest in that approach I can update it for the current code base, add support multi-dimensional arrays (I used to implement that, but lost the changes accidentally) and post it for review. /A -- Alexey Klyukin http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Optimize PL/Perl function argument passing [PATCH]
On Wed, Dec 08, 2010 at 09:21:05AM -0800, David E. Wheeler wrote: > On Dec 8, 2010, at 9:14 AM, Tim Bunce wrote: > > >> Do you have any more improvements in the pipeline? > > > > I'd like to add $arrayref = decode_array_literal('{2,3}') and > > maybe $hashref = decode_hstore_literal('x=>1, y=>2'). > > I don't know how much works would be involved in those though. > > Those would be handy, but for arrays, at least, I'd rather have a GUC > to turn on so that arrays are passed to PL/perl functions as array > references. Understood. At this stage I don't know what the issues are so I'm nervous of over promising (plus I don't know how much time I'll have). It's possible a blessed ref with string overloading would avoid backwards compatibility issues. Tim. > > Another possible improvement would be rewriting encode_array_literal() > > in C, so returning arrays would be much faster. > > +1 > > Best, > > David > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers > -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Optimize PL/Perl function argument passing [PATCH]
On Dec 8, 2010, at 9:14 AM, Tim Bunce wrote: >> Do you have any more improvements in the pipeline? > > I'd like to add $arrayref = decode_array_literal('{2,3}') and > maybe $hashref = decode_hstore_literal('x=>1, y=>2'). > I don't know how much works would be involved in those though. Those would be handy, but for arrays, at least, I'd rather have a GUC to turn on so that arrays are passed to PL/perl functions as array references. > Another possible improvement would be rewriting encode_array_literal() > in C, so returning arrays would be much faster. +1 Best, David -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Optimize PL/Perl function argument passing [PATCH]
On Tue, Dec 07, 2010 at 10:00:28AM -0500, Andrew Dunstan wrote: > > > On 12/07/2010 09:24 AM, Tim Bunce wrote: > >Changes: > > > > Sets the local $_TD via C instead of passing an extra argument. > > So functions no longer start with "our $_TD; local $_TD = shift;" > > > > Pre-extend stack for trigger arguments for slight performance gain. > > > >Passes installcheck. > > Please add it to the January commitfest. Done. https://commitfest.postgresql.org/action/patch_view?id=446 > Have you measured the speedup gained from this? No. I doubt it's significant, but "every little helps" :) > Do you have any more improvements in the pipeline? I'd like to add $arrayref = decode_array_literal('{2,3}') and maybe $hashref = decode_hstore_literal('x=>1, y=>2'). I don't know how much works would be involved in those though. Another possible improvement would be rewriting encode_array_literal() in C, so returning arrays would be much faster. I'd also like to #define PERL_NO_GET_CONTEXT, which would give a useful performance boost by avoiding all the many hidden calls to lookup thread-local storage. (PERL_SET_CONTEXT() would go and instead the 'currrent interpreter' would be passed around as an extra argument.) That's a fairly mechanical change but the diff may be quite large. Tim. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Optimize PL/Perl function argument passing [PATCH]
On 12/07/2010 09:24 AM, Tim Bunce wrote: Changes: Sets the local $_TD via C instead of passing an extra argument. So functions no longer start with "our $_TD; local $_TD = shift;" Pre-extend stack for trigger arguments for slight performance gain. Passes installcheck. Please add it to the January commitfest. Have you measured the speedup gained from this? Do you have any more improvements in the pipeline? cheers anrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Optimize PL/Perl function argument passing [PATCH]
Changes: Sets the local $_TD via C instead of passing an extra argument. So functions no longer start with "our $_TD; local $_TD = shift;" Pre-extend stack for trigger arguments for slight performance gain. Passes installcheck. Tim. diff --git a/src/pl/plperl/plperl.c b/src/pl/plperl/plperl.c index 5595baa..a924cfd 100644 *** a/src/pl/plperl/plperl.c --- b/src/pl/plperl/plperl.c *** plperl_create_sub(plperl_proc_desc *prod *** 1421,1427 EXTEND(SP, 4); PUSHs(sv_2mortal(newSVstring(subname))); PUSHs(sv_2mortal(newRV_noinc((SV *) pragma_hv))); ! PUSHs(sv_2mortal(newSVstring("our $_TD; local $_TD=shift;"))); PUSHs(sv_2mortal(newSVstring(s))); PUTBACK; --- 1421,1427 EXTEND(SP, 4); PUSHs(sv_2mortal(newSVstring(subname))); PUSHs(sv_2mortal(newRV_noinc((SV *) pragma_hv))); ! PUSHs(&PL_sv_no); /* unused */ PUSHs(sv_2mortal(newSVstring(s))); PUTBACK; *** plperl_call_perl_func(plperl_proc_desc * *** 1495,1502 PUSHMARK(SP); EXTEND(sp, 1 + desc->nargs); - PUSHs(&PL_sv_undef); /* no trigger data */ - for (i = 0; i < desc->nargs; i++) { if (fcinfo->argnull[i]) --- 1495,1500 *** plperl_call_perl_trigger_func(plperl_pro *** 1583,1595 ENTER; SAVETMPS; ! PUSHMARK(sp); ! XPUSHs(td); tg_trigger = ((TriggerData *) fcinfo->context)->tg_trigger; for (i = 0; i < tg_trigger->tgnargs; i++) ! XPUSHs(sv_2mortal(newSVstring(tg_trigger->tgargs[i]))); PUTBACK; /* Do NOT use G_KEEPERR here */ --- 1581,1596 ENTER; SAVETMPS; ! SV *TDsv = get_sv("_TD", GV_ADD); ! SAVESPTR(TDsv); /* local $_TD */ ! sv_setsv(TDsv, td); ! PUSHMARK(sp); ! EXTEND(sp, 1 + tg_trigger->tgnargs); tg_trigger = ((TriggerData *) fcinfo->context)->tg_trigger; for (i = 0; i < tg_trigger->tgnargs; i++) ! PUSHs(sv_2mortal(newSVstring(tg_trigger->tgargs[i]))); PUTBACK; /* Do NOT use G_KEEPERR here */ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers