-
Notifications
You must be signed in to change notification settings - Fork 57
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
Sending messages is inefficient because of selector lookup #49
Comments
I've been trying to mimic how clang does codegen for selectors. If you, say, call the .section __TEXT,__objc_methname,cstring_literals
L_OBJC_METH_VAR_NAME_: ## @OBJC_METH_VAR_NAME_
.asciz "hash"
.section __DATA,__objc_selrefs,literal_pointers,no_dead_strip
.align 3 ## @OBJC_SELECTOR_REFERENCES_
L_OBJC_SELECTOR_REFERENCES_:
.quad L_OBJC_METH_VAR_NAME_ And then the selector passed to If I naively try to mimic this, with some code like: const char *const MY_SELECTOR = "hash";
SEL selector = MY_SELECTOR;
NSUInteger hash = objc_msgSend(object, selector); It does not go well, terminating with an unrecognized selector after printing:
With the naive mimicry, the generated code looks like: .section __TEXT,__cstring,cstring_literals
L_.str: ## @.str
.asciz "hash"
.section __DATA,__const
.globl _MY_SELECTOR ## @MY_SELECTOR
.align 3
_MY_SELECTOR:
.quad L_.str The primary difference seems to be in the sections, particularly where So, some questions from here that I need to resolve:
|
I'll respond more in a bit but I got this sort of working:
Unfortunately, there doesn't seem to be a great way to generate the literal 9 constant. I'm going to try to prototype a solution with a coworker in the new year. |
Ooh interesting, good find with This helped me get closer to the compiler's output in C with: __attribute__((section("__DATA,__objc_selrefs")))
char *const MY_SELECTOR = "hash"; Which produces: .section __TEXT,__cstring,cstring_literals
L_.str: ## @.str
.asciz "hash"
.section __DATA,__objc_selrefs,literal_pointers,no_dead_strip
.globl _MY_SELECTOR ## @MY_SELECTOR
.align 3
_MY_SELECTOR:
.quad L_.str Unfortunately then it fails to link with "Undefined symbols for architecture x86_64: |
I recall getting a segfault on load when the method name was not in the "__objc_methname" section. |
I threw together a simple macros 1.1-abuse crate which should generate code similar to the code which @jrmuizel posted above. It's super hacky but if you wrap it in a macro_rules! wrapper it might look OK? I also haven't ensured that the output binary looks correct yet - so that's a thing too. |
Heads up - we appear to suffer from this issue heavily in gfx-backend-metal. |
I tried this piece of code in metal-rs with no success: impl DeviceRef {
pub fn name2(&self) -> &str { // new method
struct Foo(*const [u8; 5]);
unsafe impl Send for Foo {}
unsafe impl Sync for Foo {}
let s: &NSString = {
#[no_mangle]
#[link_section="__TEXT,__objc_methname,cstring_literals"]
static OBJC_METH_VAR_NAME_ : [u8; 5] = *b"name\0";
#[no_mangle]
#[link_section="__DATA,__objc_selrefs,literal_pointers,no_dead_strip"]
static OBJC_SELECTOR_REFERENCES_: Foo = Foo(&OBJC_METH_VAR_NAME_);
unsafe {
let selector: objc::runtime::Sel = mem::transmute(OBJC_SELECTOR_REFERENCES_.0);
objc::__send_message(&*self, selector, ()).unwrap()
}
};
s.as_str()
}
pub fn name(&self) -> &str { // old method
unsafe {
let name: &NSString = msg_send![self, name];
name.as_str()
}
}
} Result is:
|
@kvark is it certain methods being called in a tight loop that are causing a problem for gfx-backend-metal? If you can identify the methods, you could avoid this by registering the selector once and reusing it. Something like this untested code: let sel = sel!(name);
loop {
let name: &NSString = obj.send_message(sel, ());
} Maybe the selectors could be loaded in a lazy static? |
@SSheldon we are definitely considering that. It will be unfortunate to throw this out once the real static linking of selectors work out... |
I was never able to successfully static link a selector :/ @mystor were you seeing success with the macro crate you posted? I never got around to trying it myself. |
We attempted to use the code @mystor provided, gone through a few iterations with them, but ended up with a linker error. The plan was to go back and try to hard-code it to see if the simplest case works. It didn't, even though for a different reason - selector being not found (run-time error). The problem with explicit caching of |
In the example you gave above the selectors are not being registered during init for some reason. I'll look into it further. |
I think it might be because of a missing __objc_imageinfo section |
Indeed adding: #[no_mangle]
#[link_section="__DATA,__objc_imageinfo,regular,no_dead_strip"]
static info_version: u32 = 0;
#[no_mangle]
#[link_section="__DATA,__objc_imageinfo,regular,no_dead_strip"]
static info_flags: u32 = 64; fixes the selector missing error. |
I think something is still broken in the example code that I have though. |
I confirm - the simple example works with this addition 🎉 |
For the record here's a working example: #[macro_use]
extern crate objc;
use objc::Encode;
use objc::runtime::{Class, Object};
/// Wrapper around an `Object` pointer that will release it when dropped.
struct StrongPtr(*mut Object);
impl std::ops::Deref for StrongPtr {
type Target = Object;
fn deref(&self) -> &Object {
unsafe { &*self.0 }
}
}
impl Drop for StrongPtr {
fn drop(&mut self) {
let _: () = unsafe { msg_send![self.0, release] };
}
}
fn main() {
// Get a class
let cls = Class::get("NSObject").unwrap();
println!("NSObject size: {}", cls.instance_size());
// Allocate an instance
let obj = unsafe {
let obj: *mut Object = msg_send![cls, alloc];
let obj: *mut Object = msg_send![obj, init];
StrongPtr(obj)
};
// Invoke a method on the object
let hash: usize = unsafe {
msg_send![obj, hash]
};
println!("NSObject hash: {:x}", hash);
let hash: usize = {
use std::mem;
struct Foo(*const [u8; 5]);
unsafe impl Send for Foo {}
unsafe impl Sync for Foo {}
#[no_mangle]
#[link_section="__TEXT,__objc_methname,cstring_literals"]
static OBJC_METH_VAR_NAME_ : [u8; 5] = *b"hash\0";
#[no_mangle]
#[link_section="__DATA,__objc_imageinfo,regular,no_dead_strip"]
static info_version: u32 = 0;
#[no_mangle]
#[link_section="__DATA,__objc_imageinfo,regular,no_dead_strip"]
static info_flags: u32 = 64;
#[no_mangle]
#[link_section="__DATA,__objc_selrefs,literal_pointers,no_dead_strip"]
static OBJC_SELECTOR_REFERENCES_: Foo = Foo(&OBJC_METH_VAR_NAME_);
unsafe {
let selector: objc::runtime::Sel = mem::transmute(OBJC_SELECTOR_REFERENCES_.0);
objc::__send_message(&*obj, selector, ()).unwrap()
}
};
println!("NSObject hash: {:x}", hash);
} What's the linking problem? |
Also, it's worth noting that the 32 bit ABI requires the use of different sections. |
Can repro by: git clone https://github.com/kvark/metal-rs -b testcase
cd metal-rs && cargo build |
|
|
Isn't the objc branch missing the With those changes d836878 I get a different error message:
|
The link error is caused by just the following change: diff --git a/Cargo.toml b/Cargo.toml
index c420754..7af5116 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -17,22 +17,25 @@ default-target = "x86_64-apple-darwin"
cocoa = "0.15"
bitflags = "1"
libc = "0.2"
log = "0.4"
objc-foundation = "0.1"
objc_id = "0.1"
block = "0.1.5"
foreign-types = "0.3"
[dependencies.objc]
-version = "0.2.1"
-features = ["objc_exception"]
+#version = "0.2.1"
+git = "https://github.com/mystor/rust-objc"
+branch = "static_sel"
+features = ["objc_exception", "static_sel"]
+#features = ["objc_exception"]
[dev-dependencies]
winit = "0.13"
sema = "0.1.4"
[[example]]
name = "window"
path = "examples/window/main.rs"
[[example]] |
@jrmuizel The branch |
What I meant is that the link errors are not caused by the other changes happening in kvark/metal-rs@2021415 |
I've put a version that seems to sort of work at https://github.com/jrmuizel/rust-objc/tree/static_sel. It can successfully run |
I believe the linker errors in metal-rs are coming from msg_send! being used in a generic function. |
I remember that generic functions and statics don't mix that well, but I don't exactly recall why. |
Oh yeah, dyld3, and then who knows what Mojave/iOS 12 is going to bring? However, I suspect that anything would be better than taking the lock in |
(I'm fairly sure that that dyld3 code path is not live for things other than Apple libraries, at least on iOS 11 and below.) |
Improves SSheldon#49. This is what the assembly looks like. The fast path is only 4 instructions. lea r15, [rip + 0xbf936] ; cocoa::foundation::NSString::alloc::register_sel::SEL::h005daf5ee04d2745 mov rbx, qword ptr [r15] test rbx, rbx jne ok lea rdi, [rip + 0x95b2c] ; byte_str.o.llvm.2369336028792347561 call 0x10008fa02 ; symbol stub for: sel_registerName mov rbx, rax mov qword ptr [r15], rbx ok:
FWIW, I've created a small project called objrs that has a collection of macros that transforms Rust code so that it compiles down to effectively the same assembly as pure Objective-C. It never calls |
@mjbshaw would you be open to integrating your ideas into this crate directly? There are already quite a few crates built on top of |
@grovesNL For example, people here are mentioning that using This is just one of many thorny aspects I've had to work around. My concern is primarily the stability of the workarounds. I think the best way to integrate the parts of my |
@mjbshaw That sounds great! Thanks for the explanation. I'll try to follow |
@mjbshaw what progress have you made since your comment? I'm looking to have static selectors for my project (issue: nvzqz/fruity#2). |
@nvzqz The |
@mjbshaw I'm curious if you run into rust-lang/rust#80019 and if so, how you get past it. |
Yes, I'm able to get past that issue. The problem is that symbol names are significant for selectors and their references, and you aren't using the special symbol names required. The Selectors must use the |
@mjbshaw I didn't know that was the secret sauce to make it work. I just tested making a selector with those symbols and having an objc image info static. I'm pleased to say that I got it working in my project! Thanks so much for the pointer. I'll give you credit by adding you as a co-author to the commit. 😄 |
Any updates on this? I'm running into selector problems with gfx-rs/metal-rs#207, and I think having compile-time selectors would really help. |
I've been spending a lot of time the last few weeks thinking about and fiddling with this issue. I'd like to talk about what I've gotten done: My progressI've been able to reproduce the optimizations described previously in this thread. I've written a library that produces the linker instructions that create compile-time selector references and class references. I've also done a lot of testing with my implementation:
I also hacked together a implementation within a local version of
Hopefully that list of real-world crates like If it's still on the table, I'd love to start tinkering with getting this functionality into
I don't think either of those are obvious deal-breakers, but I figure I should voice them now. |
A other small concern I have with using the special sections clang uses is how it impacts app startup time. When used in clang, these sections are only included if the selector is used in the final app code or its libraries. When used with Rust, most apps will probably use crates like cocoa-foundation, that references a lot of selectors, most likely not used by the app. And last time I looked, these sections stay even if the selector is not used (they are |
I personally am quite curious how you managed to merge the selectors from various rlib into a single section. It’s not obvious to me how that can be achieved even in a proc macro, would be really interesting to study it. Startup time certainly could be an issue, but IMO could probably be resolved through feature flags at the class or framework level. I dunno if that’s acceptable for objc but I have support for it in another project and it performs well enough for my use. |
@vincentisambart Great thought! I didn't consider that yet, so here's what I found out looking into it: First, I wrote up a super contrived example of what you were describing: Contrived test caseunsafe fn not_called(obj: *mut objc::runtime::Object) {
msg_void![obj, not_called];
}
unsafe fn can_be_optimized_out(obj: *mut objc::runtime::Object) {
msg_void![obj, optimized_out];
}
fn main() {
unsafe {
let obj: *mut objc::runtime::Object = msg![class!(NSObject), new];
if 1 != 1 {
can_be_optimized_out(obj);
}
println!("{:016x}", obj as usize);
}
} There are two cases to note here: one where the function is never called at all, then another that is called but can be predictably optimized out. In a debug build, all three selectors are present in the binary:
That doesn't look very good. ❌
For comparison, a release build looks more like what you would expect:
The dead selectors are stripped from the binary in the release build. ✅ To summarize the above section: I found that unused selectors were showing up in debug builds 👎, but were properly stripped out in release builds 👍. That's a little bit of good news but still concerning. Alright, so back to the debug build and its 👻 ghost selectors. Looking at the assembly output from rustc, I did find some references to those selectors: Assembly snippet
Rust's generated debug info includes the selector references, therefore causing those otherwise-unused selectors to show up in debug builds. This also explains why those selectors would disappear in release builds. I checked the debug info section for regular Objective-C code (with clang) and these symbols do not show up in the debug info sections. A follow-up test also shows that Rust generates debug data for all statics in debug builds, even those that are never used1. However, even though this debug info is generated and shows up in the assembly output, the unused statics don't actually show up in the final binary2. Not really sure what to do about this, but it does seem like this debug data could be a minor issue for debug builds. Notes
|
@clavin can you put your implementation up some place so we can take a look at it? |
@jrmuizel Sure! Here's my quick hack (read: very dirty impl) in You can test it out on real-world projects that rely on the objc = { git = "https://github.com/clavin/rust-objc.git", branch = "static-refs", features = ["static_references"] }
# NOTE: the "static_references" feature must be enabled! Note: you might have to also repeat this for upstream crates of whatever you're testing on (e.g.
|
So I can't comment on what you really want to know, which is "is this acceptable for the crate". However, I have been under-the-table working on my own objc crate (partly due to this issue). So I can vouch for the fact that it's a problem that may justify significant changes, and I have reasonably informed opinions on what a good implementation looks like. This looks like a good implementation to me.
I believe you are looking for this. However it's not immediately clear to me how to use this from Rust (does Rust even use clang? Or is it just llvm?) On the other hand there appear to annotations for this in the LLVM IR so I might assume that if you could get something injected into the IR it would work. But it seems IR control is not supported in rustc. From what I can see procmacros are the best solution for this at present. One thing I will nitpick though is the hash implementation. If you disassemble a real objc executable, you will see one entry per selector, even if dozens of libs all use For b, I think it would be ok to go down from 16 to 10-11 hex characters, based on a quick birthday attack. But am I right in thinking we have the full 0-255 range here, in which case 5-6 full bytes should be sufficient? However this still leaves a. The solution I have been toying with is having the user supply a prefix for the symbol rather than rely on a hash. As a result, crates that are interested in coordinating how to share selectors among each other can pick the same prefix, and get tighter code, whereas crates not wanting to bother with all that can use a prefix like Overall though I like this patch, I'm glad there's momentum building behind getting this done. |
I don't think you should be trying to get Clang / LLVM to do the work - the fact that But overall, really nice work, if you throw a PR at some point when you get further, I'd be happy to review it! |
I've updated my version of this using some of the tricks https://github.com/clavin/rust-objc/tree/static-refs. It seems to work pretty well and doesn't seem to need any of the ident hashing. I also dropped the The biggest remaining disadvantage that I can see is that the |
I've included this in my fork Notable differences from @clavin's version:
Haven't looked much at dead stripping of the final binaries, but I suspect it's the same as what @clavin ended up with (debug bad, release good). I included |
My three cents on some of the still open questions here. Stability of ident hashing: This is also done in the Stability of Optimizing hashing: Maybe it would be possible to have an atomically increasing counter in the proc macro somehow? That would for sure give the smallest names! Though maybe has problems with no longer producing reproducible binaries? Allowing selector sharing: Wouldn't that be possible by having a crate e.g. |
It would be better if selectors were resolved at compile time instead of using sel_registerName
The text was updated successfully, but these errors were encountered: