Re: Fix performance of generic atomics

Sokolov Yura <funny.falcon@postgrespro.ru>

From: Sokolov Yura <funny.falcon@postgrespro.ru>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-hackers@postgresql.org
Date: 2017-05-25T15:12:44Z
Lists: pgsql-hackers

Attachments

Hello, Tom.

I agree that lonely semicolon looks bad.
Applied your suggestion for empty loop body (/* skip */).

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.falcon@postgrespro.ru> writes:
> @@ -382,12 +358,8 @@ 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_));
>  	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

Commits

  1. Further marginal hacking on generic atomic ops.

  2. Use more of gcc's __sync_fetch_and_xxx builtin functions for atomic ops.

  3. Remove duplicate reads from the inner loops in generic atomic ops.