metal: Migrate to objc2 architecture with objc2-metal bindings
#225
Conversation
src/metal/mod.rs
Outdated
| location: MemoryLocation, | ||
| ) -> AllocationCreateDesc<'a> { | ||
| let size_and_align = device.heap_acceleration_structure_size_and_align_with_size(size); | ||
| // TODO: Why is this one unsafe? |
There was a problem hiding this comment.
Likely just because no-one has marked it as safe.
There was a problem hiding this comment.
Oh that's interesting. How much manual work is involved in improving the representation of the Metal API (or anything objc in general)?
I never want my implementations to be one way, and would love to aid in contributing back upstream bindings improvements. Is there some documentation or guidance in what goes into deducing that a function is safe (or any other thing that I might want to change), and how/where to ultimately mark it as such?
I'll look into your crate documentation meanwhile!
There was a problem hiding this comment.
https://github.com/madsmtm/objc2/blob/master/crates/header-translator/src/data/Metal.rs looks like this is where I'll want to start making changes.
There was a problem hiding this comment.
That is the place, yeah. I've been considering designing an "auto-safe" system where you could mark an entire class as being safe, and then all the methods on that class that take references and not e.g. pointers would be marked safe.
But it's a tough issue, and not a burden I wanted to tackle right now, especially in the political sense (I cannot really afford for objc2 to be seen as something that plays quick and dirty with unsafe).
There was a problem hiding this comment.
I saw some of these methods were likely commented out and marked as unsafe because of possibly going out of bounds? E.g. setWidth: and friends, how do you deduce what happens here? And heapAccelerationStructureSizeAndAlignWithSize: too?
There was a problem hiding this comment.
how do you deduce what happens here
In reality, I don't really - the issue here is that I don't know enough about Metal to know what's safe and what isn't, the only reason that a lot of things are marked safe is mostly to retain approximate feature-compatibility with the metal crate, I haven't actually done a thorough review of everything there.
In the case of indexes I went with being overly cautious, they looked like something that might very easily run into UB if you passed Metal some buffer and then set the size of the buffer to something else, but it's very probable that Metal protects against this internally, and that it isn't actually an issue.
There was a problem hiding this comment.
That sounds very sensible. Sounds like the combination of common-sense and maybe runtime-testing (if I pass an invalid value: does Metal prevent me from (ab)using it by returning an error or otherwise)?
More importantly this might be something to document in the header-translator files directly (if not already done so), maybe serving a double purpose as # Safety docs when deducing that a function is explicitly unsafe (or maybe also why it would be "safe" if deduced as such).
the only reason that a lot of things are marked safe is mostly to retain approximate feature-compatibility with the
metalcrate
Ouch; I hope any failures there to not mark things unsafe when they really are unsafe don't trickle down into your crates. I think that's the kind of compatibility you shouldn't retain, to demonstrate that there's an obvious safety benefit in using objc2.
The current `objc` crate stack is completely unmaintained and has severely fallen out of date with no updates for over 4 years. The `metal-rs` crate, built on top of this architecture, is completely written by hand which is tedious to keep up-to-date, not to mention has inconsistencies in its implementation. All of this is superseded by the new `objc2` crate stack built by @madsmtm. Beyond providing what seems like a better, safer abstraction over Objective-C, _all_ bindings are completely autogenerated meaning we'll no longer lag behind upstream bindings (requiring painstaking manual patching) or have inconsistencies in the implementations, as long as the generator is properly able to represent the bindings.
There was a problem hiding this comment.
Glad to see that you're progressing here! I can't really help with the correctness of using the Metal API itself, am not experienced enough with it to do so, but I'll be happy to continue answering questions if you're using objc2-specific APIs that are confusing (this also goes for future PRs in this and other repos).
The current
objccrate stack is completely unmaintained and has severely fallen out of date with no updates for over 4 years. Themetal-rscrate, built on top of this architecture, is completely written by hand which is tedious to keep up-to-date, not to mention has inconsistencies in its implementation.All of this is superseded by the new
objc2crate stack built by @madsmtm. Beyond providing what seems like a better, safer abstraction over Objective-C, all bindings are completely autogenerated meaning we'll no longer lag behind upstream bindings (requiring painstaking manual patching) or have inconsistencies in the implementations, as long as the generator is properly able to represent the bindings.This PR is a draft to allow me to get a better understanding of Objective-C, as well as for @madsmtm to chime in on the current use of Metal bindings. Who also helpfully provided me a branch with planned future improvements, including changing the generated bindings to be closer to
metal-rsto make other migrations easier to manage.One important thing that is missing from this PR is interop with older
metal-rstypes.objc2(-metal)has the typicalId::from_raw()interop that could allow us to upgradegpu-allocatorhere while still usingmetal-rsfor a little while longer in our own codebase. Such interop could be provided like thepublic-winapioptional feature, or hand-rolled in our codebase if so desired.