Make Queue sendable

[pronebird]

Having Queue in async contexts is currently problematic. I think it's fair to say that Queue can be moved between threads and we should should mark it Send to make it easier to use it.

[madsmtm]

Indeed, it is defined as DISPATCH_DECL(dispatch_queue), and DISPATCH_DECL is defined here as OS_OBJECT_DECL_SUBCLASS_SWIFT(name, dispatch_object), i.e. clearly sendable.

Could you mark it as Sync too, as well as mark the other things that originate in DISPATCH_DECL?

[pronebird]

Sure thing. Since many types here hold DispatchObject, I realized that implementing Send + Sync for DispatchObject should then automatically apply to all types holding it for as long as those types only hold Send + Sync values.

Note that since DispatchObject holds a bool flag along with dispatch object, I had to wrap it into Arc<AtomicBool>.

[madsmtm]

Ah, but DispatchObject itself is not Send + Sync, at least not from Swift's perspective. Try e.g. compiling the following:

// swift -swift-version 6 foo.swift
import Dispatch
import AppKit

let queue = DispatchQueue(label: "foo")
let queue2: Sendable = queue // DispatchQueue is sendable

let obj: DispatchObject = queue
let obj2: Sendable = obj // But DispatchObject is not

[pronebird]

You’re right my bad. It’s a bit tricky though that all of the dispatch types in dispatch2 resort to wrapping their raw pointer in DispatchObject and holding it inside. DispatchObject itself then has state too so it has to be made safe too.

This reminds me of objc2::Retained which I use instead in my own bindings.

To be frank I thought this was simpler but a proper solution probably requires a rethink of this system. I lean towards obj2::Retained instead of having DispatchObject which is probably better to be a trait since we don’t have inheritance in Rust and composition just doesn’t map properly.

Does any of this make sense to you?

[madsmtm]

I am thinking of using something akin to objc2::rc::Retained<DispatchQueue>, especially so since it'd allow e.g. &DispatchQueue to be safe to pass across an ABI boundary. But there are complications with that, see #77 (comment), so it might take a little while.

[pronebird]

This turned a bit into experiment to refactor all that + use objc2::rc::Retained but then I see that the crate can somehow can compile without objc2 so I am a bit at loss to why objc2 is optional. I can certainly resort to dispatch_retain and dispatch_release but that sounds like reinventing Retained.

Also defining objects as ProtocolObject<dyn NSObjectProtocol> is not ideal because Clone cannot be automatically implemented, but Retained can be cloned. Probably can be solved at create_opaque_type level.

[madsmtm]

This turned a bit into experiment to refactor all that + use objc2::rc::Retained but then I see that the crate can somehow can compile without objc2 so I am a bit at loss to why objc2 is optional. I can certainly resort to dispatch_retain and dispatch_release but that sounds like reinventing Retained.

Yeah, I've been pondering this too, but I think the "proper" dependency chain has to be that Objective-C support in dispatch2 has to be optional (similar problems arise for objc2-core-foundation-like crates), for the simple reason that linking in libobjc takes precious startup time that you don't want to spend unless you really need to (I.e. no longer zero cost) (I'll have to test it though to make sure), but also because dispatch_retain is likely to be faster than objc_retain on libdispatch objects (also a claim that needs to be tested).

And yeah, that does require dispatch2 (and objc2-core-foundation) to define another smart pointer that will end up very similar to objc2::rc::Retained (and then have From/Into conversions to objc2::rc::Retained).

Looking at Clang's source code, this is similar to what they do, they have a distinction between three different types of retained pointers:

• OS / #import <os/os.h>-like types (libdispatch falls under here)
• CF / CoreFoundation-like types (incl. CoreText, IOSurface, AVFoundation, etc.)
• NS / Objective-C-like types (incl. Foundation, AppKit, etc.)

I'll try to see if I can get the type to implement it myself during the holidays, but... There's a few things that I'm hoping to get done there too, so might not make it.

[madsmtm]

I seem to remember you leaving a comment here about investigating in Hopper that libdispatch calls directly into objc, but I can't seem to find it now?

In any case, I don't think that's true, I've investigated the performance a bit now, my very non-scientific results:

Function	lower 95%	mean	upper 95%
dispatch_retain / dispatch_release	3.7996 ns	3.8016 ns	3.8041 ns
objc_retain / objc_release	6.2981 ns	6.3024 ns	6.3075 ns
objc_retain / objc_release chained	6.2987 ns	6.3030 ns	6.3079 ns

Benchmarking code

use criterion::{criterion_group, criterion_main, Criterion};

pub fn criterion_benchmark(c: &mut Criterion) {
    let obj = dispatch2::Queue::new("abc", dispatch2::QueueAttribute::Concurrent);

    let mut group = c.benchmark_group("retain/release");
    group.bench_function("dispatch", |b| {
        b.iter(|| {
            unsafe { dispatch2::ffi::dispatch_retain(obj.as_raw().cast()) };
            unsafe { dispatch2::ffi::dispatch_release(obj.as_raw().cast()) };
        })
    });
    group.bench_function("objc", |b| {
        b.iter(|| {
            unsafe { objc2::ffi::objc_retain(obj.as_raw().cast()) };
            unsafe { objc2::ffi::objc_release(obj.as_raw().cast()) };
        })
    });
    group.bench_function("objc chained", |b| {
        b.iter(|| {
            unsafe { objc2::ffi::objc_release(objc2::ffi::objc_retain(obj.as_raw().cast())) };
        })
    });
    group.finish();
}

criterion_group!(benches, criterion_benchmark);
criterion_main!(benches);

I.e. dispatch_retain/dispatch_release is two times faster than the equivalent Objective-C functions (on a libdispatch type).

This matches my intuition from reading the source code too, which basically boils down to:

• If not internally a Dispatch-compatible object.
  • (slow path) Invoke [obj retain] to support cost-free bridging to NSData.
• Otherwise (fast path) invoke _os_object_retain inline.

My claim about startup time is false though, libdispatch (linked via libSystem) itself calls _objc_init from within _os_object_init, so linking in libobjc doesn't change anything.

[pronebird]

Nice performance testing!

I seem to remember you leaving a comment here about investigating in Hopper that libdispatch calls directly into objc, but I can't seem to find it now?

My memory failed me. I had a fresh look at it this morning and the call to [<object> release] was indeed conditional. I assume the perf boost comes from avoiding the dynamic dispatch and calling atomic increment/decrement directly.

So that means that a custom Retained needs to be built.

and then have From/Into conversions to objc2::rc::Retained

So I assume such conversion should maintain the performance guarantee and instead of adopting the object pointer, it should actually wrap the more performant implementation of Retained using dispatch_retain() and dispatch_release() and delegate to it directly right?

Also similar memory management exists in network framework (nw_retain and nw_release) which internally call into os_retain and os_release.

Could be handy for objc2::rc::Retained to have a built-in delegation.

[madsmtm]

Nice performance testing!

Thanks! Not something I'm too familiar with, so nice to get some validation ;). Also, just for completeness I'll note that these amounts are really small in any case, and in real-world use-cases likely negligible (Swift doesn't care about the distinction, for example).

Also similar memory management exists in network framework (nw_retain and nw_release) which internally call into os_retain and os_release.

Could be handy for objc2::rc::Retained to have a built-in delegation.

This design has been explored a bit in fruity, it has fruity::core::Arc, which internally calls either CFRetain, dispatch_retain or objc_retain, depending on the type of object. I assume that's what you mean by built-in delegation?

I find it very nice from a technical perspective, but then again, I think it leads to a design that has more indirection, and is more difficult for users to understand:

• Having objc2::rc::Retained call objc_retain directly is very easy to explain in documentation, the delegation pattern is more difficult.
• There's the difficulty of where to put such a shared Retained wrapper? In a way, it is wrong that it lives in objc2, as a user of objc2-core-foundation doesn't necessarily want that as a dependency. Do we then create a new crate retain_utils?
• Retained supports more operations in Objective-C than elsewhere (Retained::retain_autoreleased, Retained::autorelease, conversion to/from rc::Weak etc.). How would you model that (simply) with a delegation pattern?

I think abstraction is a good idea when there really is a general pattern to abstract over. But in this case there is a fairly limited set of different retain/release functions (I believe the full list that we'll ever care about is libdispatch, the CoreFoundation framework, libobjc, and maybe the Network, Security and IOKit frameworks), and so I believe "specialization" (as an antonym to abstraction) is the better approach for us here.

So I assume such conversion should maintain the performance guarantee and instead of adopting the object pointer, it should actually wrap the more performant implementation of Retained using dispatch_retain() and dispatch_release() and delegate to it directly right?

I'd rather make the default case fast, i.e. use and return dispatch2::Retained (or whatever it ends up being called, naming suggestions welcome), and then allow the user to explicitly convert to objc2::rc::Retained if they really need Objective-C compatibility (which should be fairly rare). That is, it's more important to me to create a valley of success than always using the strictly most performant solution.

(Also, let's say that you take objc2::rc::Retained<NSData>, and convert it into the hypothetical objc2::rc::Retained<dispatch2::Data>; in that case, even though the type is dispatch2::Data, then user would still want objc_retain, but we'd have no way of detecting that).

---

Thanks for the input here so far btw, it forces me to write down my reasoning for some decisions that I've made implicitly in my head.

[pronebird]

Thanks! Not something I'm too familiar with, so nice to get some validation ;). Also, just for completeness I'll note that these amounts are really small in any case, and in real-world use-cases likely negligible (Swift doesn't care about the distinction, for example).

That's why I stuck with objc2::rc::Retained in the first place. I however now think that we can support alternative smart pointer to improve performance. It's not that much of a trouble.

This design has been explored a bit in fruity, it has fruity::core::Arc, which internally calls either CFRetain, dispatch_retain or objc_retain, depending on the type of object. I assume that's what you mean by built-in delegation?

Exactly.

• Having objc2::rc::Retained call objc_retain directly is very easy to explain in documentation, the delegation pattern is more difficult.

• There's the difficulty of where to put such a shared Retained wrapper? In a way, it is wrong that it lives in objc2, as a user of objc2-core-foundation doesn't necessarily want that as a dependency. Do we then create a new crate retain_utils?

• Retained supports more operations in Objective-C than elsewhere (Retained::retain_autoreleased, Retained::autorelease, conversion to/from rc::Weak etc.). How would you model that (simply) with a delegation pattern?

These are good points. But shouldn't these operations like autorelease work flawlessly with os_objects given that they are objc types themselves? So then it's up to runtime to figure out whether it can take a short cut or take a detour over objc2 message.

I think abstraction is a good idea when there really is a general pattern to abstract over. But in this case there is a fairly limited set of different retain/release functions (I believe the full list that we'll ever care about is libdispatch, the CoreFoundation framework, libobjc, and maybe the Network, Security and IOKit frameworks), and so I believe "specialization" (as an antonym to abstraction) is the better approach for us here.

That might work but perhaps not all that important right now.

I'd rather make the default case fast, i.e. use and return dispatch2::Retained (or whatever it ends up being called, naming suggestions welcome), and then allow the user to explicitly convert to objc2::rc::Retained if they really need Objective-C compatibility (which should be fairly rare). That is, it's more important to me to create a valley of success than always using the strictly most performant solution.

Yeah in my latest effort I copy-catted it as dispatch2::rc::Retained. I think it's fine to keep the same name. For the compatibility, I can only think of bridging DispatchData to NSData so having a way to go from dispatch2::Retained to objc2::rc::Retained is a nice touch.

(Also, let's say that you take objc2::rc::Retained<NSData>, and convert it into the hypothetical objc2::rc::Retained<dispatch2::Data>; in that case, even though the type is dispatch2::Data, then user would still want objc_retain, but we'd have no way of detecting that).

Yeah. It's a difficult to guarantee consistency when the API allows passing raw pointers. Perhaps we should not focus on that as objc messages work universally for these kinds of objects.

[pronebird]

Quick question: Why do we need to define dispatch objects with _inner: [u8; 0]? Would it not be less confusing to omit it and define an empty struct?

#[repr(C)]
#[derive(Copy, Clone, Debug)]
#[allow(missing_docs)]
pub struct $type_name {
-    /// opaque value
-   _inner: [u8; 0],
}

[madsmtm]

Quick question: Why do we need to define dispatch objects with _inner: [u8; 0]? Would it not be less confusing to omit it and define an empty struct?

#[repr(C)]
#[derive(Copy, Clone, Debug)]
#[allow(missing_docs)]
pub struct $type_name {
-    /// opaque value
-   _inner: [u8; 0],
}

Two reasons:

• Correctness, without the private field, users could do let my_semaphore = dispatch2::ffi::dispatch_semaphore_s { };.
• I seem to remember #[repr(C)] (or was it #[repr(transparent)]?) not working well with empty structs, but that might have changed in newer Rust versions.

---

It might take me a few days to review your PR btw (holidays), but the approach overall looks good, though in the end I'd probably like it split out into separate PRs (one for the removal of is_activated and making set_target_queue unsafe, one for converting DispatchObject into a trait, one for the Retained and one for Send + Sync).

[pronebird]

• Correctness, without the private field, users could do let my_semaphore = dispatch2::ffi::dispatch_semaphore_s { };.

Didn't think of that. That's undesired.

It might take me a few days to review your PR btw (holidays), but the approach overall looks good, though in the end I'd probably like it split out into separate PRs (one for the removal of is_activated and making set_target_queue unsafe, one for converting DispatchObject into a trait, one for the Retained and one for Send + Sync).

No rush.

I am thinking of using something akin to objc2::rc::Retained<DispatchQueue>, especially so since it'd allow e.g. &DispatchQueue to be safe to pass across an ABI boundary. But there are complications with that, see #77 (comment), so it might take a little while.

I took a bit of time and looked into returning Retained<Queue> from Queue::new() but failed to succeed with this :S Maybe I misunderstood you on this one.

Also I am not entirely sure Sync is safe for Queue and other types in current configuration where types hold Retained<T> which was a source of quite a few crashes in my tests.

[pronebird]

After poking around I figured how to cast between transparent struct and pointer. I assume this is the end result that you wanted to see from the beginning.

[pronebird]

I somewhat gave up on trying to avoid a retain or release of global queues. I think that those calls are no-op anyway so perhaps it doesn't matter.

[madsmtm]

Hey, checking back to say that I haven't forgotten this PR, have just been making a few too many promises that I can't uphold, and I'm really behind on the next release (which I've promised in more places, so I'm trying to focus on that one), sorry about the continued delay :/