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.

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

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(at)postgresql(dot)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(dot)Bunce(at)pobox(dot)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

On 01/15/2011 12:31 AM, Alex Hunsaker wrote:
> On Tue, Dec 7, 2010 at 07:24, Tim Bunce<Tim(dot)Bunce(at)pobox(dot)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(at)dunslane(dot)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(dot)Bunce(at)pobox(dot)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.