Inline len and is_empty in collections

[ljedrz]

Is there any reason not to? All of these are trivial and heavily utilized.

[rust-highfive]

r? @Mark-Simulacrum

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

[nikic]

I believe historically this has not been necessary, because generic methods were effectively automatically inline. However, this seems to no longer be entirely true nowadays, e.g. they made a large difference in #54668. It would be great if someone could clarify what exactly the impact of #[inline] on generic methods is nowadays.

[Centril]

cc @anp

[nagisa]

It used to be the case that generic functions/methods would be #[inline]-able by construction, yes. With that in mind, the guideline was to only add #[inline] if it is actually necessary.

However, nowadays, it is common to see cases where the instantiation in the dependency is called instead (and thus is not inlineable, unless LTO is enabled).

[nagisa]

@ljedrz with that in mind, are you seeing improvements in codegen with this patch?

[ljedrz]

@nagisa I'd be happy to check if you could suggest how I could do it or point me to some past PR that relied on it so I can follow its instructions.

[nagisa]

I simply had a test case that used said functions and looked at --emit=llvm-ir/asm output to see if the function(s) were inlined or not.

[ljedrz]

Ah, ok; I can do that but it will take some time until I can do some test builds; if the queue is clear, a perf run might be faster (and we would probably want one afterwards).

[ljedrz]

Ok, so I did some comparisons on a small test case and it appears that there is no difference in codegen; I'm pretty sure my test code does not get optimized away:

use std::env;
use std::collections::HashSet;

fn main() {
    let args = env::args().collect::<HashSet<_>>();

    println!("{:?}", args.is_empty());
}

I compiled with rustc main.rs --emit=asm, with and without -O.

Nevertheless, the inlined stage2 build is ~1MB smaller than the non-inlined one 🤔.

[nagisa]

@bors try

Lets run that perf run

[bors]

⌛ Trying commit 98c62b8 with merge bfa68b9...

[bors]

☀️ Test successful - status-travis
State: approved= try=True

[nagisa]

@rust-timer build bfa68b9

[rust-timer]

Success: Queued bfa68b9 with parent d311571, comparison URL.

[rust-timer]

Finished benchmarking try commit bfa68b9

[ljedrz]

The results look pretty neutral; maaaybe fractional improvements for some cases, but it might just be noise.

[memoryruins]

There is a bit of red on max-rss and faults, especially for deep-vector.

[ljedrz]

@memoryruins: these are within usual noise range; those benchmarks have high variance; that being said, I'm ok with closing the PR due to (most probably) no change.

[Mark-Simulacrum]

I'm going to close this as there's no impact performance wise.