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

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

This comment has been minimized.

Copy link
Collaborator

commented Jul 6, 2019

r? @cramertj

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

@cuviper

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2019

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

@cuviper cuviper force-pushed the cuviper:iter-closures branch from 395d967 to 095b14d Jul 8, 2019

@cramertj

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link
Member

commented Jul 8, 2019

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

Great, thanks!

@cuviper

This comment has been minimized.

Copy link
Member Author

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 cuviper force-pushed the cuviper:iter-closures branch 2 times, most recently from b057ced to 7548974 Jul 11, 2019

@cuviper

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link
Member

commented Jul 24, 2019

Ping from triage, any updates? @cramertj

@cramertj

This comment has been minimized.

Copy link
Member

commented Jul 24, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2019

📌 Commit bbb4e2e has been approved by cramertj

@Centril

This comment has been minimized.

Copy link
Member

commented Jul 24, 2019

@bors rollup=never

@bors

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2019

⌛️ Testing commit bbb4e2e with merge 06f646d...

bors added a commit that referenced this pull request Jul 27, 2019

Auto merge of #62429 - cuviper:iter-closures, r=cramertj
Reduce the genericity of closures in the iterator traits

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

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2019

💥 Test timed out

@cuviper

This comment has been minimized.

Copy link
Member Author

commented Jul 30, 2019

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

@bors retry

@cuviper cuviper force-pushed the cuviper:iter-closures branch from bbb4e2e to c4189a0 Aug 12, 2019

@cuviper

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2019

Ah, it failed on Linux i686-gnu-nopt alone because of my new overflow tests with:

// only-32bit too impatient for 2⁶⁴ items

Those are OK on 32-bit optimized builds, but too slow on nopt with --disable-optimize-tests.

I've now forced optimizations in those specific tests.

@bors r=cramertj

@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

📌 Commit bca6f28 has been approved by cramertj

@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2019

⌛️ Testing commit bca6f28 with merge 1cdcea9...

bors added a commit that referenced this pull request Aug 15, 2019

Auto merge of #62429 - cuviper:iter-closures, r=cramertj
Reduce the genericity of closures in the iterator traits

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

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2019

☀️ Test successful - checks-azure
Approved by: cramertj
Pushing 1cdcea9 to master...

@bors bors added the merged-by-bors label Aug 15, 2019

@bors bors merged commit bca6f28 into rust-lang:master Aug 15, 2019

5 checks passed

homu Test successful
Details
pr Build #20190813.1 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (LinuxTools) LinuxTools succeeded
Details
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Aug 15, 2019

📣 Toolstate changed by #62429!

Tested on commit 1cdcea9.
Direct link to PR: #62429

🎉 clippy-driver on windows: build-fail → test-pass (cc @Manishearth @llogiq @mcarton @oli-obk @phansch, @rust-lang/infra).
🎉 miri on windows: build-fail → test-pass (cc @oli-obk @RalfJung @eddyb, @rust-lang/infra).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Aug 15, 2019

📣 Toolstate changed by rust-lang/rust#62429!
Tested on commit rust-lang/rust@1cdcea9.
Direct link to PR: <rust-lang/rust#62429>

🎉 clippy-driver on windows: build-fail → test-pass (cc @Manishearth @llogiq @mcarton @oli-obk @phansch, @rust-lang/infra).
🎉 miri on windows: build-fail → test-pass (cc @oli-obk @RalfJung @eddyb, @rust-lang/infra).
@lzutao lzutao referenced this pull request Aug 15, 2019
@RalfJung

This comment has been minimized.

Copy link
Member

commented Aug 15, 2019

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

@flip1995

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2019

Same goes for Clippy

@timvermeulen

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2019

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

This comment has been minimized.

Copy link
Member Author

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 referenced this pull request Aug 17, 2019
@oli-obk

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.