Re: Small improvement to compactify_tuples

On Tue, Sep 12, 2017 at 12:49 PM, Sokolov Yura
<funny(dot)falcon(at)postgrespro(dot)ru> wrote:
> On 2017-07-21 13:49, Sokolov Yura wrote:
>>
>> On 2017-05-17 17:46, Sokolov Yura wrote:
>>>
>>> Alvaro Herrera писал 2017-05-15 20:13:
>>>>
>>>> As I understand, these patches are logically separate, so putting them
>>>> together in a single file isn't such a great idea. If you don't edit
>>>> the patches further, then you're all set because we already have the
>>>> previously archived patches. Next commitfest starts in a few months
>>>> yet, and if you feel the need to submit corrected versions in the
>>>> meantime, please do submit in separate files. (Some would even argue
>>>> that each should be its own thread, but I don't think that's necessary.)
>>>
>>>
>>> Thank you for explanation.
>>>
>>> I'm adding new version of first patch with minor improvement:
>>> - I added detection of a case when all buckets are trivial
>>> (ie 0 or 1 element). In this case no need to sort buckets at all.
>>
>>
>> I'm putting rebased version of second patch.
>
>
> Again rebased version of both patches.
> Now second patch applies cleanly independent of first patch.

Patch 1 applies cleanly, builds, and make check runs fine.

The code looks similar in style to surrounding code too, so I'm not
going to complain about the abundance of underscores in the macros :-p

I can reproduce the results in the OP's benchmark, with slightly
different numbers, but an overall improvement of ~6%, which matches
the OP's relative improvement.

Algorithmically, everything looks sound.

A few minor comments about patch 1:

+ if (max == 1)
+ goto end;

That goto is unnecessary, you could just as simply say

if (max > 1)
{
...
}

+#define pg_shell_sort_pass(elem_t, cmp, off) \
+ do { \
+ int _i, _j; \
+ elem_t _temp; \
+ for (_i = off; _i < _n; _i += off) \
+ { \

_n right there isn't declared in the macro, and it isn't an argument
either. It should be an argument, having stuff inherited from the
enclosing context like that is confusing.

Same with _arr, btw.

Patch 2 LGTM.