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

std: Improve codegen size of accessing TLS #55518

Merged
merged 2 commits into from Nov 6, 2018

Conversation

Projects
None yet
8 participants
@alexcrichton
Member

alexcrichton commented Oct 30, 2018

Some code in the TLS implementation in libstd stores Some(val) into an
&mut Option<T> (effectively) and then pulls out &T, but it currently
uses .unwrap() which can codegen into a panic even though it can never
panic. With sufficient optimizations enabled (like LTO) the compiler can
see through this but this commit helps it along in normal mode
(--release with Cargo by default) to avoid codegen'ing the panic path.

This ends up improving the optimized codegen on wasm by ensuring that a
call to panic pulling in more file size doesn't stick around.

@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Oct 30, 2018

r? @bluss

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

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Oct 30, 2018

@rust-highfive rust-highfive assigned sfackler and unassigned bluss Oct 30, 2018

@sfackler

This comment has been minimized.

Member

sfackler commented Oct 30, 2018

@bors r+

@bors

This comment has been minimized.

Contributor

bors commented Oct 30, 2018

📌 Commit 62eec9e has been approved by sfackler

(*ptr).as_ref().unwrap()
match *ptr {
Some(ref x) => x,
None => intrinsics::unreachable(),

This comment has been minimized.

@Mark-Simulacrum

Mark-Simulacrum Oct 31, 2018

Member

I think this really deserves a comment indicating why it was done -- could you add one? That way we can avoid confusion or cleanup removing this without knowing what to check

This comment has been minimized.

@alexcrichton

alexcrichton Oct 31, 2018

Member

Sure thing!

This comment has been minimized.

@RalfJung

RalfJung Oct 31, 2018

Member

Also don't we have unreachable_unchecked so that we do not have to call an intrinsic directly?

This comment has been minimized.

@alexcrichton

@alexcrichton alexcrichton force-pushed the alexcrichton:smaller-wasm branch from 62eec9e to 36990de Oct 31, 2018

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Oct 31, 2018

@bors: r=sfackler

@bors

This comment has been minimized.

Contributor

bors commented Oct 31, 2018

📌 Commit 36990de has been approved by sfackler

@alexcrichton alexcrichton force-pushed the alexcrichton:smaller-wasm branch from 36990de to f64efa5 Oct 31, 2018

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Oct 31, 2018

@bors: r=sfackler

@bors

This comment has been minimized.

Contributor

bors commented Oct 31, 2018

📌 Commit f64efa5 has been approved by sfackler

@rust-highfive

This comment was marked as resolved.

Collaborator

rust-highfive commented Oct 31, 2018

The job x86_64-gnu-llvm-5.0 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.
travis_time:end:0973e462:start=1540993844087269090,finish=1540993897605643361,duration=53518374271
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#Pull-Requests-and-Security-Restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-5.0
---
[00:04:34]    Compiling alloc v0.0.0 (/checkout/src/liballoc)
[00:04:35]    Compiling alloc_system v0.0.0 (/checkout/src/liballoc_system)
[00:04:35]    Compiling panic_abort v0.0.0 (/checkout/src/libpanic_abort)
[00:04:40]    Compiling panic_unwind v0.0.0 (/checkout/src/libpanic_unwind)
[00:04:41] error[E0425]: cannot find function `unreachable` in module `hint`
[00:04:41]     |
[00:04:41] 286 |             None => hint::unreachable(),
[00:04:41]     |                           ^^^^^^^^^^^ not found in `hint`
[00:04:41] help: possible candidate is found in another module, you can import it into scope
---
[00:04:44] 
[00:04:44] To learn more, run the command again with --verbose.
[00:04:44] 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:04:44] expected success, got: exit code: 101
[00:04:44] thread 'main' panicked at 'cargo must succeed', bootstrap/compile.rs:1115:9
[00:04:44] travis_fold:end:stage0-std

[00:04:44] travis_time:end:stage0-std:start=1540994152659334940,finish=1540994192963284602,duration=40303949662


[00:04:44] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap build
[00:04:44] Build completed unsuccessfully in 0:00:41
[00:04:44] Makefile:28: recipe for target 'all' failed
[00:04:44] make: *** [all] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:05cfb831
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:117972d8:start=1540994193575495334,finish=1540994193580007948,duration=4512614
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:09861eb1
$ 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:30ba3b40
travis_time:start:30ba3b40
$ 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:07436812
$ 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)

@alexcrichton alexcrichton force-pushed the alexcrichton:smaller-wasm branch from f64efa5 to c24c2c9 Oct 31, 2018

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Oct 31, 2018

@bors: r=sfackler

@bors

This comment has been minimized.

Contributor

bors commented Oct 31, 2018

📌 Commit c24c2c9 has been approved by sfackler

pietroalbini added a commit to pietroalbini/rust that referenced this pull request Nov 1, 2018

Rollup merge of rust-lang#55518 - alexcrichton:smaller-wasm, r=sfackler
std: Improve codegen size of accessing TLS

Some code in the TLS implementation in libstd stores `Some(val)` into an
`&mut Option<T>` (effectively) and then pulls out `&T`, but it currently
uses `.unwrap()` which can codegen into a panic even though it can never
panic. With sufficient optimizations enabled (like LTO) the compiler can
see through this but this commit helps it along in normal mode
(`--release` with Cargo by default) to avoid codegen'ing the panic path.

This ends up improving the optimized codegen on wasm by ensuring that a
call to panic pulling in more file size doesn't stick around.

bors added a commit that referenced this pull request Nov 1, 2018

Auto merge of #55579 - pietroalbini:rollup, r=pietroalbini
Rollup of 10 pull requests

Successful merges:

 - #55280 (Add libproc_macro to rust-src distribution)
 - #55469 (Regression tests for issue #54477.)
 - #55504 (Use vec![x; n] instead of iter::repeat(x).take(n).collect())
 - #55518 (std: Improve codegen size of accessing TLS)
 - #55522 (use String::from() instead of format!() macro to construct Strings.)
 - #55536 (Pass suggestions as impl Iterator instead of Vec)
 - #55542 (syntax: improve a few allocations)
 - #55558 (Tweak `MatcherPos::matches`)
 - #55574 (Use `SmallVec` within `MoveData`.)
 - #55575 (Fix invalid_const_promotion test on some archs)

Failed merges:

r? @ghost
@kennytm

This comment has been minimized.

Member

kennytm commented Nov 1, 2018

@bors r-

Failed in #55579 (comment)

alexcrichton added some commits Oct 28, 2018

wasm32: Inline a trivial function in libstd
No need for this to actually show up in optimized non-LTO executables!
std: Improve codegen size of accessing TLS
Some code in the TLS implementation in libstd stores `Some(val)` into an
`&mut Option<T>` (effectively) and then pulls out `&T`, but it currently
uses `.unwrap()` which can codegen into a panic even though it can never
panic. With sufficient optimizations enabled (like LTO) the compiler can
see through this but this commit helps it along in normal mode
(`--release` with Cargo by default) to avoid codegen'ing the panic path.

This ends up improving the optimized codegen on wasm by ensuring that a
call to panic pulling in more file size doesn't stick around.

bors added a commit that referenced this pull request Nov 4, 2018

Auto merge of #55518 - alexcrichton:smaller-wasm, r=sfackler
std: Improve codegen size of accessing TLS

Some code in the TLS implementation in libstd stores `Some(val)` into an
`&mut Option<T>` (effectively) and then pulls out `&T`, but it currently
uses `.unwrap()` which can codegen into a panic even though it can never
panic. With sufficient optimizations enabled (like LTO) the compiler can
see through this but this commit helps it along in normal mode
(`--release` with Cargo by default) to avoid codegen'ing the panic path.

This ends up improving the optimized codegen on wasm by ensuring that a
call to panic pulling in more file size doesn't stick around.
@bors

This comment has been minimized.

Contributor

bors commented Nov 4, 2018

💔 Test failed - status-appveyor

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Nov 5, 2018

@bors: retry

Seems spurious ...

@bors

This comment has been minimized.

Contributor

bors commented Nov 5, 2018

⌛️ Testing commit 0c3d08e with merge 8e950fb...

bors added a commit that referenced this pull request Nov 5, 2018

Auto merge of #55518 - alexcrichton:smaller-wasm, r=sfackler
std: Improve codegen size of accessing TLS

Some code in the TLS implementation in libstd stores `Some(val)` into an
`&mut Option<T>` (effectively) and then pulls out `&T`, but it currently
uses `.unwrap()` which can codegen into a panic even though it can never
panic. With sufficient optimizations enabled (like LTO) the compiler can
see through this but this commit helps it along in normal mode
(`--release` with Cargo by default) to avoid codegen'ing the panic path.

This ends up improving the optimized codegen on wasm by ensuring that a
call to panic pulling in more file size doesn't stick around.
@bors

This comment has been minimized.

Contributor

bors commented Nov 5, 2018

💔 Test failed - status-travis

@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Nov 5, 2018

The job x86_64-gnu-distcheck 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:27:37] warning: spurious network error (2 tries remaining): curl error: Could not resolve host: github.com
[02:27:37] ; class=Net (12)
[02:27:57] warning: spurious network error (1 tries remaining): curl error: Could not resolve host: github.com
[02:27:57] ; class=Net (12)
[02:28:18] error: failed to load source for a dependency on `rand`
[02:28:18] Caused by:
[02:28:18]   Unable to update registry `https://github.com/rust-lang/crates.io-index`
[02:28:18] 
[02:28:18] Caused by:
[02:28:18] Caused by:
[02:28:18]   failed to fetch `https://github.com/rust-lang/crates.io-index`
[02:28:18] 
[02:28:18] Caused by:
[02:28:18]   curl error: Could not resolve host: github.com
[02:28:18] ; class=Net (12)
[02:28:18] 
[02:28:18] 
[02:28:18] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "generate-lockfile" "--manifest-path" "/checkout/obj/build/tmp/distcheck-src/rust-src/lib/rustlib/src/rust/src/libstd/Cargo.toml"
[02:28:18] 
[02:28:18] 
[02:28:18] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test distcheck
[02:28:18] Build completed unsuccessfully in 2:25:39
---
travis_time:end:077f9314:start=1541400965139249602,finish=1541400965147256143,duration=8006541
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:0d540134
$ 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:start:crashlog
obj/cores/core.5222.!checkout!obj!build!tmp!distcheck!build!x86_64-unknown-linux-gnu!stage2!bin!rustc
[New LWP 5261]
[New LWP 5260]
[New LWP 5222]
warning: Could not load shared library symbols for 8 libraries, e.g. /lib/x86_64-linux-gnu/libc.so.6.
Use the "info sharedlibrary" command to see the complete listing.
Do you need "set solib-search-path" or "set sysroot"?
Core was generated by `/checkout/obj/build/tmp/distcheck/build/x86_64-unknown-linux-gnu/stage2/bin/rus'.
Program terminated with signal SIGABRT, Aborted.
#0  0x00007fc6341ce428 in ?? ()
#0  0x00007fc6341ce428 in ?? ()
#1  0x00007fc6341d002a in ?? ()
#2  0x0000000000000020 in ?? ()
#3  0x0000000000000000 in ?? ()
travis_time:end:0d540134:start=1541400965155673864,finish=1541400966883935842,duration=1728261978
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:02407ceb
travis_time:start:02407ceb
$ 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:187109f4
$ 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)

@kennytm

This comment has been minimized.

Member

kennytm commented Nov 5, 2018

@bors

This comment has been minimized.

Contributor

bors commented Nov 5, 2018

⌛️ Testing commit 0c3d08e with merge d6cb28f...

bors added a commit that referenced this pull request Nov 5, 2018

Auto merge of #55518 - alexcrichton:smaller-wasm, r=sfackler
std: Improve codegen size of accessing TLS

Some code in the TLS implementation in libstd stores `Some(val)` into an
`&mut Option<T>` (effectively) and then pulls out `&T`, but it currently
uses `.unwrap()` which can codegen into a panic even though it can never
panic. With sufficient optimizations enabled (like LTO) the compiler can
see through this but this commit helps it along in normal mode
(`--release` with Cargo by default) to avoid codegen'ing the panic path.

This ends up improving the optimized codegen on wasm by ensuring that a
call to panic pulling in more file size doesn't stick around.
@bors

This comment has been minimized.

Contributor

bors commented Nov 5, 2018

💔 Test failed - status-appveyor

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Nov 5, 2018

@bors: retry

3 hr timeout

@bors

This comment has been minimized.

Contributor

bors commented Nov 6, 2018

⌛️ Testing commit 0c3d08e with merge 24e66c2...

bors added a commit that referenced this pull request Nov 6, 2018

Auto merge of #55518 - alexcrichton:smaller-wasm, r=sfackler
std: Improve codegen size of accessing TLS

Some code in the TLS implementation in libstd stores `Some(val)` into an
`&mut Option<T>` (effectively) and then pulls out `&T`, but it currently
uses `.unwrap()` which can codegen into a panic even though it can never
panic. With sufficient optimizations enabled (like LTO) the compiler can
see through this but this commit helps it along in normal mode
(`--release` with Cargo by default) to avoid codegen'ing the panic path.

This ends up improving the optimized codegen on wasm by ensuring that a
call to panic pulling in more file size doesn't stick around.
@bors

This comment has been minimized.

Contributor

bors commented Nov 6, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: sfackler
Pushing 24e66c2 to master...

@bors bors merged commit 0c3d08e into rust-lang:master Nov 6, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@alexcrichton alexcrichton deleted the alexcrichton:smaller-wasm branch Nov 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment