Don't auto-inline const functions

[oli-obk]

fixes #53451

[rust-highfive]

r? @cramertj

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

[eddyb]

cc @nikomatsakis @michaelwoerister @alexcrichton

[cramertj]

r? @eddyb

[eddyb]

r? @michaelwoerister

[michaelwoerister]

Hm, the approach doesn't seem quite right to me. With the PR we would translate all const fns to machine code, even if they are never used anywhere. That would add code bloat. Shouldn't we be able to handle this like we handle regular #[inline] functions (or do they fail too?). We should rather adapt reachable.rs to detect that the nested function is reachable.

[oli-obk]

We also translate normal functions without #[inline], even if they are unused in the crate that defines them. One can still write #[inline] const fn foo() {} after this PR.

If we start creating more complex const fns, they would get codegened multiple times if they are auto-inlined without any way to opt back out of the implicit inlining

[michaelwoerister]

We also translate normal functions without #[inline], even if they are unused in the crate that defines them.

Only, if they are publicly visible.

If we start creating more complex const fns, they would get codegened multiple times if they are auto-inlined without any way to opt back out of the implicit inlining.

Yes, that's true. Let's do a perf run.

I still think this PR is more of a work-around than a real solution to the problem. The compiler should be able to handle both cases. That it doesn't seems like a bug.

[michaelwoerister]

@bors try

[bors]

⌛ Trying commit eb18ddd with merge 5720c63ceb9a7b0029646d8a61763ae27f358b2f...

[oli-obk]

The compiler should be able to handle both cases. That it doesn't seems like a bug.

Yes indeed. I was going to fix the real thing and questioned the motive of auto-inlining const fns while I was doing so. Thus this PR

I still think this PR is more of a work-around than a real solution to the problem.

The bug is code asking whether functions have an inline attribute instead of asking the function's Instance whether it wants to be inlined.

[bors]

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

[oli-obk]

@rust-timer build 5720c63

[rust-timer]

Please provide the full 40 character commit hash.

[oli-obk]

@rust-timer build 5720c63ceb9a7b0029646d8a61763ae27f358b2f

[rust-timer]

Success: Queued 5720c63ceb9a7b0029646d8a61763ae27f358b2f with parent 289ad6e, comparison URL.

[rust-timer]

Finished benchmarking try commit 5720c63ceb9a7b0029646d8a61763ae27f358b2f

[oli-obk]

2% perf improvement on some benchmarks, 1% regression on hyper-opt

+14% to -6% max memory usage, pretty wild results: https://perf.rust-lang.org/compare.html?start=289ad6e9922683807d455ca0020dc2a8f7bd1a7b&end=5720c63ceb9a7b0029646d8a61763ae27f358b2f&stat=max-rss

[michaelwoerister]

Many of those max-rss benchmarks seem to be very flaky to begin with, especially deep-vector-check.

OK, let's do this. Const fns that are only every used in a const context and are not publicly visible should still never be instantiated (because the collector won't find any calls to them in that case).

@bors r+

[bors]

📌 Commit eb18ddd has been approved by michaelwoerister

[bors]

⌛ Testing commit eb18ddd with merge a7cc9ff7aeaeac0dc2b706f7ccd56cf555cd1e9f...

[bors]

💔 Test failed - status-travis

[rust-highfive]

The job i686-gnu 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.

[02:09:50] test sync::mpsc::tests::stress_recv_timeout_two_threads ... ok
[02:09:50] 
[02:09:50] failures:
[02:09:50] 
[02:09:50] ---- net::tcp::tests::connect_timeout_unbound stdout ----
[02:09:50] thread 'net::tcp::tests::connect_timeout_unbound' panicked at 'called `Result::unwrap_err()` on an `Ok` value: TcpStream { addr: V4(127.0.0.1:37428), peer: V4(127.0.0.1:37428), fd: 6 }', src/libcore/result.rs:1009:5
[02:09:50] 
[02:09:50] failures:
[02:09:50]     net::tcp::tests::connect_timeout_unbound
[02:09:50] 
[02:09:50] 
[02:09:50] test result: FAILED. 769 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out
[02:09:50] 
[02:09:50] error: test failed, to rerun pass '--lib'
[02:09:50] 
[02:09:50] 
[02:09:50] command did not execute successfully: "/checkout/obj/build/i686-unknown-linux-gnu/stage0/bin/cargo" "test" "--target" "i686-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--features" "panic-unwind backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "-p" "std" "--"
[02:09:50] 
[02:09:50] 
[02:09:50] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[02:09:50] Build completed unsuccessfully in 2:07:29
---
travis_time:end:163b9d02:start=1543049193915500450,finish=1543049193925339139,duration=9838689
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:0270198b
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:2105f9d6
travis_time:start:2105f9d6
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:16246520
$ dmesg | grep -i kill

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)

[oli-obk]

@bors retry network test spuriously thinking it was able to connect