Optimize layout calculations in HashMap

[Amanieu]

This now produces the same assembly code as the previous implementation.

cc #51163@nnethercote@gnzlbg@andjo403

[rust-highfive]

r? @kennytm

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

[rust-highfive]

Warning

• These commits modify submodules.

[Amanieu]

(stupid git pull doesn't update submodules by default...)

Playground link for playing around with the generated code: https://play.rust-lang.org/?gist=b233a122e3223db47521f5b0a0a3911c&version=nightly&mode=release

[Amanieu]

r? @SimonSapin

[rust-highfive]

The job x86_64-gnu-llvm-3.9 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

[00:03:29] Compiling std_unicode v0.0.0 (file:///checkout/src/libstd_unicode)
[00:03:30] Compiling alloc_system v0.0.0 (file:///checkout/src/liballoc_system)
[00:03:30] Compiling panic_abort v0.0.0 (file:///checkout/src/libpanic_abort)
[00:03:35] Compiling panic_unwind v0.0.0 (file:///checkout/src/libpanic_unwind)
[00:03:36] error[E0425]: cannot find function `align_of` in this scope
[00:03:36] --> libstd/collections/hash/table.rs:663:60
[00:03:36] |
[00:03:36] 663 | (layout, hashes.size() + hashes.padding_needed_for(align_of::<A>()))
[00:03:36] | ^^^^^^^^ not found in this scope
[00:03:36] help: possible candidate is found in another module, you can import it into scope
[00:03:36] 11 | use core::mem::align_of;
[00:03:36] |
[00:03:36] [00:03:36] error[E0412]: cannot find type `A` in this scope
[00:03:36] error[E0412]: cannot find type `A` in this scope
[00:03:36] --> libstd/collections/hash/table.rs:663:71
[00:03:36] |
[00:03:36] 663 | (layout, hashes.size() + hashes.padding_needed_for(align_of::<A>()))
[00:03:36] | ^ did you mean `K`?
[00:03:39] error: aborting due to 2 previous errors
[00:03:39] [00:03:39] Some errors occurred: E0412, E0425.
[00:03:39] For more information about an error, try `rustc --explain E0412`.
[00:03:39] For more information about an error, try `rustc --explain E0412`.
[00:03:39] error: Could not compile `std`.
[00:03:39] [00:03:39] Caused by:
[00:03:39] process didn't exit successfully: `/checkout/obj/build/bootstrap/debug/rustc --crate-name std libstd/lib.rs --color always --error-format json --crate-type dylib --crate-type rlib --emit=dep-info,link -C prefer-dynamic -C opt-level=2 --cfg feature="alloc_jemalloc" --cfg feature="backtrace" --cfg feature="jemalloc" --cfg feature="panic-unwind" --cfg feature="panic_unwind" -C metadata=fc6b9a3d7065b2e2 -C extra-filename=-fc6b9a3d7065b2e2 --out-dir /checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/x86_64-unknown-linux-gnu/release/deps --target x86_64-unknown-linux-gnu -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/x86_64-unknown-linux-gnu/release/deps -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/release/deps --extern rustc_asan=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/x86_64-unknown-linux-gnu/release/deps/librustc_asan-a72c0fa0b2d5877e.rlib --extern std_unicode=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/x86_64-unknown-linux-gnu/release/deps/libstd_unicode-18039a1361938ca9.rlib --extern rustc_msan=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/x86_64-unknown-linux-gnu/release/deps/librustc_msan-ce1ae9dbeb02af61.rlib --extern rustc_lsan=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/x86_64-unknown-linux-gnu/release/deps/librustc_lsan-a2f6e9f263c82267.rlib --extern unwind=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/x86_64-unknown-linux-gnu/release/deps/libunwind-bd02867d7573c11e.rlib --extern alloc_system=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/x86_64-unknown-linux-gnu/release/deps/liballoc_system-386e278237d725f5.rlib --extern core=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/x86_64-unknown-linux-gnu/release/deps/libcore-e9cdce497aae9e81.rlib --extern panic_abort=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/x86_64-unknown-linux-gnu/release/deps/libpanic_abort-5f4d07ea9b3edda4.rlib --extern panic_unwind=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/x86_64-unknown-linux-gnu/release/deps/libpanic_unwind-2bd12fd5ac9768f9.rlib --extern rustc_tsan=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/x86_64-unknown-linux-gnu/release/deps/librustc_tsan-ccd411645051f1ef.rlib --extern libc=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/x86_64-unknown-linux-gnu/release/deps/liblibc-223f966213a60acb.rlib --extern alloc_jemalloc=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/x86_64-unknown-linux-gnu/release/deps/liballoc_jemalloc-5e6c2ac297f2c2e3.rlib --extern alloc=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/x86_64-unknown-linux-gnu/release/deps/liballoc-d6cd5f8b78fddf12.rlib --extern compiler_builtins=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/x86_64-unknown-linux-gnu/release/deps/libcompiler_builtins-90a13cda2e54742f.rlib -L native=/checkout/obj/build/x86_64-unknown-linux-gnu/native/libbacktrace/ -L native=/checkout/obj/build/x86_64-unknown-linux-gnu/native/libbacktrace -l static=backtrace -l static=backtrace -l dl -l rt -l pthread -L native=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/x86_64-unknown-linux-gnu/release/build/compiler_builtins-ffd422941bf53e42/out -L native=/checkout/obj/build/x86_64-unknown-linux-gnu/native/jemalloc/lib` (exit code: 101)
[00:03:39] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--features" "panic-unwind jemalloc backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "--message-format" "json"
[00:03:39] expected success, got: exit code: 101
[00:03:39] thread 'main' panicked at 'cargo must succeed', bootstrap/compile.rs:1091:9
[00:03:39] travis_fold:end:stage0-std
[00:03:39] travis_time:end:stage0-std:start=1528101059815394637,finish=1528101100038715281,duration=40223320644
[00:03:39] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:03:39] Build completed unsuccessfully in 0:00:41
[00:03:39] make: *** [tidy] Error 1
[00:03:39] Makefile:79: recipe for target 'tidy' failed
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:21f9eaf0
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

[gnzlbg]

This is weird, the NonZeroUsize::get(self) function is marked #[inline] and the constructor is a const fn. Did you by chance happen to look at the generated LLVM-IR for calculate_layout ?

[Amanieu]

Try the playground link that I posted above. If you remove the map then it generates worse code. What seems to happen is that LLVM will emit an overflowing multiply to calculate pairs.size, and will use the overflow flag to set pairs.align to either 0 (LayoutErr) or the actual alignment.

[nnethercote]

Perhaps the comment should include the second sentence of that explanation?

[SimonSapin]

Every caller of calculate_layout only use one of the returned Layout or offset. Now that they’re mostly calculated separately, aren’t we better off having two separate functions?

[Amanieu]

To me this feels like more of a temporary hack to deal with a LLVM bug, rather than a permanent solution. Logically the offset calculation belongs as part of the layout calculation.

[Amanieu]

@nnethercote I added a reference to #51346 in the comment.

[nnethercote]

Nice! Hopefully this will fix the rustc perf regression. rustc uses hash tables a lot :)

[gnzlbg]

Avoiding NonZeroUsize: https://play.rust-lang.org/?gist=419e4d6cee534087573e17943b02b674&version=nightly&mode=release

playground::new:pushraxmovrsi,rdimovecx,8movrax,rsimulrcxmovrcx,raxjo	.LBB3_3movedx,16movrax,rsimulrdxjo	.LBB3_3addrax,rcxjb	.LBB3_3movrax,rcxpoprcxret.LBB3_3:call	core::result::unwrap_failedud2

which is at least shorter (don't know if better) than using NonZeroUsize: https://play.rust-lang.org/?gist=9c5e1054b61976dffc49b2d26c86ad82&version=nightly&mode=release

playground::new:movecx,8xoresi,esimovrax,rdimulrcxmovr8,rax	setno	r9bjo	.LBB3_7movedx,16xorecx,ecxmovrax,rdimulrdx	setno	dljo	.LBB3_7mov	sil,r9bshlrsi,3movcl,dlshlrcx,3cmprcx,rsi	cmovae	rsi,rcxleardx,[r8+rcx]addrdx,-1negrcxandrcx,rdxsubrcx,r8addrcx,r8jb	.LBB3_7addrax,rcxjb	.LBB3_7movrdx,rsinegrdxcmprax,rdxja	.LBB3_7testrsi,rsije	.LBB3_7learax,[rsi+15]andrax,rsijne	.LBB3_7movrax,rcxret.LBB3_7:pushrbpmovrbp,rspcall	core::result::unwrap_failedud2

However, given that both situations should just generate:

playground::original:learax,[8*rdi]ret

I don't think that NonZeroUsize is the root of the issue here.

[Amanieu]

new is the implementation that is currently in rust right now (added by #51163). The one that you should be looking at is new_optimized which calls unreachable_unchecked if a LayoutErr occurs..

[gnzlbg]

@Amanieu that one was as good as the original IIRC (that is, this PR fixes the perf regression).

My point was that maybe it is worth fixing whatever codegen bug is causing the perf regression in the first place because, while we might add these workarounds in std, I don't think users of Layout outside of std will.

You mentioned the regression might be caused by making alignNonZeroUsize, but while that has an impact on the generated code, that does not appear to be the cause (and reverting that has little effect). I think it would be worth it to open an issue to at least try to get to the bottom of it.

[Amanieu]

@gnzlbg If you use my original playground link and remove the .map then you will see that the generated code is not the same:

playground::new_optimized:
movl	$16, %edx
xorl	%ecx, %ecx
movq	%rdi, %rax
mulq	%rdx
setno	%cl
shlq	$3, %rcx
leaq	(%rcx,%rdi,8), %rax
addq	$-1, %rax
negq	%rcx
andq	%rax, %rcx
movq	%rcx, %rax
retq
playground::original:
leaq	(,%rdi,8), %rax
retq

This is the codegen bug that I was talking about, which is caused by NonZeroUsize. This discussion was originally started because you were wondering why the .map call was necessary.

[gnzlbg]

Can you explain how does that show that it is caused by NonZeroUsize ? In my two playground links I have the code, once using NonZeroUsize (https://play.rust-lang.org/?gist=9c5e1054b61976dffc49b2d26c86ad82&version=nightly&mode=release), and once using usize instead (https://play.rust-lang.org/?gist=419e4d6cee534087573e17943b02b674&version=nightly&mode=release). The assembly code generated for new without the .map looks like the one you show in both cases.

[Amanieu]

It's obvious if you look at the generated LLVM IR:

define i64 @_ZN10playground13new_optimized17hf103a5e804c97ad6E(i64 %c) unnamed_addr #2 personality i32 (i32, i32, i64, %"unwind::libunwind::_Unwind_Exception"*, %"unwind::libunwind::_Unwind_Context"*)* @rust_eh_personality {
start:
%0 = shl i64 %c, 3
// This computes the alignment result for Layout::repeat.
// It checks if the multiplication overflows and returns either 0 (LayoutErr) or 8 (self.align).
%1 = tail call { i64, i1 } @llvm.umul.with.overflow.i64(i64 %c, i64 16) #7
%2 = extractvalue { i64, i1 } %1, 1
%spec.select.i15.i = select i1 %2, i64 0, i64 8
// This calculates the offset and Layout::padding_for in Layout::extend.
// The codegen bug is here: At this point, the alignment can *only* be 8, not 0
// since Err(LayoutErr) would have returned early before this point (due to ?).
// But LLVM fails to constant-propagate this for some reason.
%3 = add i64 %0, -1
%4 = add i64 %3, %spec.select.i15.i
%5 = sub nsw i64 0, %spec.select.i15.i
%6 = and i64 %4, %5
ret i64 %6
}

[gnzlbg]

Looking only at new_optimized with NonZeroUsize: https://play.rust-lang.org/?gist=e7cd2987b841d62657a4c8d114abea59&version=nightly&mode=release

playground::new_optimized:movrcx,rdimovedx,8movrax,rcxmulrdxmovrsi,raxmovedx,16xoredi,edimovrax,rcxmulrdx	setno	dillearax,[rsi+8*rdi]addrax,-1shlrdi,3negrdiandrdi,raxmovrax,rdiret

and without NonZeroUsize: https://play.rust-lang.org/?gist=25ce1213448b27d155bffd30c4adc831&version=nightly&mode=release

playground::new_optimized:learax,[8*rdi]ret

It looks like not using NonZeroUsize makes a hell of a difference.

[Mark-Simulacrum]

@bors p=1 for when this is approved

[SimonSapin]

Let’s land this to fix the perf regression, but I think it is rather unfortunate an somewhat defeats the point of doing #51163 in the first place.

@bors r+

[bors]

📌 Commit b69724f has been approved by SimonSapin

[bors]

⌛ Testing commit b69724f with merge 41affd0...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: SimonSapin
Pushing 41affd0 to master...

[nnethercote]

I just did some local measurements with Cachegrind and the numbers are looking much better -- the regression from #51163 looks to be entirely fixed. Thank you!

[nikic]

Perf link: http://perf.rust-lang.org/compare.html?start=cdc193db993eadab74d9ea439613e8f9d3725ac7&end=41affd03eb169830773cd1b11efda562ab81fad0&stat=instructions%3Au

This doesn't seem to show any difference though. Am using it incorrectly?

[Mark-Simulacrum]

That should be accurate; I'm not sure why we're seeing a difference from @nnethercote's local benchmarks.

[Amanieu]

Note that #51226 also significantly improve the performance since it marked several Layout functions as inlinable.

[nnethercote]

My measurements were with Cachegrind rather than perf-stat... that shouldn't make any difference though.

[nnethercote]

Can anyone think of reasons why I would see improvements on my machine, but the rustc-perf machine would not? Are the clang versions guaranteed to be the same?

[Mark-Simulacrum]

Clang is not installed on the perf collector (though not sure what specific clang you are referring to...); gcc is version 5.4.0.

[nnethercote]

Sorry, I meant LLVM, not clang. Are the LLVM versions guaranteed to be the same?

[Mark-Simulacrum]

Yes, LLVM should be version equivalent (by default), though the way it's compiled might be different, e.g. using gcc locally vs. clang on Travis. I suppose that could play a role, but it would be decidedly odd.