Repository navigation
2x benchmark loss in rayon-hash from multiple codegen-units #47665
Description
I'm seeing a huge slowdown in rayon-hash benchmarks, resolved with -Ccodegen-units=1.
$ rustc -Vv
rustc 1.25.0-nightly (97520ccb1 2018-01-21)
binary: rustc
commit-hash: 97520ccb101609af63f29919bb0a39115269c89e
commit-date: 2018-01-21
host: x86_64-unknown-linux-gnu
release: 1.25.0-nightly
LLVM version: 4.0
$ cargo bench --bench set_sum
Compiling [...]
Finished release [optimized] target(s) in 5.51 secs
Running target/release/deps/set_sum-833cf161cf760670
running 4 tests
test rayon_set_sum_parallel ... bench: 2,295,348 ns/iter (+/- 152,025)
test rayon_set_sum_serial ... bench: 7,730,830 ns/iter (+/- 171,552)
test std_set_sum_parallel ... bench: 10,038,209 ns/iter (+/- 188,189)
test std_set_sum_serial ... bench: 7,733,258 ns/iter (+/- 134,850)
test result: ok. 0 passed; 0 failed; 0 ignored; 4 measured; 0 filtered out
$ RUSTFLAGS=-Ccodegen-units=1 cargo bench --bench set_sum
Compiling [...]
Finished release [optimized] target(s) in 6.48 secs
Running target/release/deps/set_sum-833cf161cf760670
running 4 tests
test rayon_set_sum_parallel ... bench: 1,092,732 ns/iter (+/- 105,979)
test rayon_set_sum_serial ... bench: 6,152,751 ns/iter (+/- 83,103)
test std_set_sum_parallel ... bench: 8,957,618 ns/iter (+/- 132,791)
test std_set_sum_serial ... bench: 6,144,775 ns/iter (+/- 75,377)
test result: ok. 0 passed; 0 failed; 0 ignored; 4 measured; 0 filtered out
rayon_set_sum_parallel is the showcase for this crate, and it suffers the most from CGU.
From profiling and disassembly, this seems to mostly be a lost inlining opportunity. In the slower build, the profile is split 35% bridge_unindexed_producer_consumer, 34% Iterator::fold, 28% Sum::sum, and the hot loop in the first looks like:
16.72 │126d0: cmpq $0x0,(%r12,%rbx,8)
6.73 │126d5: ↓ jne 126e1 <rayon::iter::plumbing::bridge_unindexed_producer_consumer+0x201>
16.65 │126d7: inc %rbx
0.00 │126da: cmp %rbp,%rbx
7.20 │126dd: ↑ jb 126d0 <rayon::iter::plumbing::bridge_unindexed_producer_consumer+0x1f0>
0.05 │126df: ↓ jmp 1272f <rayon::iter::plumbing::bridge_unindexed_producer_consumer+0x24f>
26.93 │126e1: mov 0x0(%r13,%rbx,4),%eax
4.26 │126e6: movq $0x1,0x38(%rsp)
2.27 │126ef: mov %rax,0x40(%rsp)
1.88 │126f4: mov %r14,%rdi
4.58 │126f7: → callq 15b90 <<u64 as core::iter::traits::Sum>::sum>
0.68 │126fc: movq $0x1,0x38(%rsp)
2.58 │12705: mov %r15,0x40(%rsp)
0.62 │1270a: movq $0x1,0x48(%rsp)
0.31 │12713: mov %rax,0x50(%rsp)
0.49 │12718: movb $0x0,0x58(%rsp)
2.50 │1271d: xor %esi,%esi
0.41 │1271f: mov %r14,%rdi
0.85 │12722: → callq 153f0 <<core::iter::Chain<A, B> as core::iter::iterator::Iterator>::fold>
1.30 │12727: mov %rax,%r15
2.16 │1272a: ↑ jmp 126d7 <rayon::iter::plumbing::bridge_unindexed_producer_consumer+0x1f7>With CGU=1, 96% of the profile is in bridge_unindexed_producer_consumer, with this hot loop:
2.28 │1426d: test %rbx,%rbx
│14270: ↓ je 14296 <rayon::iter::plumbing::bridge_unindexed_producer_consumer+0x146>
5.40 │14272: mov (%rbx),%ebx
2.75 │14274: add %rbx,%rax
1.47 │14277: lea (%rdx,%rsi,4),%rbx
0.21 │1427b: nopl 0x0(%rax,%rax,1)
29.56 │14280: cmp %rdi,%rsi
0.04 │14283: ↓ jae 14296 <rayon::iter::plumbing::bridge_unindexed_producer_consumer+0x146>
2.87 │14285: add $0x4,%rbx
20.22 │14289: cmpq $0x0,(%rcx,%rsi,8)
1.48 │1428e: lea 0x1(%rsi),%rsi
8.00 │14292: ↑ je 14280 <rayon::iter::plumbing::bridge_unindexed_producer_consumer+0x130>
25.25 │14294: ↑ jmp 1426d <rayon::iter::plumbing::bridge_unindexed_producer_consumer+0x11d>
Activity
nikomatsakis commentedon Jan 25, 2018
@cuviper I wonder if we can add any
#[inline]hints to help resolve this?cuviper commentedon Jan 25, 2018
@nikomatsakis you mean in libstd? Neither
Chain::foldnoru64::sumhave#[inline]attributes today, but usually that's not needed for generic functions. I guess being generic means they didn't (couldn't) get pre-compiled in libstd, but I suppose they must have gotten separate codegen units in the end.But I hope we don't have to generally recommend
#[inline]all over the place...Mark-Simulacrum commentedon Jan 25, 2018
I'm not entirely sure this isn't expected; 2x is a bit much though. We might need to wait on the heuristics that @michaelwoerister has planned.
nikomatsakis commentedon Jan 26, 2018
@cuviper I'm not sure that being generic affects their need for an inlining hint. It is true that those functions will be instantiated in the downstream crate, but I think they are still isolated into their own codegen unit. (ThinLTO is of course supposed to help here.) I'm not sure what's the best fix though.
cuviper commentedon Feb 14, 2018
FWIW, the gap has closed some (perhaps from LLVM 6?), with parallel codegen now "only" 40% slower:
The profile of
rayon_set_sum_parallelis now 49.5%bridge_unindexed_producer_consumer, 48%Sum::sum, with this hot loop:versus
codegen-units=1:michaelwoerister commentedon Feb 15, 2018
@rust-lang/cargo, maybe
cargo benchshould default to one codegen-unit?hanna-kruppe commentedon Feb 15, 2018
If
cargo benchuses one CGU butcargo build --releaseuses multiple CGUs, benchmark results are less representative of the code that actually runs at the end of the day.michaelwoerister commentedon Feb 15, 2018
Yeah, I was thinking that too after I wrote that.
--releasereally sounds like it should be the profile configured for best possible performance. We need an additionalopt-devprofile that has "good enough" performance.matklad commentedon Feb 15, 2018
@michaelwoerister today Cargo uses separate profiles for
cargo build --releaseandcargo bench, but, by default, they use the same options. So it is possible to override number of cgus for benchmarks specifically manually.However, long-term there is a desire to phase out all special profiles except release and dev (at least in the narrow sense of profiles, as "flags for the compiler"). See also rust-lang/rfcs#2282 which suggests adding user-defined profiles.
nikomatsakis commentedon Feb 15, 2018
The question of whether or not to have an
opt-devprofile and so forth has been around for some time. My current opinion is that we can probably keep it two options, but I would do this by altering the "debug" profile: instead of doing no optimization, it should do some minimal amount of optimization. It will take some time to determine precisely what that means, but I strongly suspect that we can get "decently performing code" by doing some mild inlining and few other minor optimizations. So the default might be something more like -O1 or -Og.In that case, we might consider some other profiles (e.g., no optimization at all, or mild optimization), but I sort of suspect that the use cases are thinner. In my experience, the only reason to want -O0 was for debuginfo, and let's premise that our default profile keeps debuginfo intact. Something like opt-dev I guess corresponds to "I don't care about debuginfo but i'm not benchmarking" -- that might be better served by customizing the "default' build settings for you project?
Still, it seems like there is ultimately maybe a need for three profiles:
Probably though this conversation should be happening somewhere else. =)
aturon commentedon Feb 15, 2018
Yeah, this is closely connected to the work on revamping profiles in Cargo.
cuviper commentedon Feb 15, 2018
I just benchmarked as part of updating to rayon-1.0, and it's back to 2x slowdown, with
Chain::foldappearing in the profile again. I guess the prior inlining improvement was just lucky, whether they're grouped in the same codegen unit.workingjubilee commentedon Jul 18, 2022
Is this still an issue of concern?
cuviper commentedon Jul 18, 2022
Well
rayon-hashis deprecated, but I guess the comparison may still be informative?I was originally sad about
rayon_set_sum_parallel, and now that's pretty close, actually slower with CGU=1 but within+/-. It also looks consistent and great forhashbrown_set_sum_parallel. The rest have a pretty large benefit from CGU=1.