Hi, On 2024-02-12 17:04:23 -0600, Nathan Bossart wrote: > On Mon, Feb 12, 2024 at 01:31:30PM -0800, Andres Freund wrote: > > One thing that's worth checking is if this ends up with *worse* code when > > the > > comparators are inlined. I think none of the changed comparators will end up > > getting used with an inlined sort, but ... > > Yeah, AFAICT the only inlined sorts are in tuplesort.c and bufmgr.c, and > the patches don't touch those files. > > > The reason we could end up with worse code is that when inlining the > > comparisons would make less sense for the compiler. Consider e.g. > > return DO_COMPARE(a, b) < 0 ? > > (DO_COMPARE(b, c) < 0 ? b : (DO_COMPARE(a, c) < 0 ? c : a)) > > : (DO_COMPARE(b, c) > 0 ? b : (DO_COMPARE(a, c) < 0 ? a : c)); > > > > With a naive implementation the compiler will understand it only cares about > > a < b, not about the other possibilities. I'm not sure that's still true > > with > > the more complicated optimized version. > > You aren't kidding [0]. Besides perhaps adding a comment in > sort_template.h, is there anything else you think we should do about this > now?
I'd add also a comment to the new functions. I think it's fine otherwise. I wish there were formulation that'd be optimal for both cases, but this way we can at least adapt all places at once if either find a better formulation or change all our sorts to happen via an inline implementation of qsort or such. Greetings, Andres
