Use objc2-metal
#5641
Open
Use objc2-metal
#5641
+1,346
−1,168
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
56 tasks
madsmtm
added a commit
to madsmtm/objc2
that referenced
this pull request
May 20, 2024
This may technically be a breaking change if the user implemented these protocols themselves on a `Mutable` class, but that'd be unsound anyhow, so I'll consider this a correctness fix. This is useful for wgpu, see gfx-rs/wgpu#5641, and the hack will become unnecessary after #563.
madsmtm
force-pushed
the
objc2-metal
branch
2 times, most recently
from
92d3ca3
to
bd69f0d
Compare
To keep the diff smaller and easier to review, this uses a temporary fork of `objc2-metal` and `objc2-quartz-core` whose methods use the naming scheme of the `metal` crate. One particular difficult part with this is that the `metal` crate has several methods where the order of the arguments are swapped relative to the corresponding Objective-C methods. This includes most perilously (since these have both an offset and an index argument, both of which are integers): - `set_bytes` - `set_vertex_bytes` - `set_fragment_bytes` - `set_buffer` - `set_vertex_buffer` - `set_fragment_buffer` - `set_threadgroup_memory_length` But also: - `set_vertex_texture` - `set_fragment_texture` - `set_sampler_state` - `set_vertex_sampler_state` - `set_fragment_sampler_state`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Use the
objc2-metalcrate instead of themetalcrate. This will:ArcandMutex, as Metal objects are already reference-counted (depending on thread-safety details, not entirely sure).objc_retainAutoreleasedReturnValueunderneath the hood to avoid putting objects into the autorelease pool when possible, reducing memory pressure.Background
The
metalcrate contains bindings to the Metal framework. This usesobjcto manually perform the message sends, which is quite error-prone, see gfx-rs/metal-rs#284, gfx-rs/metal-rs#319 and gfx-rs/metal-rs#209 for a few examples of unsoundness (out of many).To solve such problems in the Rust ecosystem in general, I created a successor to
objccalledobjc2, which contains most notably the smart-pointerRetainedand the macromsg_send_id!, which together ensure that Objective-C's memory-management rules are upheld correctly.This is only part of the solution though - we'd still have to write the bindings manually. To solve this, I created a tool (planning to integrate it with
bindgen, but that's likely a multi-year project) to generate such framework crates automatically. In acknowledgement that this tool is by far not perfect, and never will be, I've ensured that there's a bunch of options to modify each generated crate.The modifications for
objc2-metalin particular are currently just a few hundred lines of code, weak evidence that the generator is fairly good at this point. I'll also bring attention to the file whereunsafemethods are marked safe - I have plans to investigate ways to semi-automatically figure out if something is safe or not, or at least reduce the burden of doing so, but it's a hard problem to ensure is completely sound, so for now it's a bit verbose.Connections
gpu-allocatoris also transitioning toobjc2-metalin Traverse-Research/gpu-allocator#225.gfx-rs/metal-rs#241 is an old open PR for using
objc2inmetalinternally instead. There currently isn't really a clear path forwards there, and it's a lot of work for less direct benefits to the ecosystem (wgpu-halis by far the biggest user ofmetal). But more fundamentally IMO, it's a problem of separation of concerns;metaldefines several Foundation types likeNSArray,NSString,NSErrorand so on, and evenCAMetalLayerfrom QuartzCore, and that's a bad idea for interoperability compared to having separate crates for each of these frameworks.Implementation
The first commit removes the
linkfeature which is not available in these crates. This was added by @crowlKats in #3853, but Deno doesn't actually use it from what I can tell, they instead specify this using-weak_framework, which is the correct solution to this problem IMO. (If I'm wrong about this, please say so, I could be persuaded to add a similar feature to theobjc2-...crates).The second commit implements the actual migration, by using a branch of
objc2, with a method naming scheme that (more closely) matchesmetal, to make it easier to review and test what's changed.The third commit moves to the real naming scheme that
objc2uses.I'd strongly recommend you review these two commits separately.
Testing
Tested by using the checklist below, as well as running each example individually, and checking that they seem to work.
During the development of this I made two quite critical typos, which were luckily found by the test suite, but there's bound to be at least one more lurking in here somewhere, please test this thoroughly!
Checklist
cargo fmt.cargo clippy.cargo xtask testto run tests.CHANGELOG.md. See simple instructions inside file.