std: Improve codegen size of accessing TLS#55518

alexcrichton · 2018-10-30T22:13:29Z

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 · 2018-10-30T22:13:33Z

r? @bluss

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

alexcrichton · 2018-10-30T22:13:50Z

r? @sfackler

sfackler · 2018-10-30T22:23:22Z

@bors r+

bors · 2018-10-30T22:23:23Z

📌 Commit 62eec9ee4c523ead0f46f1a5e6e85460ef02bc1a has been approved by sfackler

Mark-Simulacrum · 2018-10-31T00:56:42Z

src/libstd/thread/local.rs

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

Sure thing!

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

alexcrichton · 2018-10-31T01:31:52Z

@bors: r=sfackler

bors · 2018-10-31T01:31:53Z

📌 Commit 36990de9f160739bc56925d4e7036e13476300be has been approved by sfackler

alexcrichton · 2018-10-31T13:49:18Z

@bors: r=sfackler

bors · 2018-10-31T13:49:19Z

📌 Commit f64efa5ea3607047aba695154767030da5abf3d4 has been approved by sfackler

alexcrichton · 2018-10-31T14:00:16Z

@bors: r=sfackler

bors · 2018-10-31T14:00:22Z

📌 Commit c24c2c95f47f05eec17f3c4afbd981addb795fbf has been approved by sfackler

kennytm · 2018-11-01T14:49:48Z

@bors r-

Failed in #55579 (comment)

src/test/run-make/wasm-panic-small/foo.rs

No need for this to actually show up in optimized non-LTO executables!

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 · 2018-11-04T18:56:38Z

💔 Test failed - status-appveyor

alexcrichton · 2018-11-05T02:22:48Z

@bors: retry

Seems spurious ...

bors · 2018-11-05T04:25:21Z

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

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 · 2018-11-05T06:58:05Z

💔 Test failed - status-travis

rust-highfive · 2018-11-05T06:58:07Z

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 · 2018-11-05T09:36:51Z

@bors retry travis-ci/travis-ci#9696

bors · 2018-11-05T12:44:22Z

⌛ Testing commit 0c3d08e with merge d6cb28f...

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 · 2018-11-05T15:48:53Z

💔 Test failed - status-appveyor

alexcrichton · 2018-11-05T16:30:18Z

@bors: retry

3 hr timeout

bors · 2018-11-06T04:04:39Z

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

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 · 2018-11-06T06:49:55Z

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

rust-highfive assigned bluss Oct 30, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 30, 2018

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

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 30, 2018

Mark-Simulacrum reviewed Oct 31, 2018

View reviewed changes

alexcrichton force-pushed the smaller-wasm branch from 62eec9e to 36990de Compare October 31, 2018 01:31

alexcrichton force-pushed the smaller-wasm branch from 36990de to f64efa5 Compare October 31, 2018 13:49

This comment has been minimized.

Sign in to view

alexcrichton force-pushed the smaller-wasm branch from f64efa5 to c24c2c9 Compare October 31, 2018 14:00

emilyalbini mentioned this pull request Nov 1, 2018

Rollup of 13 pull requests #55579

Merged

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 1, 2018

kennytm reviewed Nov 1, 2018

View reviewed changes

src/test/run-make/wasm-panic-small/foo.rs Outdated Show resolved Hide resolved

alexcrichton added 2 commits November 1, 2018 08:24

wasm32: Inline a trivial function in libstd

49ec935

No need for this to actually show up in optimized non-LTO executables!

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 4, 2018

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 5, 2018

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 5, 2018

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 5, 2018

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 5, 2018

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 5, 2018

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

bors mentioned this pull request Nov 6, 2018

Remove the alloc_system crate #55660

Merged

alexcrichton deleted the smaller-wasm branch November 26, 2018 04:27

fitzgen mentioned this pull request May 10, 2019

Remove our custom thread local stuff in the anyref module wasm-bindgen/wasm-bindgen#1523

Closed

Uh oh!

Conversation

alexcrichton commented Oct 30, 2018

Uh oh!

rust-highfive commented Oct 30, 2018

Uh oh!

alexcrichton commented Oct 30, 2018

Uh oh!

sfackler commented Oct 30, 2018

Uh oh!

bors commented Oct 30, 2018

Uh oh!

Mark-Simulacrum Oct 31, 2018

Choose a reason for hiding this comment

Uh oh!

alexcrichton Oct 31, 2018

Choose a reason for hiding this comment

Uh oh!

RalfJung Oct 31, 2018

Choose a reason for hiding this comment

Uh oh!

alexcrichton Oct 31, 2018

Choose a reason for hiding this comment

Uh oh!

alexcrichton commented Oct 31, 2018

Uh oh!

bors commented Oct 31, 2018

Uh oh!

alexcrichton commented Oct 31, 2018

Uh oh!

bors commented Oct 31, 2018

Uh oh!

This comment has been minimized.

alexcrichton commented Oct 31, 2018

Uh oh!

bors commented Oct 31, 2018

Uh oh!

kennytm commented Nov 1, 2018

Uh oh!

Uh oh!

bors commented Nov 4, 2018

Uh oh!

alexcrichton commented Nov 5, 2018

Uh oh!

bors commented Nov 5, 2018

Uh oh!

bors commented Nov 5, 2018

Uh oh!

rust-highfive commented Nov 5, 2018

Uh oh!

kennytm commented Nov 5, 2018

Uh oh!

bors commented Nov 5, 2018

Uh oh!

bors commented Nov 5, 2018

Uh oh!

alexcrichton commented Nov 5, 2018

Uh oh!

bors commented Nov 6, 2018

Uh oh!

bors commented Nov 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants