Re: add_path optimization

On 1 Feb 2009, at 21:35, Robert Haas wrote:

> On Sun, Feb 1, 2009 at 3:25 PM, Grzegorz Jaskiewicz <gj(at)pointblue(dot)com(dot)pl
> > wrote:
>> I don't like the fact that you hardcoded that here. I know that you
>> are
>> trying to pass on few calls in one go here, but still... ugly.
>
> Well, I think you'll find that using a dynamically sized data
> structure destroys the possibility of squeezing any additional
> performance out of this part of the code. The nice thing about
> fixed-size data structures is that they cost essentially nothing to
> stack-allocate; you just move the stack pointer and away you go. We
> should in fact be looking for MORE places where we can avoid the use
> of constructs like List, since the second-highest CPU hog in my tests
> was AllocSetAlloc(), beaten out only by add_path(). With this patch
> applied, AllocSetAlloc() moves up to first.
well, true - but also, statically allocated table, without any
predefined size (with #DEFINE) , and no boundary check - is bad as well.
I suppose , this code is easy enough to let it be with your changes,
but I would still call it not pretty.
>

> Hmm, well I didn't either, but there's this handy tool called gprof
> that you might want to try out. I wouldn't be wasting my time
> patching this part of the code if it didn't make a difference, and in
> fact if you do 10% of the amount of benchmarking that I did in the
> process of creating this patch, you will find that it in fact does
> make a difference.
>
To be honest, I really didn't had a time to run it down with your
patch and gprof. I believe that you did that already, hence your
suggestion, right ?
Actually - if you did profile postgresql with bunch of queries, I
wouldn't mind to see results of it - I don't know whether it makes
sense to send that to the list (I would think it does), but if it is
too big, or something - you could send it to me in private.

> It's already static to that .c file, so the compiler likely will
> inline it. In fact, I suspect you will find that removing the static
> keyword from the implementation of that function in CVS HEAD is itself
> sufficient to produce a small but measurable slowdown in planning of
> large join trees, exactly because it will defeat inlining.
that depends on many things, including whether optimizations are on or
not.
Because that function basically consists of two ifs essentially - it
could easily be turned into two separate inlines/macros - that would
remove any function's specific overhead (stack alloc, etc, etc).