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

Reduce the genericity of many closures #673

Merged
merged 6 commits into from Aug 15, 2019

Conversation

cuviper
Copy link
Member

@cuviper cuviper commented Jul 5, 2019

The take_while closure only needs to be generic in T, along with a
captured &AtomicBool. When we write the closure directly, it carries
all the type baggage of WhileSomeFolder<'f, C>::consumer_iter<I>,
where the monomorphized type may explode. Instead, we can move this
closure to a standalone function for reduced genericity.

@cuviper
Copy link
Member Author

cuviper commented Jul 5, 2019

This fixes the type-length-limit of the specific example in #671, but I'm going to audit for similar cases before we call that fixed. So far, this only takes the max type in that example from ~1.4M characters to ~700K, sneaking under the compiler's default 1M limit, but that's still way too close for comfort.

Copy link
Member

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

annoying that it would be necessary, but I think the approach seems solid

@cuviper cuviper marked this pull request as ready for review July 11, 2019 18:07
@cuviper
Copy link
Member Author

cuviper commented Jul 11, 2019

OK, I think we're good now. I swept through pretty much everything, isolating closures wherever I saw there was a type parameter that could be avoided. This is not meant to be an aversion to closures entirely, just where they are implicitly overly generic.

move |_| f()
}

join_context(call(oper_a), call(oper_b))
Copy link
Member Author

Choose a reason for hiding this comment

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

To call out one example -- before, both of those closure wrappers depended on all 4 parameters, <A, B, RA, RB>, which meant that the join_context type also carried that duplication. Now the call wrappers only depend on their respective types, <A, RA> or <B, RB>.

@cuviper cuviper changed the title Reduce the genericity of WhileSomeFolder::consume_iter's closure Reduce the genericity of many closures Jul 11, 2019
@nikomatsakis
Copy link
Member

I'd be happy to r+ this, but @cuviper did you want to wait until the corresponding PR on rust-lang landed?

@cuviper
Copy link
Member Author

cuviper commented Aug 14, 2019

I was ready to wait, but we don't necessarily need to. I was more concerned whether upstream rust thought this approach was reasonable, and since it was approved, that's good enough here.

Looks like I have a conflict to resolve here though -- will fix.

The `take_while` closure only needs to be generic in `T`, along with a
captured `&AtomicBool`. When we write the closure directly, it carries
all the type baggage of `WhileSomeFolder<'f, C>::consumer_iter<I>`,
where the monomorphized type may explode. Instead, we can move this
closure to a standalone function for reduced genericity.
Most of these closures only need to be generic in the item type, but
when created in the context of some `I` generic iterator, they inherit
that genericity. This affects both type length and code duplication.
@cuviper
Copy link
Member Author

cuviper commented Aug 15, 2019

The rust PR is now merged, so the waiting question is now moot. 😄

bors r=nikomatsakis

bors bot added a commit that referenced this pull request Aug 15, 2019
673: Reduce the genericity of many closures r=nikomatsakis a=cuviper

The `take_while` closure only needs to be generic in `T`, along with a
captured `&AtomicBool`. When we write the closure directly, it carries
all the type baggage of `WhileSomeFolder<'f, C>::consumer_iter<I>`,
where the monomorphized type may explode. Instead, we can move this
closure to a standalone function for reduced genericity.

Co-authored-by: Josh Stone <cuviper@gmail.com>
@bors
Copy link
Contributor

bors bot commented Aug 15, 2019

@bors bors bot merged commit dbc114f into rayon-rs:master Aug 15, 2019
@cuviper cuviper deleted the generic-closures branch March 11, 2020 23:49
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.

None yet

2 participants