Re: Making empty Bitmapsets always be NULL

Em qui., 22 de jun. de 2023 às 01:43, David Rowley <dgrowleyml(at)gmail(dot)com>
escreveu:

> On Thu, 22 Jun 2023 at 00:16, Ranier Vilela <ranier(dot)vf(at)gmail(dot)com> wrote:
> > 2. Only compute BITNUM when necessary.
>
> I doubt this will help. The % 64 done by BITNUM will be transformed
> to an AND operation by the compiler which is likely going to be single
> instruction latency on most CPUs which probably amounts to it being
> "free". There's maybe a bit of reading for you in [1] and [2] if
> you're wondering how any operation could be free.
>
I think the word free is not the right one.
The end result of the code is the same, so whatever you write it one way or
the other,
the compiler will transform it as if it were written without calculating
BITNUM in advance.

See at:
https://godbolt.org/z/39MdcP7M3

The issue is the code becomes clearer and more readable with the
calculation in advance.
In that case, I think so.
But this is on a case-by-case basis, in other contexts it can be more
expensive.

>
> (The compiler is able to transform the % into what is effectively &
> because 64 is a power of 2. uintvar % 64 is the same as uintvar & 63.
> Play around with [3] to see what I mean)
>
> > 3. Avoid enlargement when nwords is equal wordnum.
> > Can save cycles when in corner cases?
>
> No, you're just introducing a bug here. Arrays in C are zero-based,
> so "wordnum >= a->nwords" is exactly the correct way to check if
> wordnum falls outside the bounds of the existing allocated memory. By
> changing that to "wordnum > a->nwords" we'll fail to enlarge the words
> array when it needs to be enlarged by 1 element.
>
Yeah, this is my fault.
Unfortunately, I missed the failure of the regression tests.

> It looks like you've introduced a bunch of random white space and
> changed around a load of other random things in the patch too. I'm not
> sure why you think that's a good idea.
>
Weel, It is much easier to read and follows the general style of the other
fonts.

> FWIW, we normally only write "if (somevar)" as a shortcut when somevar
> is boolean and we want to know that it's true. The word value is not
> a boolean type, so although "if (someint)" and "if (someint != 0)"
> will compile to the same machine code, we don't normally write our C
> code that way in PostgreSQL. We also tend to write "if (someptr !=
> NULL)" rather than "if (someptr)". The compiler will produce the same
> code for each, but we write the former to assist people reading the
> code so they know we're checking for NULL rather than checking if some
> boolean variable is true.
>
No, this is not the case.
With unsigned words, it can be a more appropriate test without == 0.

See:
https://stackoverflow.com/questions/14267081/difference-between-je-jne-and-jz-jnz

In some contexts, it can be faster when it has CMP instruction before.

> Overall, I'm not really interested in sneaking any additional changes
> that are unrelated to adjusting Bitmapsets so that don't carry
> trailing zero words. If have other optimisations you think are
> worthwhile, please include them in another thread along with
> benchmarks to show the performance increase. For learning, I'd
> encourage you to do some micro benchmarks outside of PostgreSQL and
> mock up some Bitmapset code in a single .c file and try out with any
> without your changes after calling the function in a tight loop to see
> if you can measure any performance gains. Just remember you'll never
> see any gains in performance when your change compiles into the exact
> same code as without your change. Between [1] and [2], you still
> might not see performance changes even when the compiled code is
> changed (I'm thinking of your #2 change here).
>
Well, *const* always is a good style and can prevent mistakes and
allows the compiler to do optimizations.

regards,
Ranier Vilela