Thread: Signed vs Unsigned (take 2) (src/backend/storage/ipc/procarray.c)

Hi,

On 2021-06-12 10:55:22 -0300, Ranier Vilela wrote:
> With the recent changes at procarray.c, I take a look in.
> msvc compiler, has some warnings about signed vs unsigned.

> 1. Size_t is weird, because all types are int.

Not sure why I ended up using size_t here. There are cases where using a
natively sized integer can lead to better code being generated, so I'd
want to see some evaluation of the code generation effects.

> 2. Wouldn't it be better to initialize static variables?

No, explicit initialization needs additional space in the binary, and
static variables are always zero initialized.

> 3. There are some shadowing parameters.

Hm, yea, that's not great. Those are from

commit 0e141c0fbb211bdd23783afa731e3eef95c9ad7a
Author: Robert Haas <rhaas@postgresql.org>
Date:   2015-08-06 11:52:51 -0400

    Reduce ProcArrayLock contention by removing backends in batches.

Amit, Robert, I assume you don't mind changing this?

> 4. Possible loop beyond numProcs?

What are you referring to here?

Hi Andres, thanks for taking a look.

Em sáb., 12 de jun. de 2021 às 16:27, Andres Freund <andres@anarazel.de> escreveu:

Hi,

On 2021-06-12 10:55:22 -0300, Ranier Vilela wrote:
> With the recent changes at procarray.c, I take a look in.
> msvc compiler, has some warnings about signed vs unsigned.

> 1. Size_t is weird, because all types are int.

Not sure why I ended up using size_t here. There are cases where using a
natively sized integer can lead to better code being generated, so I'd
want to see some evaluation of the code generation effects.

 Yes, sure.

I took it a step further.

Transactions

HEAD                      patched
10002207                10586781
10146167                10388685
10048919                10333359

10065764,3333333 10436275                 3,55021946687555

TPS
HEAD                       patched
33469,016009         35399,010472
33950,624679         34733,252336
33639,8429             34578,495043
33686,4945293333 34903,5859503333 3,48700968070122

3,55% Is it worth touch procarray.c for real?

With msvc 64 bits, the asm generated:

HEAD

213.731 bytes procarray.asm

patched

212.035 bytes procarray.asm

Patch attached.

On Thu, 15 Jul 2021 at 23:38, Aleksander Alekseev
<aleksander@timescale.com> wrote:
> I'm updating the status to "Ready for Committer".

I think that might be a bit premature.  I can't quite see how changing
the pids List to a const List makes any sense, especially when the
code goes and calls lappend_int() on it to assign it some different
value.

There are also problems in BackendPidGetProcWithLock around consts.

Much of this patch kinda feels like another one of those "I've got a
fancy new static analyzer" patches.  Unfortunately, it just introduces
a bunch of compiler warnings as a result of the changes it makes.

I'd suggest splitting each portion of the patch out into parts related
to what it aims to achieve.  For example,  it looks like there's some
renaming going on to remove a local variable from shadowing a function
parameter.  Yet the patch is claiming performance improvements.  I
don't see how that part relates to performance. The changes to
ProcArrayClearTransaction() seem also unrelated to performance.

I'm not sure what the point of changing things like for (int i =0...
to move the variable declaration somewhere else is about.  That just
seems like needless stylistic changes that achieve nothing but more
headaches for committers doing backpatching work.

I'd say if this patch wants to be taken seriously it better decide
what it's purpose is, because to me it looks just like a jumble of
random changes that have no clear purpose.

I'm going to set this back to waiting on author.

Em qui., 15 de jul. de 2021 às 10:01, Aleksander Alekseev <aleksander@timescale.com> escreveu:

Thanks, David.

> I lost where. Can you show me?

See the attached warnings.txt.

Thank you.

 

> But the benchmark came from:
> pgbench -i -p 5432 -d postgres
> pgbench -c 50 -T 300 -S -n

I'm afraid this tells nothing unless you also provide the
configuration files and the hardware description, and also some
information on how you checked that there is no performance
degradation on all the other supported platforms and possible
configurations.

 

Benchmarking is a very complicated topic - trust me,
been there!

Absolutely.

 

It would be better to submit two separate patches, the one that
addresses Size_t and another that addresses shadowing. Refactorings
only, nothing else.

Regarding the code formatting, please see src/tools/pgindent.

I will try.

Hi Rainer,

> Here are the two patches.
> As suggested, reclassified as refactoring only.

Please don't change the status of the patch on CF application before
it was reviewed. It will only slow things down.

Your patch seems to have some problems on FreeBSD. Please see
http://commitfest.cputube.org/

Em sex., 16 de jul. de 2021 às 09:05, Aleksander Alekseev <aleksander@timescale.com> escreveu:

Hi Rainer,

> Here are the two patches.
> As suggested, reclassified as refactoring only.

Please don't change the status of the patch on CF application before
it was reviewed. It will only slow things down.

Hi Aleksander,

Sorry, lack of practice.

Your patch seems to have some problems on FreeBSD. Please see
http://commitfest.cputube.org/

I saw.

Very strange, in all other architectures, it went well.

I will have to install a FreeBSD to be able to debug.

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       tested, passed
Spec compliant:           tested, passed
Documentation:            tested, passed

The patch was tested on MacOS against master `80ba4bb3`.

The new status of this patch is: Ready for Committer

Hi hackers,

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       tested, passed
Spec compliant:           tested, passed
Documentation:            tested, passed

The patch was tested on MacOS against master `80ba4bb3`.

The new status of this patch is: Ready for Committer

The second patch seems fine too. I'm attaching both patches to trigger cfbot and to double-check them.

On 2021/09/11 12:21, Fujii Masao wrote:
>
>
> On 2021/07/23 20:07, Ranier Vilela wrote:
>> Em sex., 23 de jul. de 2021 às 07:02, Aleksander Alekseev <aleksander@timescale.com <mailto:aleksander@timescale.com>> escreveu:
>>
>>     Hi hackers,
>>
>>         The following review has been posted through the commitfest application:
>>         make installcheck-world:  tested, passed
>>         Implements feature:       tested, passed
>>         Spec compliant:           tested, passed
>>         Documentation:            tested, passed
>>
>>         The patch was tested on MacOS against master `80ba4bb3`.
>>
>>         The new status of this patch is: Ready for Committer
>>
>>
>>     The second patch seems fine too. I'm attaching both patches to trigger cfbot and to double-check them.
>>
>> Thanks Aleksander, for reviewing this.
>
> I looked at these patches because they are marked as ready for committer.
> They don't change any actual behavior, but look valid to me in term of coding.
> Barring any objection, I will commit them.

> No need to backpatch, why this patch is classified as
> refactoring only.

I found this in the commit log in the patch. I agree that these patches
are refactoring ones. But I'm thinking that it's worth doing back-patch,
to make future back-patching easy. Thought?

On 2021/09/15 21:27, Ranier Vilela wrote:
>     I found this in the commit log in the patch. I agree that these patches
>     are refactoring ones. But I'm thinking that it's worth doing back-patch,
>     to make future back-patching easy. Thought?
>
> Thanks for picking this.

Pushed. Thanks!