Skip to content

Commit

Permalink
Remove objc_alloc optimization from msg_send!
Browse files Browse the repository at this point in the history
It's fairly difficult to make it trigger correctly in all situations, so
let's instead use it internally in the safe `alloc` functions. This
should push users towards using those instead.
  • Loading branch information
madsmtm committed Jan 21, 2025
1 parent 913d3eb commit 04077a2
Show file tree
Hide file tree
Showing 15 changed files with 118 additions and 100 deletions.
3 changes: 3 additions & 0 deletions crates/objc2/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
* **BREAKING**: Removed the ability to implement `ClassType` manually, to make
it easier to evolve the API going forwards.
* **BREAKING**: Removed the deprecated `apple` Cargo feature flag.
* The optimization for converting `msg_send_id![cls, alloc]` to a call to
the faster runtime function `objc_alloc` no longer works, use
`AllocAnyThread::alloc` or `MainThreadOnly::alloc` instead.

### Fixed
* Remove an incorrect assertion when adding protocols to classes in an unexpected
Expand Down
5 changes: 0 additions & 5 deletions crates/objc2/src/__macro_helpers/method_family.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,6 @@ pub type MutableCopyFamily = MethodFamily<5>;
/// No family.
pub type NoneFamily = MethodFamily<6>;

/// The `alloc` selector itself.
///
/// Used for a fast-path optimization using `objc_alloc`.
pub type AllocSelector = MethodFamily<7>;

// These are used to avoid trying to do retain-semantics for these special
// selectors that would otherwise fall under `NoneFamily`.

Expand Down
6 changes: 3 additions & 3 deletions crates/objc2/src/__macro_helpers/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,9 @@ pub use self::define_class::{
pub use self::defined_ivars::DefinedIvarsHelper;
pub use self::image_info::ImageInfo;
pub use self::method_family::{
method_family, method_family_import, AllocFamily, AllocSelector, AutoreleaseSelector,
CopyFamily, DeallocSelector, InitFamily, MethodFamily, MutableCopyFamily, NewFamily,
NoneFamily, ReleaseSelector, RetainSelector,
method_family, method_family_import, AllocFamily, AutoreleaseSelector, CopyFamily,
DeallocSelector, InitFamily, MethodFamily, MutableCopyFamily, NewFamily, NoneFamily,
ReleaseSelector, RetainSelector,
};
pub use self::module_info::ModuleInfo;
pub use self::msg_send_retained::{MsgSend, MsgSendError, MsgSendSuper, MsgSendSuperError};
Expand Down
47 changes: 4 additions & 43 deletions crates/objc2/src/__macro_helpers/msg_send_retained.rs
Original file line number Diff line number Diff line change
@@ -1,14 +1,13 @@
use core::ptr;

use crate::encode::{Encode, RefEncode};
use crate::rc::{Allocated, Retained};
use crate::rc::Retained;
use crate::runtime::{AnyClass, MessageReceiver, Sel};
use crate::{ClassType, Message};
use crate::ClassType;

use super::null_error::encountered_error;
use super::{
AllocSelector, ConvertArguments, KindSendMessage, KindSendMessageSuper, RetainSemantics,
TupleExtender,
ConvertArguments, KindSendMessage, KindSendMessageSuper, RetainSemantics, TupleExtender,
};

//
Expand Down Expand Up @@ -41,44 +40,6 @@ where
}
}

impl<Return> MsgSend<&AnyClass, Allocated<Return>> for AllocSelector
where
Return: Message,
{
#[inline]
unsafe fn send_message<A: ConvertArguments>(
cls: &AnyClass,
sel: Sel,
args: A,
) -> Allocated<Return> {
// Available on non-fragile Apple runtimes.
#[cfg(all(
target_vendor = "apple",
not(all(target_os = "macos", target_arch = "x86"))
))]
{
// We completely ignore both the selector and the arguments, since
// we know them to be `alloc` and empty (since this is the
// `AllocSelector` "family").
let _ = sel;
let _ = args;

// SAFETY: Checked by caller.
let obj: *mut Return = unsafe { crate::ffi::objc_alloc(cls).cast() };
// SAFETY: The object is newly allocated, so this has +1 retain count
unsafe { Allocated::new(obj) }
}
#[cfg(not(all(
target_vendor = "apple",
not(all(target_os = "macos", target_arch = "x86"))
)))]
{
// SAFETY: Checked by caller
unsafe { super::AllocFamily::send_message(cls, sel, args) }
}
}
}

//
// MsgSendSuper
//
Expand Down Expand Up @@ -357,7 +318,7 @@ mod tests {

use super::*;

use crate::rc::{autoreleasepool, PartialInit, RcTestObject, ThreadTestData};
use crate::rc::{autoreleasepool, Allocated, PartialInit, RcTestObject, ThreadTestData};
use crate::runtime::{AnyObject, NSObject, NSZone};
use crate::{class, define_class, extern_methods, msg_send, test_utils, AllocAnyThread};

Expand Down
4 changes: 2 additions & 2 deletions crates/objc2/src/__macro_helpers/os_version/apple.rs
Original file line number Diff line number Diff line change
Expand Up @@ -207,8 +207,8 @@ fn version_from_plist() -> OSVersion {
let path = PathBuf::from(root).join("System/Library/CoreServices/SystemVersion.plist");
let path = path.as_os_str().as_bytes();

// SAFETY: Allocating a string is valid.
let alloc: Allocated<NSObject> = unsafe { msg_send![class!(NSString), alloc] };
// SAFETY: Allocating a string is valid on all threads.
let alloc: Allocated<NSObject> = unsafe { Allocated::alloc(class!(NSString)) };
// SAFETY: The bytes are valid, and the length is correct.
unsafe {
let bytes_ptr: *const c_void = path.as_ptr().cast();
Expand Down
2 changes: 1 addition & 1 deletion crates/objc2/src/macros/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1197,7 +1197,7 @@ macro_rules! msg_send {
});
[$obj:expr, alloc $(,)?] => ({
let result;
result = <$crate::__macro_helpers::AllocSelector as $crate::__macro_helpers::MsgSend<_, _>>::send_message(
result = <$crate::__macro_helpers::AllocFamily as $crate::__macro_helpers::MsgSend<_, _>>::send_message(
$obj,
$crate::sel!(alloc),
(),
Expand Down
4 changes: 2 additions & 2 deletions crates/objc2/src/main_thread_marker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use core::fmt;
use core::marker::PhantomData;

use crate::rc::Allocated;
use crate::{msg_send, ClassType, MainThreadOnly};
use crate::{ClassType, MainThreadOnly};

/// Whether the current thread is the main thread.
#[inline]
Expand Down Expand Up @@ -161,7 +161,7 @@ impl MainThreadMarker {
// SAFETY: We hold `MainThreadMarker`, and classes are either only
// safe to allocate on the main thread, or safe to allocate
// everywhere.
unsafe { msg_send![T::class(), alloc] }
unsafe { Allocated::alloc(T::class()) }
}
}

Expand Down
36 changes: 35 additions & 1 deletion crates/objc2/src/rc/allocated_partial_init.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use core::ptr::NonNull;
use core::{fmt, ptr};

use crate::__macro_helpers::defined_ivars::initialize_ivars;
use crate::runtime::{objc_release_fast, AnyObject};
use crate::runtime::{objc_release_fast, AnyClass, AnyObject};
use crate::{DefinedClass, Message};

/// An Objective-C object that has been allocated, but not initialized.
Expand Down Expand Up @@ -83,6 +83,40 @@ impl<T: ?Sized + Message> Allocated<T> {
}
}

/// Allocate the object with a fast path using `objc_alloc`.
///
///
/// # Safety
///
/// The object must be safe to allocate on the current thread, and the
/// object `T` must be an instance of the class.
#[doc(alias = "objc_alloc")]
#[inline]
pub(crate) unsafe fn alloc(cls: &AnyClass) -> Self
where
T: Sized,
{
// Available on non-fragile Apple runtimes.
#[cfg(all(
target_vendor = "apple",
not(all(target_os = "macos", target_arch = "x86"))
))]
{
// SAFETY: Thread safety checked by the caller.
let obj: *mut T = unsafe { crate::ffi::objc_alloc(cls).cast() };
// SAFETY: The object is newly allocated, so this has +1 retain count
unsafe { Self::new(obj) }
}
#[cfg(not(all(
target_vendor = "apple",
not(all(target_os = "macos", target_arch = "x86"))
)))]
{
// SAFETY: Thread safety checked by the caller.
unsafe { crate::msg_send![cls, alloc] }
}
}

/// Returns a raw pointer to the object.
///
/// The pointer is valid for at least as long as the `Allocated` is held.
Expand Down
9 changes: 6 additions & 3 deletions crates/objc2/src/top_level_traits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use crate::__macro_helpers::defined_ivars::get_initialized_ivar_ptr;
use crate::encode::RefEncode;
use crate::rc::{Allocated, Retained};
use crate::runtime::{AnyClass, AnyProtocol, ProtocolObject};
use crate::{msg_send, MainThreadMarker};
use crate::MainThreadMarker;

/// Types that can be sent Objective-C messages.
///
Expand Down Expand Up @@ -455,7 +455,7 @@ pub unsafe trait AllocAnyThread: private::SealedAllocAnyThread {
// to be allowed to `init` on the current thread.
//
// See also `MainThreadMarker::alloc`.
unsafe { msg_send![Self::class(), alloc] }
unsafe { Allocated::alloc(Self::class()) }
}
}

Expand Down Expand Up @@ -551,7 +551,10 @@ pub unsafe trait MainThreadOnly: private::SealedMainThreadOnly {
where
Self: Sized + ClassType,
{
mtm.alloc()
let _ = mtm;
// SAFETY: We hold `MainThreadMarker`, and the class is safe to
// allocate on the main thread.
unsafe { Allocated::alloc(Self::class()) }
}
}

Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading

0 comments on commit 04077a2

Please sign in to comment.