wvwwvwwv / scalable-concurrent-containers

High performance containers and utilities for concurrent and asynchronous programming
Apache License 2.0
306 stars 15 forks source link

AtomicArc::clone() is not consistent in behaviour #64

Closed rsdy closed 2 years ago

rsdy commented 2 years ago

If the AtomicArc is null, cloning it effectively makes a new, independent AtomicArc, which is counter-intuitive. However, if the AtomicArc instance is non-null, reference counting works as expected.

This is a tricky behaviour, because on one hand, I understand why this happens, on the other hand, now I need to use an Arc<AtomicArc<T>> or AtomicArc<Option<T>> for nullable struct members, eg. in a LinkedList implementation, neither of which seem very clean.

wvwwvwwv commented 2 years ago

Your understanding it correct, and that is actually my intent. Thanks for your opinion on the semantics. Honestly, I haven't found it problematic before - I just thought that AtomicArc was like a pointer with ownership (e.g., crossbeam_epoch::Atomic<T> in + std::sync::Arc), and therefore cloning an AtomicArc = cloning a crossbeam_epoch::Atomic<T> + reference counting, so I guess, it was more like Option<Arc<T>> rather than Arc<Option<T>>.

However, implementing something like AtomicArc<Option<T>> will be trickier than it looks, since,

  1. Constructing an instance of T while there are multiple pointers to it requires synchronization. Constructing should only be allowed to one thread. Something like AtomicOption has to be implemented.
  2. Dereferencing the instance requires additional memory barriers. Say, we have AtomicOption, constructing T has to release the instance - release-store memory barrier -, and reading the instance also has to acquire it - load-acquire memory barrier. Of course release-acquire is free on X86, but still, PPC is a major player in the enterprise hardware world, and ARM is also a rising star; optimizing memory operations is very, very important in enterprise software development.

So, my suggestion here is, you implement something like AtomicOption, and use AtomicArc<AtomicOption<T>>.

For this issue, I'll add more comments and documentation about the semantics.

rsdy commented 2 years ago

However, considering your points, I increasingly think that a AtomicOption is basically equivalent to AtomicArc. An AtomicArc<AtomicOption<T>> is simply initialized with AtomicArc::new(AtomicArc::null()), and you get the desired semantics.

Is there something I'm missing with this comparison?

wvwwvwwv commented 2 years ago

Let me explain AtomicArc a little bit further.

AtomicArc<T> points to an instance of T where the instance is garbage collected later after the AtomicArc<T> is dropped.

AtomicOption is different. It can be allocated in stack and the instance is dropped when the AtomicOption is destroyed.

Furthermore, AtomicArc<AtomicArc<T>> is different from AtomicArc<AtomicOption<T>>.

Semantically, yes, you're right, AtomicArc<AtomicOption<T>> and AtomicArc<AtomicArc<T>> are the same, but in practice, they are different in terms of performance as the former requires users to dereference two pointers to read the instance.

rsdy commented 2 years ago

Thanks for your thoughts.

It seems to me that specifically for use in a LinkedList, I'm better off using an Arc<AtomicArc<T>>, since this way everything can track the same AtomicArc instance, simplifying memory management, since the Node references can use the same ref counted pointer.

Overall, however, it comes out as Arc -> AtomicArc -> Arc -> T to reach the value.