Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use objc2 instead of objc #241

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Use objc2 instead of objc #241

wants to merge 12 commits into from

Conversation

madsmtm
Copy link
Contributor

@madsmtm madsmtm commented Sep 1, 2022

Replace objc with my fork, objc2.

This crate contains many improvements, in particular:

  • Better tools for protecting against UB (see first commit)
  • You no longer have to worry about using bool in msg_send!.
  • The msg_send_id! macro greatly helps with follow memory management rules (similar to Objective-C ARC), potentially also alleviating a lot of the need for autoreleasepools.
  • The extern_protocol! macro is used in place of foreign_obj_type!

Notable breaking changes:

  • objc2::Message differs from objc::Message, so message sending is not compatible

  • NSUInteger is now usize instead of u32/u64 depending on platform (equivalently for NSInteger)

  • There is no longer three types for each class (e.g. MTLFoo, Foo and FooRef); these have been combined into one: Foo. Pointers to this can be used across FFI boundaries, and objc2::rc::Id<Foo, Shared> can be used as a strong pointer. In table form:

    Previously Now
    *mut MTLFoo *mut Foo
    &FooRef &Foo
    Foo Id<Foo, Shared>

When done, this should resolve the following issues:

Copy link
Contributor Author

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kvark @cwfitzgerald I think I'm ready for some feedback on this before I proceed; there's a lot of things going on here, but I'd like you to take a look at e.g. the last commit, where I've changed things to use the extern_class!, extern_protocol! and msg_send_id! macros.

Feel free to ask me to clarify things about objc2 in general, I recognize that the documentation of it is not (yet) ideal.

src/sync.rs Outdated Show resolved Hide resolved
src/sync.rs Show resolved Hide resolved
@i509VCB
Copy link

i509VCB commented Nov 20, 2022

The pull request title appears to be wrong, shouldn't objc and objc2 be swapped in the title?

@madsmtm madsmtm changed the title Use objc instead of objc2 Use objc2 instead of objc Nov 20, 2022
@madsmtm
Copy link
Contributor Author

madsmtm commented Jan 9, 2023

An update on this:

I decided to make an automatically generated wrapper for Objective-C frameworks called icrate. There are a few downsides to an automatically generated library, but most of these can be mitigated by manual additions, which I've explicitly designed icrate to support.

In madsmtm/objc2#329 I've added support for Metal, with the primary addition being the file crates/header-translator/src/data/Metal.rs, which details which methods are safe (methods are naturally unsafe by default).

This library fixes the same issues as described in the PR description, as well as paving a way forwards for many other improvements, such as:

  • Automatic API availability / deprecation (so that users only call new methods when available).
  • Quick SDK updating (e.g. Adding support for the raytracing. #252 would've just been: Download the new SDK, rerun the build script, and mark safe methods).
  • Reduced risk of mistakes that lead to unsoundness (for example, all of metal's enum definitions are unsound, since a future version of the OS is allowed to add more variants, and that would break Rust's assumption that the enum is exhaustive).

I'm still working on improving how protocols are mapped, which has a big effect on Metal - would like some help with this in madsmtm/objc2#291 (and maybe Metal protocols should work differently from other frameworks?).


I would also like to discuss with the maintainers (@kvark @cwfitzgerald); what do you think the best way forwards is?

I shall be blunt 😉: My ideal future would be for the metal crate just being a pub use icrate::Metal::*.

To achieve this I'm very open to changing things that you may be unhappy with, like the method naming scheme or adding collaborators to icrate/objc2 if you would be more comfortable with it if the bus factor was decreased?

I'm reachable on Matrix as @madsmtm:matrix.org, and will probably see a ping on #wgpu:matrix.org as well, if you want to have a more synchronous discussion.

@grovesNL
Copy link
Collaborator

I would also like to discuss with the maintainers (@kvark @cwfitzgerald); what do you think the best way forwards is?

I shall be blunt 😉: My ideal future would be for the metal crate just being a pub use icrate::Metal::*.

To achieve this I'm very open to changing things that you may be unhappy with, like the madsmtm/objc2#284 or adding collaborators to icrate/objc2 if you would be more comfortable with it if the bus factor was decreased?

Looks really great so far! Awesome work and I'm excited about fixing some of the autorelease pool and selector issues we've discussed over the years.

I'm a little hesitant about generating all bindings and placing them all in icrate. A few other projects have taken a similar approach to replace objc (e.g., objr, fruity, objrs, etc.) but have never achieved broad adoption by the existing ecosystem, so I'm not confident that objc2/icrate will become the standard yet (though I acknowledge that this is a bit of a chicken-and-egg problem).

For example, ideally I don't think it should be the metal's crate responsibility to say that we only work with NSColors that originate from objc2/icrate, but I could see us easily stuck in that position if we switch.

Any of the following would help a lot:

  • more collaborators to reduce the bus factor
  • confidence that the generated bindings are a net time saver/reduce maintenance complexity
    • e.g., lowering the amount of manual work instead of often having to patch the generator itself or write a lot of manual fixes on top of the output
  • clear signs that the ecosystem will really move in this direction
    • e.g., maybe upgrading the existing ecosystem instead of replacing it where possible
  • better understanding around increases or decreases in compilation time or runtime performance

I'm also curious if there's a way we could adopt generated bindings internally without breaking the existing API too much (instead of, or as a temporary measure while we work towards being able to do pub use icrate::Metal::*).

Automatic API availability / deprecation (so that users only call new methods when available).

For what it's worth, we wouldn't want compile-time detection for this in all cases. For example, wgpu's Metal backend would want to be able to compile against the highest possible deployment target and decide at runtime which methods to use.

@madsmtm
Copy link
Contributor Author

madsmtm commented Jan 10, 2023

Looks really great so far! Awesome work and I'm excited about fixing some of the autorelease pool and selector issues we've discussed over the years.

Thanks! It's great to hear you raise some concerns, I'll try to respond to them below:

I'm a little hesitant about generating all bindings and placing them all in icrate. A few other projects have taken a similar approach to replace objc (e.g., objr, fruity, objrs, etc.) but have never achieved broad adoption by the existing ecosystem, so I'm not confident that objc2/icrate will become the standard yet (though I acknowledge that this is a bit of a chicken-and-egg problem).

Yeah, I'm aware, though still very much a valid point. If it helps, I'm the maintainer of winit and I plan to use it there, which will likely cause pressure on wgpu to at the very least switch to objc2 to reduce the dependency tree.

For example, ideally I don't think it should be the metal's crate responsibility to say that we only work with NSColors that originate from objc2/icrate, but I could see us easily stuck in that position if we switch.

Metal doesn't actually use NSColor, but I get what you mean with NSString, NSArray, NSDictionary, and so on ;).

In my defence, metal is already tied to objc's versioning, though yeah the problem does get worse with a large crate (and probably for a while a fast-moving target) like icrate.

Any of the following would help a lot:

* more collaborators to reduce the bus factor

Yeah, will hopefully get better with time. I'll probably make a call out on TWiR at some point for starters.

* confidence that the generated bindings are a net time saver/reduce maintenance complexity

  * e.g., lowering the amount of manual work instead of often having to patch the generator itself or write a lot of manual fixes on top of the output

I think it actually does really well on this mark already (I was surprised myself, I thought I'd have to implement a bunch more hacks for Metal), the only real burden was in creating the data file for which methods are unsafe, which is as it should be, that part should require attention.

* clear signs that the ecosystem will really move in this direction

That's indeed on my roadmap: madsmtm/objc2#174

  * e.g., maybe upgrading the existing ecosystem instead of replacing it where possible

Could you provide an example of what you mean here?

* better understanding around increases or decreases in compilation time or runtime performance

Wrt. compilation time, I'm working on a system to help with this in madsmtm/objc2#311.

Runtime performance is kinda difficult to prove before the entirety of the migration is done, but I can at least say that the project has assembly-level tests for ensuring that the autorelease optimizations do happen.

I'm also curious if there's a way we could adopt generated bindings internally without breaking the existing API too much (instead of, or as a temporary measure while we work towards being able to do pub use icrate::Metal::*).

It is very much possible to do something like this:

use icrate::Metal::MTLDepthStencilDescriptor;

#[repr(u64)]
#[derive(Copy, Clone, Debug, Eq, PartialEq, Hash)]
pub enum MTLCompareFunction {
    Never = 0,
    Less = 1,
    Equal = 2,
    LessEqual = 3,
    Greater = 4,
    NotEqual = 5,
    GreaterEqual = 6,
    Always = 7,
}

// from_icrate and into_icrate for MTLCompareFunction, unless we simply export icrate's MTLCompareFunction


foreign_obj_type! {
    type CType = MTLDepthStencilDescriptor;
    pub struct DepthStencilDescriptor;
}
// Will now translate to roughly
{
    pub struct DepthStencilDescriptor(Id<MTLDepthStencilDescriptor, Shared>);

    #[repr(transparent)]
    pub struct DepthStencilDescriptorRef(MTLDepthStencilDescriptor);

    impl Deref<Target = DepthStencilDescriptorRef> for DepthStencilDescriptor { ... }

    impl DepthStencilDescriptor {
        // Could also be a `From` implementation, depending on how much you want
        // to expose that you're using `icrate` under the hood.
        fn from_icrate(obj: Id<MTLDepthStencilDescriptor, Shared>) -> Self {
            Self(obj)
        }
    
        fn into_icrate(...) { ... }
    }
    
    // from_icrate and into_icrate for DepthStencilDescriptorRef
}


impl DepthStencilDescriptor {
    pub fn new() -> Self {
        Self(MTLDepthStencilDescriptor::new())
    }
}

impl DepthStencilDescriptorRef {
    pub fn depth_compare_function(&self) -> MTLCompareFunction {
        MTLCompareFunction::from_objc2(self.0.depthCompareFunction())
    }

    pub fn set_depth_compare_function(&self, func: MTLCompareFunction) {
        self.0.setDepthCompareFunction(func.into_objc2());
    }

    pub fn depth_write_enabled(&self) -> bool {
        self.0.isDepthWriteEnabled();
    }

    pub fn set_depth_write_enabled(&self, enabled: bool) {
        self.0.setDepthWriteEnabled(enabled);
    }

    pub fn front_face_stencil(&self) -> Option<StencilDescriptor> {
        self.frontFaceStencil().map(StencilDescriptor::from_objc2)
    }

    pub fn set_front_face_stencil(&self, descriptor: Option<&StencilDescriptorRef>) {
        self.setFrontFaceStencil(descriptor.map(|d| d.into_objc2()))
    }

    // ...

    // Inefficient, but correct! An alternative would be to return a newtype `MetalString(Id<NSString, Shared>)` instead.
    pub fn label(&self) -> String {
        self.0.label().to_string()
    }

    pub fn set_label(&self, label: &str) {
        self.0.setLabel(&NSString::from_str(label));
    }
}

And this -sys pattern is what I would usually do for normal C-APIs; however, Objective-C is different in that they have quite high type-safety, which means, as you can see, a lot of things just end up being duplicated work.

But yes, it would definitely be a way forward, and I could probably do it at some point if you want.

Automatic API availability / deprecation (so that users only call new methods when available).

For what it's worth, we wouldn't want compile-time detection for this in all cases. For example, wgpu's Metal backend would want to be able to compile against the highest possible deployment target and decide at runtime which methods to use.

The plan is to have the checks be made at runtime, but in a way such that you'll get a panic if you use some functionality without having made the runtime check beforehand, similar to Swift's #available (which, yeah, will require a bit of trickery to get to work in Rust).


Again, thanks for the kind response!

@madsmtm madsmtm force-pushed the objc2 branch 4 times, most recently from 96fb19c to 88b3aa2 Compare August 1, 2023 14:16
@madsmtm
Copy link
Contributor Author

madsmtm commented Aug 1, 2023

Okay, so to get this moving forwards, I've reduced this PR to only a "move to objc2", and have kept the more fancy features and improvements out for now - they will come later, but I'd like to get the ball rolling if possible.

So, the primary thing that's changed is that all types now implement Encode, which allows us to ensure at compile-time that the type's ABI is valid (e.g. it's not Vec<u8>, &str or similar), and at runtime that the type matches what is expected by the method.
I really should write some better docs on this, but I recently explained it here, that may help - if not, feel free to ask!

Re compilation time, a clean build has not changed much, we've traded the core-graphics-types crate for icrate, and both of those compile faster than syn anyhow. But icrate is big, and not as optimized on that front as it should be, so there's definitely still room for improvement.


Note: This PR is a breaking change, both because of the changed Message impls for everything (e.g. code that wanted to send messages manually to metal types now have to use objc2, or have to do a cast), but also because I changed a lot of APIs to no longer directly depend on Objective-C types. Concretely, I made the following public-facing changes:

  • Removed NSInteger and NSUInteger type aliases, instead we just use isize and usize.
  • Removed NSRange, methods now take Range<usize> instead.
  • Use f64 in [set_]contents_scale instead of CGFloat.
  • Changed the signature of drawable_size from fn(&self) -> CGSize to fn(&self) -> (f64, f64).
  • Changed the signature of set_drawable_size from fn(&self, size: CGSize) to fn(&self, width: f64, height: f64).

src/lib.rs Show resolved Hide resolved
- argument.rs: These previously returned the owned types, which would cause a double-release
- library.rs: These returned `&*mut NSArray` instead of `&NSArray`
- device.rs: `retain` returns a pointer to the object, not `void`/`()`
- Implement Encode and RefEncode for most types
NSUInteger is now usize, so a bunch of `as u64` casts were removed
@madsmtm
Copy link
Contributor Author

madsmtm commented Sep 5, 2023

Just tested, all the examples except the ones in #282 now work (the errors in that issue are caught by debug assertions, which is why they don't).

@madsmtm
Copy link
Contributor Author

madsmtm commented Sep 5, 2023

@grovesNL: I'm a bit unsure because I think you asked about a lot of things that are not strictly relevant to this PR anymore (since I've kept the autogenerated bindings out for now), but think I've resolved/answered your questions, are there any outstanding concerns, or something else?

CC @kvark, @cwfitzgerald

@zackangelo
Copy link

Is there anything blocking the merge of this PR? I'm happy to help if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Host documentation
4 participants