Re: [HACKERS] 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.falcon@postgrespro.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
-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers