On Tue, Dec 6, 2011 at 8:13 PM, Peter Geoghegan <peter@2ndquadrant.com> wrote:
> On 7 December 2011 00:18, Robert Haas <robertmhaas@gmail.com> wrote:
>> Works for me. I think we should go ahead and get this part committed
>> first, and then we can look at the inlining stuff as a further
>> optimization for certain cases...
>
> Do you mean just inlining, or inlining and the numerous other
> optimisations that my patch had?
Whichever you like. But I think part of the point here is to
disentangle those optimizations from each other and decide how broadly
it makes sense to apply each one. Avoiding the FunctionCallInfo stuff
is one, and it seems like we can apply that to a wide variety of data
types (maybe all of them) for both in-memory and on-disk sorting, plus
btree index ops, merge joins, and merge append. The gains may be
modest, but they will benefit many use cases. Your original patch
targets a much narrower use case (in-memory sorting of POD types) but
the benefits are larger. We don't have to pick between a general but
small optimization and a narrower but larger one; we can do both.
In this regard, I think Heikki's remarks upthread are worth some
thought. If inlining is a win just because it avoids saving and
restoring registers or allows better instruction scheduling, then
inlining is the (probably?) the only way to get the benefit. But if
most of the benefit is in having a separate path for the
single-sort-key case, we can do that without duplicating the qsort()
code and get the benefit for every data type without much code bloat.
I'd like to see us dig into that a little, so that we get the broadest
possible benefit out of this work. It doesn't bother me that not
every optimization will apply to every case, and I don't object to
optimizations that are intrinsically narrow (within some reasonable
limits). But I'd rather not take what could be a fairly broad-based
optimization and apply it only narrowly, all things being equal.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
