Re: Small improvement to compactify_tuples

Hello, Claudio.

Thank you for review and confirm of improvement.

On 2017-09-23 01:12, Claudio Freire wrote:
> 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)
> {
> ...
> }

Done.
(I don't like indentation, though :-( )

>
>
> +#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.

pg_shell_sort_pass is not intended to be used outside pg_shell_sort
and ph_insertion_sort, so I think, stealing from their context is ok.
Nonetheless, done.

>
>
> Patch 2 LGTM.

--
Sokolov Yura aka funny_falcon
Postgres Professional: https://postgrespro.ru
The Russian Postgres Company