Re: Switch to std::atomic?

[Rik]

On 09/26/2019 12:01 PM, John W. Eaton 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.
>
I like it.  This seems much cleaner.  The benchmark for bm.toeplitz.orig.m
shifted from 7.5 to 7.6 seconds.  That is within the noise for this
measurement so no problems there.  There doesn't seem to be any ill
effects.  Running the GUI on an installed version and using
__run_test_suite__ produced no errors nor any segfaults.

It's done, so maybe it doesn't matter, but I very much doubt that objects
are shared so extensively that they require octave_idx_type-sized
refcounts.  On my machine, 'int' is 4 bytes while octave_idx_type is 8
bytes.  It's not the extra 4B of storage that are important, but maybe the
fact that 'int' is supposed to always be the most natural unit for the
machine in question.  Maybe there are slight performance benefits to using
int for reference counts?

Was there a particular reason to make oct-atomic.c a 'C' language file
rather than a C++ file?  If possible, it would seem better to make this C++
and possible also add an 'inline' declaration so that we don't get function
call overhead.

--Rik