Re: better atomics - v0.5

On Wed, Jun 25, 2014 at 10:44 PM, Andres Freund <andres@2ndquadrant.com>
wrote:
>
> Attached is a new version of the atomic operations patch. Lots has
> changed since the last post:
>
> * gcc, msvc work. acc, xlc, sunpro have blindly written support which
>   should be relatively easy to fix up. All try to implement TAS, 32 bit
>   atomic add, 32 bit compare exchange; some do it for 64bit as well.

I have reviewed msvc part of patch and below are my findings:

1.
+pg_atomic_compare_exchange_u32_impl(volatile pg_atomic_uint32 *ptr,
+ uint32 *expected, uint32 newval)
+{
+ bool ret;
+ uint32 current;
+ current = InterlockedCompareExchange(&ptr->value, newval, *expected);

Is there a reason why using InterlockedCompareExchange() is better
than using InterlockedCompareExchangeAcquire() variant?
*Acquire or *Release variants can provide acquire memory ordering
semantics for processors which support the same else will be defined
as InterlockedCompareExchange.


2.
+pg_atomic_compare_exchange_u32_impl()
{
..
+ *expected = current;
}

Can we implement this API without changing expected value?
I mean if the InterlockedCompareExchange() is success, then it will
store the newval in ptr->value, else *ret* will be false.
I am not sure if there is anything implementation specific in changing
*expected*.

I think this value is required for lwlock patch, but I am wondering why
can't the same be achieved if we just return the *current* value and
then let lwlock patch do the handling based on it.  This will have another
advantage that our pg_* API will also have similar signature as native
API's.


3. Is there a overhead of using Add, instead of increment/decrement
version of Interlocked?

I could not find any evidence which can clearly indicate, if one is better
than other except some recommendation in below link which suggests to
*maintain reference count*, use Increment/Decrement

Refer *The Locking Cookbook* in below link:
http://blogs.technet.com/b/winserverperformance/archive/2008/05/21/designing-applications-for-high-performance-part-ii.aspx

However, when I tried to check the instructions each function generates,
then I don't find anything which suggests that *Add variant can be slow.

Refer Instructions for both functions:

cPublicProc _InterlockedIncrement,1
cPublicFpo 1,0
        mov     ecx,Addend              ; get pointer to addend variable
        mov     eax,1                   ; set increment value
Lock1:
   lock xadd    [ecx],eax               ; interlocked increment
        inc     eax                     ; adjust return value
        stdRET _InterlockedIncrement    ;

stdENDP _InterlockedIncrement




cPublicProc _InterlockedExchangeAdd, 2
cPublicFpo 2,0

        mov     ecx, [esp + 4]          ; get addend address
        mov     eax, [esp + 8]          ; get increment value
Lock4:
   lock xadd    [ecx], eax              ; exchange add

        stdRET  _InterlockedExchangeAdd

stdENDP _InterlockedExchangeAdd

For details, refer link:
http://read.pudn.com/downloads3/sourcecode/windows/248345/win2k/private/windows/base/client/i386/critsect.asm__.htm

I don't think there is any need of change in current implementation,
however I just wanted to share my analysis, so that if any one else
can see any point in preferring one or the other way of implementation.

4.
#pragma intrinsic(_ReadWriteBarrier)
#define pg_compiler_barrier_impl() _ReadWriteBarrier()

#ifndef pg_memory_barrier_impl
#define pg_memory_barrier_impl() MemoryBarrier()
#endif

There is a Caution notice in microsoft site indicating
_ReadWriteBarrier/MemoryBarrier are deprected.

Please refer below link:
http://msdn.microsoft.com/en-us/library/f20w0x5e(v=vs.110).aspx

When I checked previous versions of MSVC, I noticed that these are
supported on x86, IPF, x64 till VS2008 and supported only on x86, x64
for VS2012 onwards.

Link for VS2010 support:
http://msdn.microsoft.com/en-us/library/f20w0x5e(v=vs.100).aspx

5.
+ * atomics-generic-msvc.h
..
+ * Portions Copyright (c) 2013, PostgreSQL Global Development Group

Shouldn't copyright notice be 2014?

6.
pg_atomic_compare_exchange_u32()

It is better to have comments above this and all other related functions.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com