On 12/16/2014 11:04 AM, David Rowley wrote:> These are some very
promising performance increases.
>
> I've done a quick pass of reading the patch. I currently don't have a
> system with a 128bit int type, but I'm working on that.
Sorry for taking some time to get back. I have been busy before
Christmas. A new version of the patch is attached.
> This fragment needs fixed to put braces on new lines
Fixed!
> It also looks like your OIDs have been nabbed by some jsonb stuff.
Fixed!
> I'm also wondering why in numeric_int16_sum() you're doing:
>
> #else
> return numeric_sum(fcinfo);
> #endif
>
> but you're not doing return int8_accum() in the #else part
> of int8_avg_accum()
> The same goes for int8_accum_inv() and int8_avg_accum_inv(), though
> perhaps you're doing it here because of the elog() showing the wrong
> function name. Although that's a pretty much "shouldn't ever happen"
> case that mightn't be worth worrying about.
No strong reason. I did it for symmetry with int2_accum() and int4_accum().
> Also since I don't currently have a machine with a working int128, I
> decided to benchmark master vs patched to see if there was any sort of
> performance regression due to numeric_int16_sum calling numeric_sum, but
> I'm a bit confused with the performance results as it seems there's
> quite a good increase in performance with the patch, I'd have expected
> there to be no change.
Weird, I noticed similar results when doing my benchmarks, but given
that I did not change the accumulator function other than adding an
ifdef I am not totally sure if this difference is real.
master
tps = 1.001984 (excluding connections establishing)
Without int128
tps = 1.014511 (excluding connections establishing)
With int128
tps = 3.185956 (excluding connections establishing)
--
Andreas Karlsson
