[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Switch to std::atomic?
From: |
Dmitri A. Sergatskov |
Subject: |
Re: Switch to std::atomic? |
Date: |
Thu, 26 Sep 2019 15:25:08 -0500 |
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.
--
- Switch to std::atomic?, Rik, 2019/09/13
- Re: Switch to std::atomic?, John W. Eaton, 2019/09/25
- Re: Switch to std::atomic?, John W. Eaton, 2019/09/25
- Is fork() broken in octave 5.1 ?, Kay Nick, 2019/09/26
- Re: Is fork() broken in octave 5.1 ?, Mike Miller, 2019/09/26
- Re: Is fork() broken in octave 5.1 ?, Kay Nick, 2019/09/26
- Re: Is fork() broken in octave 5.1 ?, Mike Miller, 2019/09/26
- Re: Is fork() broken in octave 5.1 ?, John W. Eaton, 2019/09/26
- Re: Is fork() broken in octave 5.1 ?, Dr. K. nick, 2019/09/27
- Re: Switch to std::atomic?, John W. Eaton, 2019/09/26
- Re: Switch to std::atomic?,
Dmitri A. Sergatskov <=
- Re: Switch to std::atomic?, John W. Eaton, 2019/09/27
- Re: Switch to std::atomic?, Rik, 2019/09/26
- Re: Switch to std::atomic?, John W. Eaton, 2019/09/26
- Re: Switch to std::atomic?, Rik, 2019/09/26
- Re: Switch to std::atomic?, John W. Eaton, 2019/09/27
- Re: Switch to std::atomic?, Pantxo, 2019/09/27
- Re: Switch to std::atomic?, John W. Eaton, 2019/09/27
- Re: Switch to std::atomic?, Pantxo Diribarne, 2019/09/28