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

Inconsistent inlineing of Iterator Adaptors - Missed Optimizations #47461

Open
jonasbb opened this issue Jan 15, 2018 · 6 comments
Open

Inconsistent inlineing of Iterator Adaptors - Missed Optimizations #47461

jonasbb opened this issue Jan 15, 2018 · 6 comments
Labels
A-iterators Area: Iterators C-enhancement Category: An issue proposing an enhancement or a PR with one. I-slow Issue: Problems and improvements with respect to performance of generated code. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@jonasbb
Copy link
Contributor

jonasbb commented Jan 15, 2018

While profiling some rust code of me, I noticed that the following pattern does not optimize well:

vec![1,2,3,4]
    .into_iter()
    .map(|v| ...)
    .skip_while(|v| ...)

skip_while is implemented using find and find is implemented using try_fold. The functions SkipWhile::next() and Iterator::find() use the #[inline] annotation. The function Map::try_fold() does not. This means that Map::try_fold() will not be inlined.

I started looking at the source code and inlineing of iterators seems to follow no rule. I could not find any bug reports related to this.

Some iterators like Cloned do not have any function marked as inline. Not even next() is marked as inline.

The PR introducing try_fold does not give justification why some try_folds are inline and some are not.

The methods len and is_empty of ExactSizeIterator's are also not marked as inlineable, even though they are always implemented as pass-through to the underlying iterator.

If desired I can prepare a pull request to mark those functions as inlineable. Is there a list of functions for the iterator traits (e.g., Iterator, ExactSizeIterator) which should be inline/not be inline?

@Mark-Simulacrum Mark-Simulacrum added the I-slow Issue: Problems and improvements with respect to performance of generated code. label Jan 15, 2018
@hanna-kruppe
Copy link
Contributor

hanna-kruppe commented Jan 15, 2018

Note that all iterator adaptors are generic and therefore instantiated in the crate using them, making them available for inlining in principle. Adding #[inline] in those cases just gives a hint to be slightly more aggressive about inlining than the default. Codegen units + absence of ThinLTO might change this to some degree, not sure what's the current interaction between CGUs and #[inline].

We should still be consistent about how we use #[inline], and there might very well be regressions in some cases caused by absence of #[inline]. But generally the attribute is not required to make a function inlinable.

@varkor
Copy link
Member

varkor commented Jan 15, 2018

Is there an improvement in the generated instructions if you annotate try_fold with #[inline]?

@jonasbb
Copy link
Contributor Author

jonasbb commented Jan 15, 2018

@varkor I do not have a minimal example or a good testcase where I could compare the assembly. I just noticed that in profiling the try_fold does not occur anymore, meaning it got inlined.

@rkruppe You make a good point. I forgot that generic functions are always inlineable. I got fooled by the results.

@scottmcm
Copy link
Member

I had no deep logic for marking or not marking try_fold methods with #[inline]. Roughly I had it there when the corresponding fold had it, but that's it.

@jonasbb I see "anymore" in your comment. Did the behaviour here change in a recent nightly?

@jonasbb
Copy link
Contributor Author

jonasbb commented Jan 16, 2018

@scottmcm No, it is unrelated to nightly changes. For other reasons I already have xargo setup and just played around with adding inline annotation and reprofiling.

@XAMPPRocky XAMPPRocky added C-enhancement Category: An issue proposing an enhancement or a PR with one. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Apr 23, 2018
@jonas-schievink jonas-schievink added the A-iterators Area: Iterators label Apr 20, 2020
@Pzixel
Copy link

Pzixel commented May 15, 2022

I can confirm that I have this issue on rustc 1.62.0-nightly (cb1219871 2022-05-08): code with iter.copied().map(|x| something(x)) produced bad assembly while iter.map(|x| something(*x) was properly inlined. I've tried to create an MRE but I wasn't able to reproduce it there. I'm not an expert on compiler optimizations but I can confirm that I witnessed this behavior in the wild.

Although I failed to create a MRE I can provide function source code and assembly that gets generated if needed. I suspect it has to deal something with inlining budget or some werights but I'm pretty sure I don't want to have iterators in resulting release assembly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-iterators Area: Iterators C-enhancement Category: An issue proposing an enhancement or a PR with one. I-slow Issue: Problems and improvements with respect to performance of generated code. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

8 participants