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

[Richard Guenther <rguenther at suse dot de>]

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.

> 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.

Thanks,
Richard.


2006-07-12  Richard Guenther  <rguenther@suse.de>

	* include/ext/rope: Include <bits/atomicity.h>.
	(_Refcount_Base::_RC_t): Change type to _Atomic_word.
	(_Refcount_Base::_M_ref_count): Remove volatile qualifier.
	(_Refcount_Base::_M_ref_count_lock): Remove.
	(_Refcount_Base::_Refcount_Base): Remove initialization of
	_M_ref_count_lock.
	(_Refcount_Base::_M_incr): Use __atomic_add_dispatch to
	increment reference counter.
	(_Refcount_Base::_M_decr): Use __atomic_exchange_and_add_dispatch
	to decrement reference counter.

Index: include/ext/rope
===================================================================
*** include/ext/rope	(revision 115262)
--- include/ext/rope	(working copy)
***************
*** 55,60 ****
--- 55,61 ----
  #include <bits/stl_function.h>
  #include <bits/stl_numeric.h>
  #include <bits/allocator.h>
+ #include <bits/atomicity.h>
  #include <ext/hash_fun.h>
  
  # ifdef __GC
*************** _GLIBCXX_BEGIN_NAMESPACE(__gnu_cxx)
*** 434,474 ****
    struct _Refcount_Base
    {
      // The type _RC_t
!     typedef size_t _RC_t;
      
      // The data member _M_ref_count
!     volatile _RC_t _M_ref_count;
  
      // Constructor
-     __gthread_mutex_t _M_ref_count_lock;
  
!     _Refcount_Base(_RC_t __n) : _M_ref_count(__n), _M_ref_count_lock()
      {
- #ifdef __GTHREAD_MUTEX_INIT
-       __gthread_mutex_t __tmp = __GTHREAD_MUTEX_INIT;
-       _M_ref_count_lock = __tmp;
- #elif defined(__GTHREAD_MUTEX_INIT_FUNCTION)
-       __GTHREAD_MUTEX_INIT_FUNCTION (&_M_ref_count_lock);
- #else
- #error __GTHREAD_MUTEX_INIT or __GTHREAD_MUTEX_INIT_FUNCTION should be defined by gthr.h abstraction layer, report problem to libstdc++@gcc.gnu.org.
- #endif
      }
  
      void
      _M_incr()
      {
!       __gthread_mutex_lock(&_M_ref_count_lock);
!       ++_M_ref_count;
!       __gthread_mutex_unlock(&_M_ref_count_lock);
      }
  
      _RC_t
      _M_decr()
      {
!       __gthread_mutex_lock(&_M_ref_count_lock);
!       volatile _RC_t __tmp = --_M_ref_count;
!       __gthread_mutex_unlock(&_M_ref_count_lock);
!       return __tmp;
      }
    };
  
--- 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;
      }
    };