Skip to content

[DO NOT MERGE] Alternative of #1699, option (C) in #1626 #1700

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

kenoss
Copy link
Contributor

@kenoss kenoss commented Mar 30, 2025

This shows how (C) in #1626 looks like.

.render_elements(renderer, location, scale, alpha)
.into_iter()
.map(SpaceRenderElements::Surface)
.collect::<Vec<_>>(),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still we need .collect::<Vec<_>>() for monomorphization; we need a common type with the arm below.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need a Vec and eagerly evaluate the vector.

We could either

  • Box it (still an allocation)
  • create a dispatching enum, which imo the macro should just create?

Copy link
Contributor Author

@kenoss kenoss Apr 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reg. enum

create a dispatching enum, which imo the macro should just create?

This is one of what either crate provides (doc).

I thought generalizing it won't work well. Consider the case that there are, say, five types. For example, 1. The enum that implements AsRenderElements has five arms; or 2. there are four .chain() calls and early returns. We have to return different types and a dispatching enum needs lots of variants. A problem is API. In Either, a user specifies variants explicitly.

return Either::Left(std::iter::empty());
...
return Either::Right(vec![1, 2, 3, 4, 5]);

So, we need something like this:

return CushioningMaterial::Variant0(...);
return CushioningMaterial::Variant1(...);
return CushioningMaterial::Variant2(...);
...

It's feasible to write a proc macro that helps this pattern, but I feel it's not a good idea as it is too magical and hinders user's intuition.

Note: Here's (one of) my (bad) idea:

#[cushioning_material::auto_convert_return]
fn render_elements() -> ... {
    ...
    // Automatically convert it to `return CushioningMaterial::Variant0(...);`.
    return std::iter::empty();
    ...
    // Automatically convert it to `return CushioningMaterial::Variant1(...);`.
    return ;
    ...
    // Or, alternatively, we can use explicit marker. (I guess it's unavoidable as we have a pattern that doesn't encourage `return`.)
    match ... {
        ... => {
            #[foo]
            ...
        }
        ... => {
            #[foo]
            ...
        }
    }
}

Reg. Box

I think this type of discussion should be based on a benchmark, but here's my opinion: Using Box<dyn IntoIterator<...>> or Box<dys Iterator<...>> introduces vtable calls (.next()) per render element per frame. I feel .collect<Vec<_>>() sohuld be cheaper, as the cost of it is memcpy, roughly. (Ignoring map etc. as it is common in these options.)

Let me know if you have a way of benchimarking.

.render_elements(renderer, location.to_physical_precise_round(scale), scale, alpha)
// So, we need to collect.
.into_iter()
.collect::<Vec<_>>()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

troublesome point 2

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a move to the closure should fix this, given we collect afterwards anyway.

Copy link
Contributor Author

@kenoss kenoss Apr 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No.

error: captured variable cannot escape `FnMut` closure body
   --> src/desktop/space/mod.rs:421:17
    |
419 |               .flat_map(move |e: &InnerElement<E>| {
    |                                                  - inferred to be a `FnMut` closure
420 |                   let location = e.render_location() - region.loc;
421 | /                 e.element
422 | |                     .render_elements(renderer, location.to_physical_precise_round(scale), scale, alpha)
    | |_______________________________________________________________________________________________________^ returns a reference to a captured variable which escapes the closure body
    |
    = note: `FnMut` closures only have access to their captured variables while they are executing...
    = note: ...therefore, they cannot allow references to captured variables to escape

The followings are my understanding.

  1. T has a member xs and implements AsRenderElements, by returning self.xs.iter().map(|x| ...). Iter grabs self.xs, Map can grab other members. So, in general, opaque impl IntoIterator/impl Iterator implicitly grab references to arguments. (Like + use<...> in this PR.)
  2. renderer: &mut R is caputuerd to the FnMut. This FnMut can use renderer as lifetime 'b, which is the scope of this FnMut. (While renderer outlives outer collect().)
  3. 1 and 2 contradict.

Note that I guess this code can be fixed by using bare for loop.

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

Successfully merging this pull request may close these issues.

2 participants