RE: [PATCH][RFC] Remove volatile from data members in libstdc++

[Richard Guenther <rguenther at suse dot de>]

On Wed, 12 Jul 2006, Richard Guenther wrote:

> On Tue, 11 Jul 2006, Boehm, Hans wrote:
> 
> > Looking more carefully at the rope sources, I think the following are
> > true:
> > 
> > 1) Rope usually uses reference counts, as it always did.
> > 
> > 2) Once upon a time (around 2.95 and shortly thereafter), it did this
> > reasonably intelligently on a few platforms (Irix and win32, mostly) by
> > using atomic operations.  It used pthread locks on Linux.  The refcount
> > field was not volatile.  This was probably officially incorrect, since
> > it did some refcount == 1 tests outside a lock.  (The logic is such that
> > once you see refcount==1, it's guaranteed to stay that way.  Thus it
> > probably worked in all cases, but it clearly violated pthread rules.)
> > 
> > 3) It appears that at some later point the use of atomic operations was
> > dropped, and it now always uses locks?  This strikes me as a serious
> > mistake, since my recollection is that performance drops through the
> > floor when you do that.  It really needed to use atomic operations on
> > more platforms, not fewer.
> > 
> > 4) At some point atomicity.h was added.  It operates on volatiles.  It
> > has to, since it doesn't provide an atomic read operation, and I suspect
> > a bunch of clients read the values directly.  Thus if it didn't operate
> > on volatiles it might actually yield incorrect results on common
> > platforms; this way it's not in any sense standard conforming, but
> > probably works.
> 
> It doesn't operate on volatiles.  It simply has volatile qualified
> function parameters to avoid warnings if it happens to operate on 
> volatiles.  Also you don't need atomic read operations (there is no
> such thing really), instead you have to work out your reference counting
> scheme correctly, as you mentioned above.  Volatile doesn't help if
> that is wrong.

See also http://gcc.gnu.org/onlinedocs/gcc/Volatiles.html#Volatiles
which specifically has

"There is no guarantee that these reads and writes are atomic, especially 
for objects larger than int."

which is the reason int is used.  "especially for objects larger than int"
means that there is possibly a CPU instruction to read memory of size int
available and used (possibly, not guaranteed, but reasonable).

> > 5) At some point the rope reference count field became volatile.  This
> > is the right thing, since it should be using atomicity.h, and the type
> > of the reference count field should be changed suitably.  It's also
> > perhaps the right thing since the refcount==1 tests are still carried
> > out without a lock.  But it wasn't carried far enough.
> > 
> > 6) Eventually this should use something like atomic<intptr_t>, if/when
> > that makes it into the standard.  At that point, the volatiles should
> > probably disappear, and all accesses, including the ref_count == 1
> > tests, should use the proper accessors.  Currently I believe the proper
> > accessors don't exist, so volatile is the best we can do.
> > (I noticed that on my IA64/Debian box atomicity.h operates on ints,
> > which seems wrong for general reference counts without an overflow
> > check.  A pointer-sized int seems more appropriate.  Is there an
> > alternative I overlooked?)
> > 
> > Changing the rope reference counts to any form of atomics would be a
> > great improvement.
> 
> As below.  Testing in progress.

Needs the typo fix below.  I wonder if this is an ABI change?

Tested on x86_64-unknown-linux-gnu.

Ok for mainline?

Thanks,
Richard.

> --- 435,461 ----
>     struct _Refcount_Base
>     {
>       // The type _RC_t
> !     typedef _Atomic_word _RC_t;
>       
>       // The data member _M_ref_count
> !     _RC_t _M_ref_count;
>   
>       // Constructor
>   
> !     _Refcount_Base(_RC_t __n) : _M_ref_count(__n)
>       {
>       }
>   
>       void
>       _M_incr()
>       {
> !       __gnu_cxx::__atomic_add_dispatch(&this->_M_ref_count, 1);
>       }
>   
>       _RC_t
>       _M_decr()
>       {
> !       return __gnu_cxx::__exchange_and_add_dispatch(&this->_M_refcount, -1) - 1;

typo, &this->_M_ref_count

>       }
>     };
>   
> 

--
Richard Guenther <rguenther@suse.de>
Novell / SUSE Labs