Simplify AsRenderElements::render_elements<C>()

[kenoss]

The current definition (A) is the following:

pub trait AsRenderElements<R>
where
    R: Renderer,
{
    /// Type of the render element
    type RenderElement: RenderElement<R>;
    /// Returns render elements for a given position and scale
    fn render_elements<C: From<Self::RenderElement>>(
        &self,
        renderer: &mut R,
        location: Point<i32, Physical>,
        scale: Scale<f64>,
        alpha: f32,
    ) -> Vec<C>;
}

IIUC, the intention of the parameter is reducing memcpying Vec. Consider the following case:

// OutputDamageTracker::render_output_with() is called on this.
enum OutputRenderElements<R> {
   Foo(FooRenderElements<R>),
   Bar(BarRenderElements<R>),
}

impl AsRenderElements for Output {
    type RenderElement: OutputRenderElement<R>;

    fn render_elements<C: From<Self::RenderElement>>(...) -> Vec<C> {
        match self.0 {
           Foo(foo) => foo.render_elements(...).into_iter().map(C::from).collect::<Vec<_>>() // (1)
           Bar(bar) => bar.render_elements(...).into_iter().map(C::from).collect::<Vec<_>>()
        }
    }
}

enum FooRenderElements<R> {
   Hoge(HogeRenderElements<R>),
   Fuga(FugaRenderElements<R>),
}

impl AsRenderElements for Foo {
    type RenderElement: FooRenderElement<R>;

    fn render_elements<C: From<Self::RenderElement>>(...) -> Vec<C> {
        match self.0 {
           Hoge(hoge) => hoge.render_elements(...).into_iter().map(C::from).collect::<Vec<_>>() // (2)

           Fuga(fuga) => fuga.render_elements(...).into_iter().map(C::from).collect::<Vec<_>>()
        }
    }
}

struct HogeRenderElements<R>(SolidColorRenderElement<R>);

impl AsRenderElements for Hoge {
    type RenderElement: HogeRenderElement<R>;

    fn render_elements<C: From<Self::RenderElement>>(...) -> Vec<C> {
        vec![HogeRenderElements(SolidColorRenderElement::from_buffer()).into()] // (3)
    }
}

Here, conversion is done at (3) (from HogeRenderElements to FooRenderElements::Hoge) and (2) (from FooRenderElements to OutputRenderElements::Hoge)
as <Output as AsRenderElements>::render_elements() is called with C = OutputRenderElements. But memcpying Vec might also occur in (1) as we use .into_iter().map(C::from).collect::<Vec<_>>().
("Might" means it might vanish with optimization.)

Consider the following trait definition (B) instead:

pub trait AsRenderElements<R>
where
    R: Renderer,
{
    /// Type of the render element
    type RenderElement: RenderElement<R>;
    /// Returns render elements for a given position and scale
    fn render_elements(
        &self,
        renderer: &mut R,
        location: Point<i32, Physical>,
        scale: Scale<f64>,
        alpha: f32,
    ) -> Vec<Self::RenderElement>;
}

Then, conversion is done at (2), and (1). memcpying is done at (3), (2), (1).

But, in real applications, we can't expect the reduce.
For example, OutputRenderElements is constructed with multiple sources in Anvil [code].

We can consider yet another definition (C):

pub trait AsRenderElements<R>
where
    R: Renderer,
{
    /// Type of the render element
    type RenderElement: RenderElement<R>;
    /// Returns render elements for a given position and scale
    fn render_elements(
        &self,
        renderer: &mut R,
        location: Point<i32, Physical>,
        scale: Scale<f64>,
        alpha: f32,
    ) -> impl IntoIter<Item = Self::RenderElement>;
}

The chaining pattern like xs.into_iter().map(|x| x.into()).chain(ys.into_iter().map(|y| y.into())) is very common like the above example in applications built on top of smithay.
This should make code very simple and intuitive.

I prefer (C) > (B) > (A). WDYT? Let me know if you know the reason why the current implementation is using (A).

[Drakulix]

I am pretty sure impl Trait return types in traits didn't work, when this trait was introduced. Looking this up now, it seems like it should desugar into an "anonymous associated type", which sounds exactly like what we need (and what was previously impossible). Given we relatively recently bumped our MSRV, it might be, that we can make use of this feature now. Do you happen to know when this was stabilized?

@cmeissl I personally always envisioned the trait to look like (C), any obvious concerns with that approach?

[cmeissl]

No concern, actually I really wanted to optimize this part as it has shown up in the performance profiles. So, I would be glad if (C) work finally work!

[cmeissl]

Looks like return-position impl Trait in traits were part of 1.75, but with some limitations. We just recently bumped the MSRV from 1.72 to 1.80.1, so yeah, this was just not possible before.

[kenoss]

Thanks!

Ok then, I'll try to rewrite it. (I'll first update smithay to HEAD in my compositor. It'll take some time.)

[zerosign]

Hi all, (sorry for bumping old thread)

I wonder what if rather than returning a Vec<C>, let just let the caller provide a Vec<C> instead ? (maybe as another Trait?)

pub trait PopulateRenderElements<R>
where
    R: Renderer,
{
    /// Type of the render element
    type RenderElement: RenderElement<R>;

    /// Populate render elements for a given position and scale into storage S
    fn populate_elements<C: From<Self::RenderElement>, S>(
        &self,
        storage: &mut S,
        renderer: &mut R,
        location: Point<i32, Physical>,
        scale: Scale<f64>,
        alpha: f32,
    ) where S: Extend<C>;
}

It lets the implementation to just reuse storage over and over into child of elements. I've seen that in some compositor, it call over Vec::new() over and over again on render_elements and mostly its being called in their render() path (which probably hotpaths).

Thanks!