Hello, Tom. I agree that lonely semicolon looks bad. Applied your suggestion for empty loop body (/* skip */).
Advertising
Patch in first letter had while(true), but I removed it cause I think it is uglier: - `while(true)` was necessary for grouping read with `if`, - but now there is single statement in a loop body and it is condition for loop exit, so it is clearly just a loop. Optimization is valid cause compare_exchange always store old value in `old` variable in a same atomic manner as atomic read. Tom Lane wrote 2017-05-25 17:39:
Sokolov Yura <funny.fal...@postgrespro.ru> writes: @@ -382,12 +358,8 @@ static inline uint64pg_atomic_fetch_and_u64_impl(volatile pg_atomic_uint64 *ptr, uint64 and_){ uint64 old; - while (true) - { - old = pg_atomic_read_u64_impl(ptr); - if (pg_atomic_compare_exchange_u64_impl(ptr, &old, old & and_)) - break; - } + old = pg_atomic_read_u64_impl(ptr); + while (!pg_atomic_compare_exchange_u64_impl(ptr, &old, old & and_)); return old; } #endif FWIW, I do not think that writing the loops like that is good style. It looks like a typo and will confuse readers. You could perhaps write the same code with better formatting, eg while (!pg_atomic_compare_exchange_u64_impl(ptr, &old, old & and_)) /* skip */ ; but why not leave the formulation with while(true) and a break alone? (I take no position on whether moving the read of "old" outside the loop is a valid optimization.) regards, tom lane
With regards, -- Sokolov Yura aka funny_falcon Postgres Professional: https://postgrespro.ru The Russian Postgres Company
From d377e36da652ade715b868a6fc7164736f38d1df Mon Sep 17 00:00:00 2001 From: Sokolov Yura <funny.fal...@postgrespro.ru> Date: Thu, 25 May 2017 14:54:45 +0300 Subject: [PATCH] Fix performance of Atomics generic implementation pg_atomic_compare_exchange_*_impl stores old value into `old` variable, so there is no need to reread value in loop body. Also add gcc specific __sync_fetch_and_(or|and), cause looks like compiler may optimize code around intrinsic better than around assembler, although intrinsic is compiled to almost same CAS loop. --- src/include/port/atomics/generic-gcc.h | 36 +++++++++++++++++ src/include/port/atomics/generic.h | 72 ++++++++++++---------------------- 2 files changed, 60 insertions(+), 48 deletions(-) diff --git a/src/include/port/atomics/generic-gcc.h b/src/include/port/atomics/generic-gcc.h index 7efc0861e7..79ada94047 100644 --- a/src/include/port/atomics/generic-gcc.h +++ b/src/include/port/atomics/generic-gcc.h @@ -185,6 +185,24 @@ pg_atomic_fetch_add_u32_impl(volatile pg_atomic_uint32 *ptr, int32 add_) } #endif +#if !defined(PG_HAVE_ATOMIC_FETCH_AND_U32) && defined(HAVE_GCC__SYNC_INT32_CAS) +#define PG_HAVE_ATOMIC_FETCH_AND_U32 +static inline uint32 +pg_atomic_fetch_and_u32_impl(volatile pg_atomic_uint32 *ptr, uint32 and_) +{ + return __sync_fetch_and_and(&ptr->value, and_); +} +#endif + +#if !defined(PG_HAVE_ATOMIC_FETCH_OR_U32) && defined(HAVE_GCC__SYNC_INT32_CAS) +#define PG_HAVE_ATOMIC_FETCH_OR_U32 +static inline uint32 +pg_atomic_fetch_or_u32_impl(volatile pg_atomic_uint32 *ptr, uint32 or_) +{ + return __sync_fetch_and_or(&ptr->value, or_); +} +#endif + #if !defined(PG_DISABLE_64_BIT_ATOMICS) @@ -223,6 +241,24 @@ pg_atomic_fetch_add_u64_impl(volatile pg_atomic_uint64 *ptr, int64 add_) } #endif +#if !defined(PG_HAVE_ATOMIC_FETCH_AND_U64) && defined(HAVE_GCC__SYNC_INT64_CAS) +#define PG_HAVE_ATOMIC_FETCH_AND_U64 +static inline uint64 +pg_atomic_fetch_and_u64_impl(volatile pg_atomic_uint64 *ptr, uint64 and_) +{ + return __sync_fetch_and_and(&ptr->value, and_); +} +#endif + +#if !defined(PG_HAVE_ATOMIC_FETCH_OR_U64) && defined(HAVE_GCC__SYNC_INT64_CAS) +#define PG_HAVE_ATOMIC_FETCH_OR_U64 +static inline uint64 +pg_atomic_fetch_or_u64_impl(volatile pg_atomic_uint64 *ptr, uint64 or_) +{ + return __sync_fetch_and_or(&ptr->value, or_); +} +#endif + #endif /* !defined(PG_DISABLE_64_BIT_ATOMICS) */ #endif /* defined(HAVE_ATOMICS) */ diff --git a/src/include/port/atomics/generic.h b/src/include/port/atomics/generic.h index 424543604a..75ffaf6e87 100644 --- a/src/include/port/atomics/generic.h +++ b/src/include/port/atomics/generic.h @@ -170,12 +170,9 @@ static inline uint32 pg_atomic_exchange_u32_impl(volatile pg_atomic_uint32 *ptr, uint32 xchg_) { uint32 old; - while (true) - { - old = pg_atomic_read_u32_impl(ptr); - if (pg_atomic_compare_exchange_u32_impl(ptr, &old, xchg_)) - break; - } + old = pg_atomic_read_u32_impl(ptr); + while (!pg_atomic_compare_exchange_u32_impl(ptr, &old, xchg_)) + /* skip */; return old; } #endif @@ -186,12 +183,9 @@ static inline uint32 pg_atomic_fetch_add_u32_impl(volatile pg_atomic_uint32 *ptr, int32 add_) { uint32 old; - while (true) - { - old = pg_atomic_read_u32_impl(ptr); - if (pg_atomic_compare_exchange_u32_impl(ptr, &old, old + add_)) - break; - } + old = pg_atomic_read_u32_impl(ptr); + while (!pg_atomic_compare_exchange_u32_impl(ptr, &old, old + add_)) + /* skip */; return old; } #endif @@ -211,12 +205,9 @@ static inline uint32 pg_atomic_fetch_and_u32_impl(volatile pg_atomic_uint32 *ptr, uint32 and_) { uint32 old; - while (true) - { - old = pg_atomic_read_u32_impl(ptr); - if (pg_atomic_compare_exchange_u32_impl(ptr, &old, old & and_)) - break; - } + old = pg_atomic_read_u32_impl(ptr); + while (!pg_atomic_compare_exchange_u32_impl(ptr, &old, old & and_)) + /* skip */; return old; } #endif @@ -227,12 +218,9 @@ static inline uint32 pg_atomic_fetch_or_u32_impl(volatile pg_atomic_uint32 *ptr, uint32 or_) { uint32 old; - while (true) - { - old = pg_atomic_read_u32_impl(ptr); - if (pg_atomic_compare_exchange_u32_impl(ptr, &old, old | or_)) - break; - } + old = pg_atomic_read_u32_impl(ptr); + while (!pg_atomic_compare_exchange_u32_impl(ptr, &old, old | or_)) + /* skip */; return old; } #endif @@ -261,12 +249,9 @@ static inline uint64 pg_atomic_exchange_u64_impl(volatile pg_atomic_uint64 *ptr, uint64 xchg_) { uint64 old; - while (true) - { - old = ptr->value; - if (pg_atomic_compare_exchange_u64_impl(ptr, &old, xchg_)) - break; - } + old = ptr->value; + while (!pg_atomic_compare_exchange_u64_impl(ptr, &old, xchg_)) + /* skip */; return old; } #endif @@ -357,12 +342,9 @@ static inline uint64 pg_atomic_fetch_add_u64_impl(volatile pg_atomic_uint64 *ptr, int64 add_) { uint64 old; - while (true) - { - old = pg_atomic_read_u64_impl(ptr); - if (pg_atomic_compare_exchange_u64_impl(ptr, &old, old + add_)) - break; - } + old = pg_atomic_read_u64_impl(ptr); + while (!pg_atomic_compare_exchange_u64_impl(ptr, &old, old + add_)) + /* skip */; return old; } #endif @@ -382,12 +364,9 @@ static inline uint64 pg_atomic_fetch_and_u64_impl(volatile pg_atomic_uint64 *ptr, uint64 and_) { uint64 old; - while (true) - { - old = pg_atomic_read_u64_impl(ptr); - if (pg_atomic_compare_exchange_u64_impl(ptr, &old, old & and_)) - break; - } + old = pg_atomic_read_u64_impl(ptr); + while (!pg_atomic_compare_exchange_u64_impl(ptr, &old, old & and_)) + /* skip */; return old; } #endif @@ -398,12 +377,9 @@ static inline uint64 pg_atomic_fetch_or_u64_impl(volatile pg_atomic_uint64 *ptr, uint64 or_) { uint64 old; - while (true) - { - old = pg_atomic_read_u64_impl(ptr); - if (pg_atomic_compare_exchange_u64_impl(ptr, &old, old | or_)) - break; - } + old = pg_atomic_read_u64_impl(ptr); + while (!pg_atomic_compare_exchange_u64_impl(ptr, &old, old | or_)) + /* skip */; return old; } #endif -- 2.11.0
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers