Re: Switch to std::atomic?

[John W. Eaton]

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