Skip to content
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: Minimize size of panicking on wasm #49488

Merged
merged 4 commits into from
Apr 17, 2018

Conversation

alexcrichton
Copy link
Member

This commit applies a few code size optimizations for the wasm target to
the standard library, namely around panics. We notably know that in most
configurations it's impossible for us to print anything in
wasm32-unknown-unknown so we can skip larger portions of panicking that
are otherwise simply informative. This allows us to get quite a nice
size reduction.

Finally we can also tweak where the allocation happens for the
Box<Any> that we panic with. By only allocating once unwinding starts
we can reduce the size of a panicking wasm module from 44k to 350 bytes.

@alexcrichton
Copy link
Member Author

r? @sfackler

@est31
Copy link
Member

est31 commented Mar 29, 2018

So if I previously registered a custom panic handler to print the panic message via console.error (which gives you a nice stack trace btw), this panic handler will not print anything anymore?

In production you probably won't want such a handler so the default setting is probably most useful for production settings. Only asking in order to understand this PR.

/// not use.
#[unstable(feature = "std_internals", issue = "0")]
#[doc(hidden)]
pub trait BoxMeUp {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: shouldn't this be an unsafe trait since the box_me_up method must return a valid box pointer?

@alexcrichton
Copy link
Member Author

@est31

this panic handler will not print anything anymore?

No, we will still invoke panic handlers

@@ -164,6 +166,12 @@ fn default_hook(info: &PanicInfo) {
#[cfg(feature = "backtrace")]
use sys_common::backtrace;

// Some platfors know that printing to stderr won't ever actually print
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

platfor

// option. This file may not be copied, modified, or distributed
// except according to those terms.

#![unstable(feature = "thread_local_internals", issue = "0")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wat

@sfackler
Copy link
Member

LGTM other than the fast thread local stuff that sounds like it's not needed.

@est31
Copy link
Member

est31 commented Mar 30, 2018

@alexcrichton yes but will it get a meaningful payload?

@alexcrichton
Copy link
Member Author

@bors: r=sfackler

@est31 yes

@bors
Copy link
Contributor

bors commented Mar 30, 2018

📌 Commit 03a18aa has been approved by sfackler

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Mar 30, 2018
@alexcrichton
Copy link
Member Author

@bors: r-

Ah talking with @fitzgen yesterday I'd like to add a test for this as well

@bors 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 Mar 31, 2018
@fitzgen
Copy link
Member

fitzgen commented Mar 31, 2018

cc @rust-lang/wg-wasm; just a heads up!

we can reduce the size of a panicking wasm module from 44k to 350 bytes.

@alexcrichton
Copy link
Member Author

@bors: r=sfackler

@bors
Copy link
Contributor

bors commented Mar 31, 2018

📌 Commit 033b177 has been approved by sfackler

@bors 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 31, 2018
@bors
Copy link
Contributor

bors commented Mar 31, 2018

⌛ Testing commit 033b17760deef5251f9151f0347ad2e7d1192f1f with merge 31927a8e74b50918ba8a2d9f73c0072912245e10...

@bors
Copy link
Contributor

bors commented Mar 31, 2018

💔 Test failed - status-travis

@bors 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 Mar 31, 2018
@TimNN
Copy link
Contributor

TimNN commented Apr 7, 2018

Your PR failed on Travis. 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.
[01:33:45] died due to signal 11
[01:33:45] error: test failed, to rerun pass '--test coretests'
[01:33:45]
[01:33:45]
[01:33:45] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "test" "--target" "arm-linux-androideabi" "-j" "4" "--release" "--locked" "--color" "always" "--features" "panic-unwind jemalloc backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "-p" "core" "--"
---
$ ls -lat $HOME/Library/Logs/DiagnosticReports/
ls: cannot access /home/travis/Library/Logs/DiagnosticReports/: No such file or directory
travis_time:end:018c9a0b:start=1523093342887998261,finish=1523093342904923268,duration=16925007
travis_fold:end:after_failure.2
travis_fold:start:after_failure.3
travis_time:start:3045d9a8
$ find $HOME/Library/Logs/DiagnosticReports -type f -name '*.crash' -not -name '*.stage2-*.crash' -not -name 'com.apple.CoreSimulator.CoreSimulatorService-*.crash' -exec printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" {} \; -exec head -750 {} \; -exec echo travis_fold":"end:crashlog \; || true
find: `/home/travis/Library/Logs/DiagnosticReports': No such file or directory
travis_time:end:3045d9a8:start=1523093342910523349,finish=1523093342920816512,duration=10293163
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:035f6e3c
$ dmesg | grep -i kill
[   10.415132] init: failsafe main process (1094) killed by TERM signal

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.

1 similar comment
@TimNN
Copy link
Contributor

TimNN commented Apr 7, 2018

Your PR failed on Travis. 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.
[01:33:45] died due to signal 11
[01:33:45] error: test failed, to rerun pass '--test coretests'
[01:33:45]
[01:33:45]
[01:33:45] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "test" "--target" "arm-linux-androideabi" "-j" "4" "--release" "--locked" "--color" "always" "--features" "panic-unwind jemalloc backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "-p" "core" "--"
---
$ ls -lat $HOME/Library/Logs/DiagnosticReports/
ls: cannot access /home/travis/Library/Logs/DiagnosticReports/: No such file or directory
travis_time:end:018c9a0b:start=1523093342887998261,finish=1523093342904923268,duration=16925007
travis_fold:end:after_failure.2
travis_fold:start:after_failure.3
travis_time:start:3045d9a8
$ find $HOME/Library/Logs/DiagnosticReports -type f -name '*.crash' -not -name '*.stage2-*.crash' -not -name 'com.apple.CoreSimulator.CoreSimulatorService-*.crash' -exec printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" {} \; -exec head -750 {} \; -exec echo travis_fold":"end:crashlog \; || true
find: `/home/travis/Library/Logs/DiagnosticReports': No such file or directory
travis_time:end:3045d9a8:start=1523093342910523349,finish=1523093342920816512,duration=10293163
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:035f6e3c
$ dmesg | grep -i kill
[   10.415132] init: failsafe main process (1094) killed by TERM signal

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.

@kennytm
Copy link
Member

kennytm commented Apr 7, 2018

The same error happened in the previous rollup, so pretty sure it is legit. Or maybe it is #49775.

@kennytm kennytm 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-review Status: Awaiting review from the assignee but also interested parties. labels Apr 7, 2018
@bors
Copy link
Contributor

bors commented Apr 7, 2018

☔ The latest upstream changes (presumably #49661) made this pull request unmergeable. Please resolve the merge conflicts.

@alexcrichton
Copy link
Member Author

Ok I think I've debugged the issue of the failing test.

In the meantime I did a few more size optimizations here for libstd, @sfackler mind re-reviewing and re-r+'ing?

@alexcrichton
Copy link
Member Author

@bors: p=0

@@ -736,6 +736,10 @@ fn alloc_guard(alloc_size: usize) -> Result<(), CollectionAllocErr> {
}
}

fn capacity_overflow() -> ! {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels non-obvious as to why it's extracted, maybe a comment would be good?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -1212,7 +1212,7 @@ impl<'a> Formatter<'a> {
// truncation. However other flags like `fill`, `width` and `align`
// must act as always.
if let Some((i, _)) = s.char_indices().skip(max).next() {
&s[..i]
s.get(..i).unwrap_or(&s)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a comment would be helpful...

@@ -369,9 +424,7 @@ pub fn begin_panic<M: Any + Send>(msg: M, file_line_col: &(&'static str, u32, u3
/// This is the entry point or panics from libcore, formatted panics, and
/// `Box<Any>` panics. Here we'll verify that we're not panicking recursively,
/// run panic hooks, and then delegate to the actual implementation of panics.
#[inline(never)]
#[cold]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing these seems fine, but it might be good to get some perf stats on some benchmark (though I don't have any particular ideas...).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW the comment here is out of date and no longer correct, this function hasn't survived well through the various refactorings. I'll update it.

@alexcrichton
Copy link
Member Author

Updated!

@bors
Copy link
Contributor

bors commented Apr 13, 2018

☔ The latest upstream changes (presumably #49669) made this pull request unmergeable. Please resolve the merge conflicts.

This commit applies a few code size optimizations for the wasm target to
the standard library, namely around panics. We notably know that in most
configurations it's impossible for us to print anything in
wasm32-unknown-unknown so we can skip larger portions of panicking that
are otherwise simply informative. This allows us to get quite a nice
size reduction.

Finally we can also tweak where the allocation happens for the
`Box<Any>` that we panic with. By only allocating once unwinding starts
we can reduce the size of a panicking wasm module from 44k to 350 bytes.
Create one canonical location which panics with "capacity overflow" instead of
having many. This reduces the size of a `panic!("{}", 1)` binary on wasm from
34k to 17k.
The expression `&s[..i]` in general can panic if `i` is out of bounds or not on
a character boundary for a string, and this caused the codegen for
`Formatter::pad` to be a bit larger than it otherwise needed to be. This commit
replaces this with `s.get(..i).unwrap_or(&s)` which while having different
behavior if `i` is out of bounds has a much smaller code footprint and otherwise
avoids the need for `unsafe` code.
This commit removes allocation of the panic message in instances like
`panic!("foo: {}", "bar")` if we don't actually end up needing the message. We
don't need it in the case of wasm32 right now, and in general it's not needed
for panic=abort instances that use the default panic hook.

For now this commit only solves the wasm use case where with LTO the allocation
is entirely removed, but the panic=abort use case can be implemented at a later
date if needed.
@alexcrichton
Copy link
Member Author

ping r? @sfackler

@sfackler
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Apr 16, 2018

📌 Commit 46d16b6 has been approved by sfackler

@bors 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 16, 2018
@bors
Copy link
Contributor

bors commented Apr 16, 2018

⌛ Testing commit 46d16b6 with merge 3809bbf...

bors added a commit that referenced this pull request Apr 16, 2018
std: Minimize size of panicking on wasm

This commit applies a few code size optimizations for the wasm target to
the standard library, namely around panics. We notably know that in most
configurations it's impossible for us to print anything in
wasm32-unknown-unknown so we can skip larger portions of panicking that
are otherwise simply informative. This allows us to get quite a nice
size reduction.

Finally we can also tweak where the allocation happens for the
`Box<Any>` that we panic with. By only allocating once unwinding starts
we can reduce the size of a panicking wasm module from 44k to 350 bytes.
@bors
Copy link
Contributor

bors commented Apr 17, 2018

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

@bors bors merged commit 46d16b6 into rust-lang:master Apr 17, 2018
@alexcrichton alexcrichton deleted the small-wasm-panic branch April 20, 2018 06:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants