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

AshMemoryDevice could avoid unsafe code during construction #63

Open
i509VCB opened this issue Jun 18, 2022 · 6 comments
Open

AshMemoryDevice could avoid unsafe code during construction #63

i509VCB opened this issue Jun 18, 2022 · 6 comments

Comments

@i509VCB
Copy link
Contributor

i509VCB commented Jun 18, 2022

At the moment the following is done:

    #[repr(transparent)]
    pub struct AshMemoryDevice {
        device: Device,
    }
    
    impl AshMemoryDevice {
        pub fn wrap(device: &Device) -> &Self {
            unsafe {
                // Safe because `Self` is `repr(transparent)`
                // with only field being `DeviceLoader`.
                &*(device as *const Device as *const Self)
            }
        }
    }

This works but requires what I feel is unnecessary unsafe code.

I imagine we could change AshMemoryDevice to the following if we use a lifetime:

pub struct AshMemoryDevice<'a> {
    device: &'a Device,
}

impl AshMemoryDevice<'_> {
    pub fn wrap(device: &Device) -> Self {
        Self { device }
    }
}
@zakarumych
Copy link
Owner

I'm wouldn't like passing multilevel references everywhere.
So then all API must accept device by value and require Copy. Not sure API would look better that way.

@nerditation
Copy link
Contributor

if I understand it correctly, the reason there's the wrapper type AshMemoryDevice is due to the orphan rule; the wrapper type would be totally unneccessary if the "backend implementations" (e.g. gpu-alloc-ash) are not separate crates but instead sub-modules of the same crate, and selectable by cargo feature flags. that way, you could simply

impl crate::MemoryDevice<vk::DeviceMemory> for ash::Device{
//...
}

and use the ash::Device directly for alloc, dealloc, cleanup, etc

so I wonder? is there any plan for furture work toward this direction?


PS: to be honest, I feel it a little inconvenient having to write AshMemoryDevice::wrap(&device) a lot.

with the current implementations, I would suggest to make it more ergonomic to use , for instance, if we change the signature to:

pub unsafe fn cleanup<MD: MemoryDevice<M>>(&mut self, device: impl AsRef<MD>);

and besides AshMemoryDevice::wrap, we also add impl AsRef<AshMemoryDevice> for ash::Device {...},
then we can call the function like this:

// just a plain `ash::Device`, no need to manually wrap it
let device: ash::Device;
//...
// need this in scope so the so type inference could resolve
// or, explictly specify the type argument without import into scope
use gpu_alloc_ash::AshMemoryDevice
allocator.cleanup(&device);
allocator.cleanup::<gpu_alloc_ash::AshMemoryDevice>(&device);

@zakarumych
Copy link
Owner

The reason to have separate crates is flexibility in version updates.
Crate for each backend is updated with backend's nee version.
And main crate can be updated separately from them.

@zakarumych
Copy link
Owner

API ergonomics suggestions and PRs are welcome

@nerditation
Copy link
Contributor

The reason to have separate crates is flexibility in version updates.

right, that makes perfect sense

API ergonomics suggestions and PRs are welcome

ok, I think I can make a PR. it should be trivial to impelement, and I believe it should not break old code.

@nerditation
Copy link
Contributor

#65 is created.

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

No branches or pull requests

3 participants