Simpler slice Iterator methods

[nnethercote]

These reduce the amount of LLVM IR generated, helping compile times.

r? @cuviper

[nnethercote]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion

[bors]

⌛ Trying commit 3b10858 with merge 7f1659b19bed7255f86fa296721f74e5f107f9ef...

[bors]

☀️ Try build successful - checks-actions, checks-azure
Build commit: 7f1659b19bed7255f86fa296721f74e5f107f9ef (7f1659b19bed7255f86fa296721f74e5f107f9ef)

[rust-timer]

Queued 7f1659b19bed7255f86fa296721f74e5f107f9ef with parent a2e0b48, future comparison URL.

[rust-timer]

Finished benchmarking try commit 7f1659b19bed7255f86fa296721f74e5f107f9ef, comparison URL.

[andjo403]

nice perf results
but some questions that I started thinking of:

• is this common enough to grant special handling only 5 of 114 benchmarks with more than 1% so maybe not?
• when to stop with manual inlining the next() function is also called and probably inlined in all this functions and if that is inlined by hand shall the next level of functions be inlined also?
• when/if mir inlining is merged will someone remember to remove this manual inlining if that gives no extra compile time?

[nnethercote]

* is this common enough to grant special handling only 5 of 114 benchmarks with more than 1% so maybe not?

It's a huge win for clap-rs. It's a sizeable win for a few other benchmarks. And it's a tiny win for a lot of benchmarks. Plenty of compile time optimizations that land only affect a few benchmarks.

* when to stop with manual inlining the next() function is also called and probably inlined in all this functions and if that is inlined by hand shall the next level of functions be inlined also?

The exact inlining is up to LLVM. The methods in Iterator are also marked with #[inline] so this is not much of a change.

* when/if mir inlining is merged will someone remember to remove this manual inlining if that gives no extra compile time?

Maybe, maybe not. But if these functions are unnecessary, the cost of them remaining is small.

In general, many optimizations for compile time involve trade-offs, adding complexity for speed. This one is no exception, but I feel that the extra complexity in this case is small relative to the speed improvements.

Overall, I feel this optimization scores quite well on the speed-ups vs. complexity trade-off.

[andjo403]

• when to stop with manual inlining the next() function is also called and probably inlined in all this functions and if that is inlined by hand shall the next level of functions be inlined also?

The exact inlining is up to LLVM. The methods in Iterator are also marked with #[inline] so this is not much of a change.

what I was trying to ask was. as this change have inlined try_for in to this functions so this have already taken the decision away from LLVM so why stop there.
eg. there is already macros used maybe have a macro that do what next() is doing.

only wondering if this will open the floodgate for PRs that changes inline functions to macros.

also feel that there is a difference if it is a compiler change that makes a win for a few benchmarks or if it is a change in the std lib.

but I agree overall this PR is not making the code more complex and there is good compile time speedups nice work as always and thanks for the answers.

[nnethercote]

I thought about doing a similar thing in other iterators, but can't see any suitable candidates. try_fold gets used reasonably often for Filter and Map, e.g. if you have x.iter().filter(...).any(). But we want to call try_fold on the iterator within Filter in that case, to maximize optimization opportunities, so we can't do a simple loop-based implementation for Filter::any. Unless I'm overlooking something.

[nnethercote]

cc @bluss

[cuviper]

Looks good!

@bors r+

[bors]

📌 Commit 3b10858 has been approved by cuviper

[nnethercote]

Final perf results are here. This landed as part of a rollup because I forgot to mark it with rollup=never, but those results look very similar to the results we saw above.