Thread: 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.
Attachment
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
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.
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
functionsas array references.
> Another possible improvement would be rewriting encode_array_literal()
> in C, so returning arrays would be much faster.
+1
Best,
David
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
>
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
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
On Tue, Dec 7, 2010 at 07:24, Tim Bunce <Tim.Bunce@pobox.com> 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, 10000000);
Time: 30908.675 ms
Time: 30916.995 ms
Time: 31173.122 ms
-- post patch
SELECT sum(func(1)) from generate_series(1, 10000000);
Time: 26460.987 ms
Time: 26465.480 ms
Time: 25958.016 ms
-- pre patch, trigger
insert into t (a) select generate_series(1, 1000000);
Time: 18186.390 ms
Time: 21291.721 ms
Time: 20782.238 ms
-- post
insert into t (a) select generate_series(1, 1000000);
Time: 19136.633 ms
Time: 21140.095 ms
Time: 22062.578 ms
Attachment
On 01/15/2011 12:31 AM, Alex Hunsaker wrote: > On Tue, Dec 7, 2010 at 07:24, Tim Bunce<Tim.Bunce@pobox.com> 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
On Mon, Jan 31, 2011 at 12:22, Andrew Dunstan <andrew@dunslane.net> 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.
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<Tim.Bunce@pobox.com> 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.
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
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.