Thread: [COMMITTERS] pgsql: Extend & revamp pg_bswap.h infrastructure.
Extend & revamp pg_bswap.h infrastructure.
Upcoming patches are going to address performance issues that involve
slow system provided ntohs/htons etc. To address that expand
pg_bswap.h to provide pg_ntoh{16,32,64}, pg_hton{16,32,64} and
optimize their respective implementations by using compiler intrinsics
for gcc compatible compilers and msvc. Fall back to manual
implementations using shifts etc otherwise.
Additionally remove multiple evaluation hazards from the existing
BSWAP32/64 macros, by replacing them with inline functions when
necessary. In the course of that the naming scheme is changed to
pg_bswap16/32/64.
Author: Andres Freund
Discussion: https://postgr.es/m/20170927172019.gheidqy6xvlxb325@alap3.anarazel.de
Branch
------
master
Details
-------
https://git.postgresql.org/pg/commitdiff/510b8cbff15fcece246f66f2273ccf830a6c7e98
Modified Files
--------------
config/c-compiler.m4 | 17 ++++++
configure | 24 ++++++++
configure.in | 1 +
contrib/btree_gist/btree_uuid.c | 4 +-
src/include/pg_config.h.in | 3 +
src/include/pg_config.h.win32 | 3 +
src/include/port/pg_bswap.h | 132 ++++++++++++++++++++++++++++++++--------
src/include/port/pg_crc32c.h | 2 +-
8 files changed, 159 insertions(+), 27 deletions(-)
--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers
On Fri, Sep 29, 2017 at 5:29 PM, Andres Freund <andres@anarazel.de> wrote:
> Extend & revamp pg_bswap.h infrastructure.
This looks wrong:
+static inline uint16
+pg_bswap64(uint16 x)
+{
+ return
+ ((x << 56) & UINT64CONST(0xff00000000000000)) |
+ ((x << 40) & UINT64CONST(0x00ff000000000000)) |
+ ((x << 24) & UINT64CONST(0x0000ff0000000000)) |
+ ((x << 8) & UINT64CONST(0x000000ff00000000)) |
+ ((x >> 8) & UINT64CONST(0x00000000ff000000)) |
+ ((x >> 24) & UINT64CONST(0x0000000000ff0000)) |
+ ((x >> 40) & UINT64CONST(0x000000000000ff00)) |
+ ((x >> 56) & UINT64CONST(0x00000000000000ff));
+}
--
Peter Geoghegan
--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers
On 2017-09-29 17:33:51 -0700, Peter Geoghegan wrote:
> On Fri, Sep 29, 2017 at 5:29 PM, Andres Freund <andres@anarazel.de> wrote:
> > Extend & revamp pg_bswap.h infrastructure.
>
> This looks wrong:
>
> +static inline uint16
> +pg_bswap64(uint16 x)
> +{
Yea, just noticed that :(. Running test, and pushing. Thanks for
checking!
Greetings,
Andres Freund
--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers
Andres Freund <andres@anarazel.de> writes:
> Extend & revamp pg_bswap.h infrastructure.
Hm, what is the point of this in pg_bswap.h:
+#ifdef _MSC_VER
+#include <stdlib.h>
+#endif
c.h will already have included <stdlib.h>. There might be some
value in this if we anticipated allowing freestanding use of this
header, but that won't happen because it depends on configure symbols.
regards, tom lane
--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers
On 2017-09-30 12:17:06 -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > Extend & revamp pg_bswap.h infrastructure. > > Hm, what is the point of this in pg_bswap.h: > > +#ifdef _MSC_VER > +#include <stdlib.h> > +#endif > > c.h will already have included <stdlib.h>. There might be some > value in this if we anticipated allowing freestanding use of this > header, but that won't happen because it depends on configure symbols. Well, it's not that obvious where the _byteswap_* functions are coming from on msvc. I guess we can just leave the comment /* In all supported versions msvc provides _byteswap_* functions in stdlib.h */ there, but I see no harm in the current form either. > but that won't happen because it depends on configure symbols. FWIW, I've wondered about replacing the pg_config.h tests with explicit gcc version checks. But doesn't seem worth it for now. Greetings, Andres Freund -- Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-committers
Andres Freund <andres@anarazel.de> writes:
> On 2017-09-30 12:17:06 -0400, Tom Lane wrote:
>> c.h will already have included <stdlib.h>. There might be some
>> value in this if we anticipated allowing freestanding use of this
>> header, but that won't happen because it depends on configure symbols.
> Well, it's not that obvious where the _byteswap_* functions are coming
> from on msvc. I guess we can just leave the comment
> /* In all supported versions msvc provides _byteswap_* functions in stdlib.h */
> there, but I see no harm in the current form either.
I'm OK with a comment like that, but I think we have a general style
guideline against headers duplicating stuff from c.h/postgres.h.
It just promotes confusion, IMO.
regards, tom lane
--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers