About OomError

[tomaka]

Most functions in Vulkan can return VK_ERROR_OUT_OF_HOST_MEMORY and VK_ERROR_OUT_OF_DEVICE_MEMORY. In fact most functions can only return one of these two errors.

This is reflected in vulkano by the fact that a lot of functions can return OomError.

However in practice it is very unlikely that an out-of-memory error actually happens, with the exception of vkAllocateMemory.

The API would be much more convenient to use if an out of memory error returned by Vulkan resulted in a panic instead of returning an error, like it's the case for the core Rust. Again, with the exception of DeviceMemory which would still return OomError.

[tomaka]

(this issue is for discussion about the problem, not something that is currently planned to be done)

[Mixthos]

If a the device memory is filled with cached textures/meshes and an important command returns VK_ERROR_OUT_OF_DEVICE_MEMORY, you would probably want to release some textures/meshes and repeat the command. Is this something that vulkano could handle?

[tomaka]

@Mixthos Currently yes, but if OomError is turned into panicking like this issue suggests, then no. The reasoning is that the chance that anything returns OUT_OF_DEVICE_MEMORY are very low if you except the commands that allocate memory.

For example why would creating a fence or a semaphore return out of memory? A fence or a semaphore is usually just a boolean. Almost all commands can return an out of memory error, but for most of these it would be surprising if they did.

[Mixthos]

I guess it depends on what the drivers do. Maybe we could test this for each function before changing it? Preferably in most drivers, in case one of the drivers has some weird behaviour.

[tomaka]

Well, drivers are more or less non-existing today. The ones that exist don't do many optimizations. We can't assume that if it works today it will likely work tomorrow.

[Mixthos]

The only added convenience is not having to call unwrap(), right? In that case I wouldn't want to give up the ability to recover from an error in case it does happen at some point in time. Otherwise there's no easy way to implement a patch to the application if it crashes on a new driver.

[Mixthos]

Or there could be two versions of each of these functions. For Example:

fn newSafe(device: &Arc<Device>) -> Result<Arc<Fence>, OomError>;
fn new(device: &Arc<Device>) -> Arc<Fence> {
    Fence::newSafe(device).unwrap()
}

[tomaka]

That looks like a good idea. This would be similar to the difference between [] and .get() in the stdlib for example.

[Mixthos]

If you tell me the naming convention you want to use and which functions should be left out, I could implement that and let you work on the harder stuff. ;)

[tomaka]

I thought something like this:

pub fn raw(device: &Arc<Device>) -> Result<Foo, OomError> { ... }

#[inline]
pub fn new(device: &Arc<Device>) -> Arc<Foo> {
    Arc::new(Foo::new(device).unwrap())
}

This way the raw function would be the lowest-level thing possible, not using Arc and returning all possible errors.

For objects that can return an error other than just OOM, I think we can keep the OOM error part of the possible errors. For example I think we can keep OomError inside SamplerCreationError.

If you tell me the naming convention you want to use and which functions should be left out, I could implement that and let you work on the harder stuff. ;)

That would be nice :) However you should keep in mind that some constructors that currently return OomError may return something else in the future. It may be tricky to figure out which ones by yourself.

[Mixthos]

Just to be sure I got it right before I do the rest:

    /// Creates a new pool.
    ///
    /// The command buffers created with this pool can only be executed on queues of the given
    /// family.
    ///
    /// # Panic
    ///
    /// Panicks if the queue family doesn't belong to the same physical device as `device`.
    ///
    #[inline]
    pub fn raw(device: &Arc<Device>, queue_family: &QueueFamily)
               -> Result<CommandBufferPool, OomError>
    {
        assert_eq!(device.physical_device().internal_object(),
                   queue_family.physical_device().internal_object());

        let vk = device.pointers();

        let pool = unsafe {
            let infos = vk::CommandPoolCreateInfo {
                sType: vk::STRUCTURE_TYPE_COMMAND_POOL_CREATE_INFO,
                pNext: ptr::null(),
                flags: 0,       // TODO: 
                queueFamilyIndex: queue_family.id(),
            };

            let mut output = mem::uninitialized();
            try!(check_errors(vk.CreateCommandPool(device.internal_object(), &infos,
                                                   ptr::null(), &mut output)));
            output
        };

        Ok(CommandBufferPool {
            pool: Mutex::new(pool),
            device: device.clone(),
            queue_family_index: queue_family.id(),
        })
    }

    /// Creates a new pool.
    ///
    /// The command buffers created with this pool can only be executed on queues of the given
    /// family.
    ///
    /// # Panic
    ///
    /// - Panicks if the queue family doesn't belong to the same physical device as `device`.
    /// - Panicks if the device or host ran out of memory.
    ///
    #[inline]
    pub fn new(device: &Arc<Device>, queue_family: &QueueFamily)
               -> Arc<CommandBufferPool>
    {
        Arc::new(CommandBufferPool::raw(device, queue_family).unwrap())
    }

Is there a reason the original constructor is inlined and should I undo it?

[Mixthos]

Also: CommandBufferPool is named "Command Pool" in the spec. I think vulkano should use the same names as the spec when there isn't a good reason not to.

[tomaka]

@Mixthos Your gist looks good. I usually inline functions that I think are short. Here it's really ambiguous.

Could you please open another issue for that name problem, so that I don't forget? The command buffer stuff needs to be redone anyway, so it's not worth changing now.

[Mixthos]

I opened an issue for the name. That function doesn't look short to me. ;)

[Mixthos]

Should I do the same for CustomRenderPass? The macro confuses me. ;) Also, I'm leaving out the structs with Unsafe in front of their name for now. Should I add the raw constructor to them as well? EDIT: I left out PipelineCache as well. I'm not sure about the memory allocation in that.

[tomaka]

Should I do the same for CustomRenderPass?

Yes. If it's too hard, it's not a problem.

Should I add the raw constructor to them as well?

Yes, why not.

[Mixthos]

Ok, I'll do that. I noticed that Surface::is_supported is returning the wrong error type. (vkGetPhysicalDeviceSurfaceSupportKHR can return VK_ERROR_SURFACE_LOST_KHR as well). Should I add a comment for that like you did for Surface::get_capabilities?

EDIT: I opened an issue for that instead.

[Mixthos]

I'm assuming vkBindImageMemory and vkBindBufferMemory shouldn't allocate memory. If I'm wrong, tell me, and I'll revert my changes to bind_memory. EDIT: Same for vkCreateImageView.

[Mixthos]

What should I call the original version of SwapchainImage::from_raw? from_raw_raw sounds weird. ;)

[Mixthos]

I created a pull request: #32.

[tomaka]

SwapchainImage::from_raw doesn't need a raw/non-raw version for now.

Swapchain::new() must return an Arc to the swapchain for technical reasons. Therefore it doesn't make sense to provide a function that returns an Arc<Swapchain> but non-Arc SwapchainImages. Therefore it's no use creating non-Arc SwapchainImages.

[Mixthos]

This way the raw function would be the lowest-level thing possible, not using Arc and returning all possible errors.

Out of curiosity, what is the reason for automatically wrapping all objects in an Arc? Couldn't Vulkano just use references and let the engine that uses Vulkano wrap it in an Arc when that engine needs to share it across multiple threads?

EDIT: Something like this seems cleaner:

struct Instance {
}

impl Instance {
    pub fn new() -> Instance {
        Instance {
        }
    }
}

struct Device<'a> {
    instance: &'a Instance,
}

impl<'a> Device<'a> {
    pub fn new(instance: &'a Instance) -> Device<'a> {
        Device {
            instance: instance,
        }
    }
}

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

impl<'a> CommandPool<'a> {
    pub fn new(device: &'a Device<'a>) -> CommandPool<'a> {
        CommandPool {
            device: device,
        }
    }
}

struct CommandBuffer<'a> {
    pool: &'a CommandPool<'a>,
}

impl<'a> CommandBuffer<'a> {
    pub fn new(pool: &'a CommandPool<'a>) -> CommandBuffer<'a> {
        CommandBuffer {
            pool: pool,
        }
    }
}

[tomaka]

Something like this seems cleaner:

That's a usability problem with Rust. I had already talked about it back in 2015 in a gist (which I don't have time to find right now).

I'm already using vulkano in a real project, and here is an example code that I have:

pub struct BatchSystem<R> {
    /// Queue used to transfer buffers to textures.
    transfer_queue: Arc<Queue>,

    /// Queue used to draw sprites.
    gfx_queue: Arc<Queue>,

    /// Pool for the various command buffers used in this system.
    cb_pool: Arc<CommandBufferPool>,

    /// The graphics pipelines used to draw sprites.
    pipeline_no_depth: Arc<GraphicsPipeline<SingleBufferDefinition<SpriteVertex>,
                                            pipeline_layout::CustomPipeline, R>>,
    pipeline_depth: Arc<GraphicsPipeline<SingleBufferDefinition<SpriteVertex>,
                                         pipeline_layout::CustomPipeline, R>>,

    render_pass: Arc<R>,
    render_pass_subpass: u32,

    /// Texture.
    texture: Arc<ImmutableImage<format::R8G8B8A8Srgb>>,

    /// Descriptor set
    descriptor_set: Arc<pipeline_layout::set0::Set>,

    /// List of queue submissions that are pending execution.
    queue_submissions: Arc<Mutex<Vec<Arc<Submission>>>>,

    .. other fields...
}

Now imagine writing the same thing with lifetimes. Since you can't put the "dependee" and the "depender" in the same struct, you would have to split this BatchSystem in at least 5 different structs that depend on each other.

If then another type named Foo wants to use BatchSystem internally, it would have to split in 5 different structs as well. Then if another type wants to use Foo internally, it would have to split in 5 different structs as well. This would make the whole program unusable.

Another point is that you can notice that some types are absent from the BatchSystem struct. For example I have a descriptor set and no descriptor pool. This is possible because the descriptor set keeps the descriptor pool alive with an Arc. Something like this wouldn't be possible with lifetimes. Some "systems" can even consist in only a command buffer that is submitted at each frame, and that's very convenient.

[tomaka]

I also started making some structs generic over the kind of references that they need, like Fence or #29. This way the type of the pointer is the same as the thing you pass when you called ::new(), including references, and if you put the object in a struct without explicitly precising a type, it would default to Arc.

However there's a safety problem caused by the fact that a Deref implementation could return a different object every time, forcing the types to always check that the object is correct.

I'm hesitating between continuing this way or providing a custom Deref-like trait that would be unsafe and that would be forced to return the same object every time.

[jeltz]

Here is the gist if anyone is curious.

[Mixthos]

Ok. This seems like something that should be added to Rust in the future if somebody can figure out a way do do it without screwing up the type system.

Here is the gist if anyone is curious.

Thanks!

[Mixthos]

This is possible:

use std::mem;

struct Instance {
}

impl Instance {
    pub fn new() -> Instance {
        Instance {}
    }
}

struct Device<'a> {
    instance: &'a Instance,
}

impl<'a> Device<'a> {
    pub fn new(instance: &'a Instance) -> Device<'a> {
        Device { instance: instance }
    }
}

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

impl<'a> CommandPool<'a> {
    pub fn new(device: &'a Device<'a>) -> CommandPool<'a> {
        CommandPool { device: device }
    }
}

struct CommandBuffer<'a> {
    pool: &'a CommandPool<'a>,
}

impl<'a> CommandBuffer<'a> {
    pub fn new(pool: &'a CommandPool<'a>) -> CommandBuffer<'a> {
        CommandBuffer { pool: pool }
    }
}

struct Assets<'a> {
    instance: Instance,
    device: Device<'a>,
    pool: CommandPool<'a>,
    buffer: CommandBuffer<'a>,
}

fn main() {
    let mut assets = unsafe { mem::uninitialized::<Assets>() };

    assets.instance = Instance::new();
    assets.device = Device::new(&assets.instance);
    assets.pool = CommandPool::new(&assets.device);
    assets.buffer = CommandBuffer::new(&assets.pool);
}

[tomaka]

But you can't move it: http://is.gd/q5IFK6

[Mixthos]

Is using a Box okay? http://is.gd/4f2pyP

[tomaka]

Well, that's unsafe.

[Mixthos]

The same thing with an Arc: http://is.gd/9bWfsL I don't think it's too bad. If the macro is used properly the only unsafe part is the instantiation. Maybe there are ways to make it safer?

[tomaka]

I don't really understand. If the plan is to put this macro in vulkano, then you're blindly assuming that the user just wants an instance, a device, a pool and a command buffer. If the plan is to put this macro in the user's code, then they have to write unsafe code, which is bad.

[Mixthos]

The macro is generic and could be put into vulkano. The struct I used was just an example.

Your example could look like this:

pub struct BatchSystem<'a, R: 'a> {
    /// Queue used to transfer buffers to textures.
    transfer_queue: Queue<'a>,

    /// Queue used to draw sprites.
    gfx_queue: Queue<'a>,

    /// Pool for the various command buffers used in this system.
    cb_pool: CommandBufferPool<'a>,

    /// The graphics pipelines used to draw sprites.
    pipeline_no_depth: GraphicsPipeline<'a, SingleBufferDefinition<SpriteVertex>,
                                            pipeline_layout::CustomPipeline, R>,
    pipeline_depth: GraphicsPipeline<'a SingleBufferDefinition<SpriteVertex>,
                                         pipeline_layout::CustomPipeline, R>,

    render_pass: R,
    render_pass_subpass: u32,

    /// Texture.
    texture: ImmutableImage<'a, format::R8G8B8A8Srgb>,

    /// Descriptor set
    descriptor_set: pipeline_layout::set0::Set<'a>,

    /// List of queue submissions that are pending execution.
    //queue_submissions: Arc<Mutex<Vec<Arc<Submission>>>>,
}

// ...

fn main() {
    // ...
    let batch_system = make_assets!(BatchSystem<'a, R> {
        transfer_queue: create_transfer_queue(/*...*/),
        gfx_queue: create_gfx_queue(/*...*/),
        cb_pool: create_cb_pool(/* ... */),
        pipeline_no_depth: create_pipeline_no_depth(/* ... */),
        pipeline_depth: create_pipeline_depth(/* ... */),
        render_pass: create_render_pass(/* ... */),
        render_pass_subpass: create_render_pass_subpass(/* ... */),
        texture: create_texture(/* ... */),
        descriptor_set: create_descriptor_set(/* ... */),
    });
}

Each created value can be borrowed to create the ones after it.

[tomaka]

But isn't that totally out of scope of vulkano? As I said I started rewriting some structs to be generic over Deref<Target = Foo> instead of using Arc<Foo> directly.

Once that's done, the user can get that macro from another crate.

[Mixthos]

But isn't that totally out of scope of vulkano?

Probably.

[Mixthos]

Once that's done, the user can get that macro from another crate.

I created a crate for it: self-ref It's safer than the version I posted here (but works only in Rust 1.9 and up).

[tomaka]

I guess I'm going to close this.