Documentation of `Arc::from_raw` is unnecessarily restrictive

[terrarier2111]

Location

Arc::from_raw

Summary

Arc::from_raw mentions that the provided pointer has to have been returned from Arc::into_raw but it doesn't mention Arc::as_ptr at all. This disallows many usages that depend on not modifying the reference counter while passing Arc around in form of a pointer and converting it back by calling Arc::from_raw on the pointer and wrapping the result inside a ManuallyDrop (as after one conversion round of Arc::into_raw -> ManuallyDrop<Arc::from_raw> -> Arc::as_ptr would disallow recreating the Arc through Arc::from_raw)

[saethlin]

Isn't this code pattern you want documented as valid unnecessarily dangerous, like Vec::from_raw_parts with a pointer from Vec::as_ptr?

Why do you want to pass around an Arc created like this instead of a reference or a raw pointer?

[albertlarsan68]

@rustbot claim

[lukaslueg]

Beware of the footgun: To call Arc::into_raw, you consume one reference count, as into_raw consumes the Arc; this is the symmetry with from_raw. On the other hand, as_raw takes an &Arc, so its possible to call this many times wrt the same reference count; if you then call from_raw on those pointers, the reference count will underflow when those newly created Arcs get dropped.
Obviously we are in unsafe-land anyway, but the docs should point this out explicitly, as as_raw+from_raw is much easier to get wrong than into_raw/from_raw

[terrarier2111]

Isn't this code pattern you want documented as valid unnecessarily dangerous, like Vec::from_raw_parts with a pointer from Vec::as_ptr?

Why do you want to pass around an Arc created like this instead of a reference or a raw pointer?

I was able to find a different solution using into_raw and from_raw, but i still think mentioning the possibility of using Arc::as_raw should be mentioned.

[terrarier2111]

Beware of the footgun: To call Arc::into_raw, you consume one reference count, as into_raw consumes the Arc; this is the symmetry with from_raw. On the other hand, as_raw takes an &Arc, so its possible to call this many times wrt the same reference count; if you then call from_raw on those pointers, the reference count will underflow when those newly created Arcs get dropped. Obviously we are in unsafe-land anyway, but the docs should point this out explicitly, as as_raw+from_raw is much easier to get wrong than into_raw/from_raw

Yea, you are absolutely right with that but by passing around ManuallyDrop<Arc> this can be largely avoided (except if one manually drops the Arc)

[pitaj]

Why would you need to do ManuallyDrop::new(from_raw(as_ptr)) instead of frow_raw(into_raw(clone))?

I can't imagine avoiding a single atomic increment brings measurable benefit.

[terrarier2111]

Why would you need to do ManuallyDrop::new(from_raw(as_ptr)) instead of frow_raw(into_raw(clone))?

I can't imagine avoiding a single atomic increment brings measurable benefit.

If you want to you can perform measurements, this is the repo: https://github.com/terrarier2111/SwapArc
(it's essentially a data structure that presents a faster alternative to ArcSwap)
but yea generally speaking on the fast path i only have a single Relaxed load and even on the slow paths i try to use as few cmp_exchg and fetch_xxx operations as possible because these are pretty much the most expensive atomic operations i am using, even without contention (at least on x86_64), also every atomic modification of the counter invalidates its cache line which can also be very punishing. But i am not alone with this concern, the author of ArcSwap also noticed the impact of Arc cloning on performance in his implementation.

[pitaj]

Could you use something like Arc::as_weak instead? #100472

[terrarier2111]

I can't because that would mean returning a type that's not just T (in Arc<T>) which is a requirement for my API (as it is generic and allows to use more than just Arc)

[pitaj]

Couldn't you put an associated type on DataPtrConvert? For instance:

impl<T: Send + Sync> DataPtrConvert<T> for Arc<T> {
    type Weak = sync::sync::Weak<T>;

    fn as_weak(&self) -> Self::Weak { todo!() }
}