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?

Greetings,

Andres Freund

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.

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.

Yes, I missed this part.
But I was worried about this line:

/* hasn't been updated yet */
if (!TransactionIdIsValid(ComputeXidHorizonsResultLastXmin))

The first run with ComputeXidHorizonsResultLastXmin = 0, is ok?

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?

My mistake.

best regards,
Ranier Vilela

Em dom., 13 de jun. de 2021 às 09:43, Ranier Vilela <ranier.vf@gmail.com>
escreveu:

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'm a little confused by the msvc compiler, but here's the difference in
code generation.
Apart from the noise caused by unnecessary changes regarding the names.

Microsoft (R) C/C++ Optimizing Compiler Versão 19.28.29915 para x64

diff attached.

regards,
Ranier Vilela

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.

regards,
Ranier Vilela

Em seg., 14 de jun. de 2021 às 21:01, Ranier Vilela <ranier.vf@gmail.com>
escreveu:

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.

Added to next CF (https://commitfest.postgresql.org/33/3169/)

regards,
Ranier Vilela

Hi hackers,

Patch attached.

Added to next CF (https://commitfest.postgresql.org/33/3169/)

The proposed code casts `const` variables to non-`const`. I'm surprised
MSVC misses it. Also, there were some issues with the code formatting. The
corrected patch is attached.

The patch is listed under the "Performance" topic on CF. However, I can't
verify any changes in the performance because there were no benchmarks
attached that I could reproduce. By looking at the code and the first
message in the thread, I assume this is in fact a refactoring.

Personally I don't believe that changes like:

-               for (int i = 0; i < nxids; i++)
+               int     i;
+               for (i = 0; i < nxids; i++)

.. or:

-       for (int index = myoff; index < arrayP->numProcs; index++)
+       numProcs = arrayP->numProcs;
+       for (index = myoff; index < numProcs; index++)

... are of any value, but other changes may be. I choose to keep the patch
as-is except for the named defects and let the committer decide which
changes, if any, are worth committing.

I'm updating the status to "Ready for Committer".

--
Best regards,
Aleksander Alekseev

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

Hi hackers,

Patch attached.

Added to next CF (https://commitfest.postgresql.org/33/3169/)

Hi Aleksander, thanks for taking a look at this.

The proposed code casts `const` variables to non-`const`. I'm surprised
MSVC misses it.

I lost where. Can you show me?

Also, there were some issues with the code formatting. The corrected patch
is attached.

Sorry, thanks for correcting.

The patch is listed under the "Performance" topic on CF. However, I can't
verify any changes in the performance because there were no benchmarks
attached that I could reproduce. By looking at the code and the first
message in the thread, I assume this is in fact a refactoring.

My mistake, a serious fault.
But the benchmark came from:
pgbench -i -p 5432 -d postgres
pgbench -c 50 -T 300 -S -n

Personally I don't believe that changes like:

-               for (int i = 0; i < nxids; i++)
+               int     i;
+               for (i = 0; i < nxids; i++)

Yeah, it seems to me that this style will be consolidated in Postgres 'for
(int i = 0;'.

.. or:

-       for (int index = myoff; index < arrayP->numProcs; index++)
+       numProcs = arrayP->numProcs;
+       for (index = myoff; index < numProcs; index++)

The rationale here is to cache arrayP->numProcs to local variable, which
improves performance.

... are of any value, but other changes may be. I choose to keep the patch
as-is except for the named defects and let the committer decide which
changes, if any, are worth committing.

I'm updating the status to "Ready for Committer".

Thank you.

regards,
Ranier Vilela

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.

David

Em qui., 15 de jul. de 2021 às 09:45, David Rowley <dgrowleyml@gmail.com>
escreveu:

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.

I understood.
I will try to address all concerns in the new version.

regards,
Ranier Vilela

Thanks, David.

I lost where. Can you show me?

See the attached warnings.txt.

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!

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.

--
Best regards,
Aleksander Alekseev

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.

regards,
Ranier Vilela

Em qui., 15 de jul. de 2021 às 10:04, Ranier Vilela <ranier.vf@gmail.com>
escreveu:

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.

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

regards,
Ranier Vilela

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/

--
Best regards,
Aleksander Alekseev

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.

Thanks for your review.

best regards,
Ranier Vilela

Em sex., 16 de jul. de 2021 às 09:41, Ranier Vilela <ranier.vf@gmail.com>
escreveu:

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.

There are a typo in
0001-Promove-unshadowing-of-two-variables-PGPROC-type.patch

- ProcArrayEndTransactionInternal(proc, proc->procArrayGroupMemberXid);
+ ProcArrayEndTransactionInternal(nextproc,
nextproc->procArrayGroupMemberXid);

Attached new version v1, with fix.
Now pass check-world at FreeBSD 13 with clang 11.

regards,
Ranier Vilela

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.

--
Best regards,
Aleksander Alekseev

Em sex., 23 de jul. de 2021 às 07:02, Aleksander Alekseev <
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.

regards,
Ranier Vilela

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.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION