Re: Switch to std::atomic?

[Dmitri A. Sergatskov]

On Thu, Sep 26, 2019 at 2:29 PM John W. Eaton <address@hidden> wrote:
>
> On 9/25/19 6:10 PM, John W. Eaton wrote:
> > On 9/25/19 10:34 AM, John W. Eaton wrote:
> >> On 9/13/19 5:29 PM, Rik wrote:
> >>
> >>> Would it make sense to switch to using std::atomic<octave_idx_type>?  In
> >>> that case, Octave would be portable to any machine with a valid C++
> >>> runtime
> >>> library, and we certainly require that.  The std::atomic class overloads
> >>> the (pre-, post-)(increment, decrement) operators so one could write
> >>> more
> >>> readable code with count++ rather than
> >>> OCTAVE_ATOMIC_POST_INCREMENT(count).
> >>
> >> Yes, now that it is a standard C++ feature, we should be using that.
> >> For a simple transition, can we first define our refcount class using
> >> std::atomic<T>?
> >>
> >> Would you like to make this change?  If not, I can look at it.
> >
> > I was curious to see what it would take and came up with the following
> > change.  I think this is OK as far as it goes, but I haven't pushed it.
> >
> > Notes and questions:
> >
> >    * The C11 atomic_fetch_add and atomic_fetch_sub functions were
> > required to handle the tricky way that the dim_vector class stores the
> > reference count.  I believe that to do this job in C++ requires the
> > atomic_ref object that is a new feature in C++20.  Either that or use a
> > separate reference count variable instead of storing it in the array of
> > values, but then as the comment in dim-vector.h says, we will need two
> > allocations, one for the rep object and one for the array that it will
> > then contain.
> >
> >    * Some reference counts use int and some use octave_idx_type.  Maybe
> > we should always use the same type?  Should it be int or octave_idx_type
> > or size_t?
> >
> >    * The copy constructor for the cdef_class_rep object looks wrong to
> > me.  Instead of copying the reference count, shouldn't the count for the
> > new object be 1?  But I'm not sure I understand the way this code is
> > supposed to work so I left it alone and provided a copy constructor for
> > the refcount class.
>
> I ended up pushing the following three changes:
>
>    http://hg.savannah.gnu.org/hgweb/octave/rev/c98953e85220
>    http://hg.savannah.gnu.org/hgweb/octave/rev/c23aee2104de
>    http://hg.savannah.gnu.org/hgweb/octave/rev/396996f1dad0
>
> Now all reference counts use octave_idx_type and I was able to eliminate
> the object_count member variable in the cdef_class_rep object.  It was
> not a reference count and was set but not used otherwise.
>
> Until we have the C++20 atomic_ref class (or some interim replacement
> for it), then I think the best we can do is to use the C11
> atomic_fecth_add and atomic_fetch_sub functions in the dim_vector class.
>
> jwe
>
>

There are some errors on clang compiles, e.g.:

http://buildbot.octave.org:8010/#/builders/9/builds/1121



../src/liboctave/util/oct-atomic.c:34:3: error: address argument to
atomic operation must be a pointer to _Atomic type ('octave_idx_type
*' (aka 'long *') invalid)
  atomic_fetch_add (x, 1);
  ^                 ~
/usr/lib64/clang/8.0.0/include/stdatomic.h:146:43: note: expanded from
macro 'atomic_fetch_add'
#define atomic_fetch_add(object, operand)
__c11_atomic_fetch_add(object, operand, __ATOMIC_SEQ_CST)
                                          ^                      ~~~~~~
../src/liboctave/util/oct-atomic.c:42:3: error: address argument to
atomic operation must be a pointer to _Atomic type ('octave_idx_type
*' (aka 'long *') invalid)
  atomic_fetch_sub (x, 1);
  ^                 ~
/usr/lib64/clang/8.0.0/include/stdatomic.h:149:43: note: expanded from
macro 'atomic_fetch_sub'
#define atomic_fetch_sub(object, operand)
__c11_atomic_fetch_sub(object, operand, __ATOMIC_SEQ_CST)
                                          ^                      ~~~~~~
2 errors generated.
make[2]: *** [Makefile:16937: liboctave/util/libutil_la-oct-atomic.lo] Error 1


Dmitri.
--