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 closures in the iterator traits #62429

Merged
merged 24 commits into from
Aug 15, 2019

Conversation

cuviper
Copy link
Member

@cuviper cuviper commented Jul 6, 2019

By default, closures inherit the generic parameters of their scope,
including Self. However, in most cases, the closures used to implement
iterators don't need to be generic on the iterator type, only its Item
type. We can reduce this genericity by redirecting such closures through
local functions.

This does make the closures more cumbersome to write, but it will
hopefully reduce duplication in their monomorphizations, as well as
their related type lengths.

@rust-highfive
Copy link
Collaborator

r? @cramertj

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 6, 2019
@cuviper
Copy link
Member Author

cuviper commented Jul 6, 2019

For some context, rayon-rs/rayon#671 reported that it is fairly easy to get parallel iterators to exceed the type length limit. I opened rayon-rs/rayon#673 to reduce genericity in a few specific cases, which seems to help. I decided it might be worthwhile to try this on the standard library too, if only so I could get some feedback on this approach.

I kind of wish the compiler could just figure this out, but that may be more difficult.

@bors
Copy link
Contributor

bors commented Jul 8, 2019

☔ The latest upstream changes (presumably #62473) made this pull request unmergeable. Please resolve the merge conflicts.

@cramertj
Copy link
Member

cramertj commented Jul 8, 2019

it will hopefully reduce duplication in their monomorphizations, as well as their related type lengths.

Have you had a chance to do any investigation to see if this benefit actually occurs, perhaps by looking at the generated LLVM IR or the resulting generated code? If so, consider adding a test for this to ensure that it isn't regressed in the future.

@cuviper
Copy link
Member Author

cuviper commented Jul 8, 2019

@scottmcm

Might be worth double-checking there's a test for overflow checks here? I'm never confident in what rustc_inherit_overflow_checks does. (And I hear that one can use Add::add(count, 1) instead of using the attribute.)

There are overflow tests for sum and RangeFrom::next in run-pass/iterators, but not for count. Overflowing a 64-bit count would take a while, but I think we can reasonably test 32-bit targets.

@cramertj

it will hopefully reduce duplication in their monomorphizations, as well as their related type lengths.

Have you had a chance to do any investigation to see if this benefit actually occurs, perhaps by looking at the generated LLVM IR or the resulting generated code? If so, consider adding a test for this to ensure that it isn't regressed in the future.

I verified manually beforehand that such duplication was happening, and that they weren't afterward. And I know in rayon-rs/rayon#673 that similar changes did reduce the type length. But yes, I can and should add codegen tests specifically for these aspects.

@cramertj
Copy link
Member

cramertj commented Jul 8, 2019

But yes, I can and should add codegen tests specifically for these aspects.

Great, thanks!

@cuviper
Copy link
Member Author

cuviper commented Jul 8, 2019

The deduplication is now tested in codegen.

I had a hard time figuring out how to demonstrate the type length as-is, but I was able to do so with Map folds and adding the optimizations for those closures too.

It's interesting though, because even if I crank up type_length_limit to let that new Map test pass unchanged, the giant types don't appear in LLVM IR at all. I suspect something in the compiler is producing temporary types that aren't actually needed, perhaps for type inference, method resolution, I don't know what. The excerpted type in the error message isn't useful, and I'm not sure I could dig through over a million characters of a type anyway...

If the Map changes look good, I can go through the other iterator adapters too...

@cuviper
Copy link
Member Author

cuviper commented Jul 12, 2019

As an external data point, I looked again at rayon#673 tests/issue671.rs. With rustc nightly, the lowest I could get type_length_limit was 245,000. With this PR I can go down to 36,000.

@Alexendoo
Copy link
Member

Ping from triage, any updates? @cramertj

@cramertj
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jul 24, 2019

📌 Commit bbb4e2e has been approved by cramertj

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 24, 2019
@Centril
Copy link
Contributor

Centril commented Jul 24, 2019

@bors rollup=never

@bors
Copy link
Contributor

bors commented Jul 27, 2019

⌛ Testing commit bbb4e2e with merge 06f646dcc71e856eb76f4f0dcc14ca10b267839a...

@bors
Copy link
Contributor

bors commented Jul 27, 2019

💥 Test timed out

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 27, 2019
@cuviper
Copy link
Member Author

cuviper commented Jul 30, 2019

The timed-out build included an LLVM rebuild -- I guess we can retry?

@bors retry

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 30, 2019
@RalfJung
Copy link
Member

@rust-lang/infra that notification doesn't make any sense -- Miri does not build, tests cannot possibly pass on Windows.

@flip1995
Copy link
Member

Same goes for Clippy

@timvermeulen
Copy link
Contributor

Is there an issue that tracks any attempts to improve how closures are compiled, such that changes like these aren't necessary anymore? It's effective, but it's a lot of boilerplate code...

I'm also wondering whether a macro could give the same upsides without the visual clutter of turning all closures into functions. @cuviper Have you tried this, or thought about it?

@cuviper
Copy link
Member Author

cuviper commented Aug 16, 2019

@timvermeulen I don't have a good answer for either of those questions, and I agree it's a pain.

I suspect that improving how closures are compiled -- I guess inferring tighter generic parameters -- would be RFC territory. This would be sort of like RFC 2229 at the type level. If that's possible, I would be happy to see these changes go back to simpler closures again.

As for a macro, at least for macro_rules!, I'm not sure what this would look like. The clutter is all in repeating the types and bounds explicitly for the function, and I don't know how a macro would infer this. Do you have ideas?

One saving grace is that I don't think we need to turn this PR into a general "avoid closures" advice for Rust developers. I think iterators are unusual in this respect, and especially iterator adaptors, to have complicated generic types but closures that only operate on a simpler item type. Parallel iterators in rayon are similar, of course, and maybe futures should think about this too. But generally, I suspect most closures probably do need their full generic context.

@oli-obk oli-obk mentioned this pull request Aug 17, 2019
@oli-obk
Copy link
Contributor

oli-obk commented Aug 17, 2019

I don't think we need an RFC for this (at least not a language RFC, just an implementation one at best), since it doesn't actually change anything from a user perspective. I opened #63660

@eddyb
Copy link
Member

eddyb commented Aug 18, 2019

Isn't this #46477? We should revive those efforts and implement it as "polymorphic" codegen (still-generic over some parameters which aren't actually used), in a similar way to how "polymorphic" CTFE works with miri today.

@timvermeulen
Copy link
Contributor

#46477 was closed, does that mean this is no longer necessary?

@cuviper
Copy link
Member Author

cuviper commented Sep 16, 2020

The tail end of that bug was discussing additional work that would be needed to reduce I to I::Item, which is much of what this PR did. I don't know if there's been any follow-up in that direction.

@timvermeulen
Copy link
Contributor

Great, thank you.

@eddyb

This comment has been minimized.

@eddyb
Copy link
Member

eddyb commented Feb 3, 2021

I've hidden the comment now that I've opened #81721.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.