Skip to content

Reduce the genericity of many closures#673

Merged
bors[bot] merged 6 commits into
rayon-rs:masterfrom
cuviper:generic-closures
Aug 15, 2019
Merged

Reduce the genericity of many closures#673
bors[bot] merged 6 commits into
rayon-rs:masterfrom
cuviper:generic-closures

Conversation

@cuviper

@cuviper cuviper commented Jul 5, 2019

Copy link
Copy Markdown
Member

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

cuviper commented Jul 5, 2019

Copy link
Copy Markdown
Member Author

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.

@nikomatsakis nikomatsakis left a comment

Copy link
Copy Markdown
Member

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 cuviper requested a review from nikomatsakis July 11, 2019 18:07
@cuviper

cuviper commented Jul 11, 2019

Copy link
Copy Markdown
Member Author

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
Copy Markdown
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
Copy Markdown
Member

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

@cuviper

cuviper commented Aug 14, 2019

Copy link
Copy Markdown
Member Author

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

cuviper commented Aug 15, 2019

Copy link
Copy Markdown
Member Author

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

bors Bot commented Aug 15, 2019

Copy link
Copy Markdown
Contributor

@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.

2 participants