gfx-rs / gfx

[maintenance mode] A low-overhead Vulkan-like GPU API for Rust.
http://gfx-rs.github.io/
Apache License 2.0
5.35k stars 549 forks source link

Don't use Borrow with lifetimes in the API #3569

Closed kvark closed 3 years ago

kvark commented 3 years ago

We have a lot of patterns like this:

where
        T: IntoIterator,
        T::Item: Borrow<hal::memory::Barrier<'a, Backend>>,

Where the borrowed type is defined as:

pub enum Barrier<'a, B: Backend> {
    Buffer {
        target: &'a B::Buffer,
        ...
    },
    Image {
        target: &'a B::Image,
        ...
    },
    ...
}

The main motivation for Borrow was to accommodate for cases where the user has an iterator containing our data, but Rust doesn't allow us to go from Iterator<Item=T> to Iterator<Item=&T>, unfortunately.

I think there is a bit of contradiction here, since there is Borrow, but still we require &'a B::Something inside. So this would only work for cases where the user has an iterator over something that contains Barrier to start with. This seems like a weird use case. It's more likely that the user stores B::Buffer and B::Image in some structures that are iterated over, and the user constructs Barrier on the fly. In this case the Borrow bound doesn't help at all.

Suggestion: remove Borrow from all the API where items depend on 'a. Instead, we can work with just IntoIterator<Item = Something>.

kvark commented 3 years ago

Got another idea. If the user has an iterator over something that contains B::Buffer or B::Image, then they would need something like this on the API boundary:

enum Barrier<Y, U> {
    Buffer {
        target: Y,
        ...
    },
    Image {
        target: U,
        ...
    },
    ...
}

And then in the function declaration we'd have:

where
        T: Borrow<B::Buffer>,
        Y: Borrow<B::Image>,
        T: IntoIterator<Item = Barrier<T, Y>>,

So that would be one way to proceed without reducing generality. Frankly, I'd love to simplify our function signatures. We may need to check with all the important users (basically, wgpu, webrender, and gfx-portability, I don't think Amethyst/Rendy is maintained) and see where the generality makes sense at all.

grovesNL commented 3 years ago

:+1: this sounds ok to me, assuming it works well with the main users as mentioned

kvark commented 3 years ago

Hmm, it doesn't look (so far) that either gfx-portability or wgpu-core would benefit from that last suggestion. Barriers often need to be chained to each other, and chaining breaks ExactSizeIterator. So things are getting constructed somewhere, anyway :/

kvark commented 3 years ago

Hmm, I'm getting a bit stuck here. It's easy to remove the Borrow bounds, but it's hard to shape the whole API in a consistent way that solves problems we faced previously. I.e. it's hard to draw the line between "this is a structure with a lifetime" and "this is an object reference under Borrow". For example, we have:

Here is one of the use cases our general approach is meant to support: I have an iterator over (Arc<T>, Something), and I need to provide it to gfx, which essentially needs an iterator over (&'a T, Something). We can't borrow things in the iterator, but we can have Arc<T>: Borrow bound. So it sounds like my second suggestion here - https://github.com/gfx-rs/gfx/issues/3569#issuecomment-757973753 would allow our API to be more consistent.

kvark commented 3 years ago

I'm think of just abandoning Borrow bounds completely and consistently...

As radical as it seems, I suspect them to play a major role in the compile times. Building hal is what we timed previously, but no backend code actually gets produced/instantiated. And with so much generics on the API we are doomed to have the complex functions monomorphised multiple times depending on what the client does. It's a compile time hazard.

Besides, now that we added ExactSizeIterator bounds, even if the client has only an iterator, it has to be exact size as well. And that means they can always allocate space with inplace_it on the stack if they want to, as a last resort.