Use cold functions for panic formatting Option::expect, Result::unwra… #31116

bluss · 2016-01-22T16:13:58Z

Use cold functions for panic formatting Option::expect, Result::unwrap, expect

These methods are marked inline, but insert a big chunk of formatting
code, as well as other error path related code, such as
deallocating a std::io::Error if you have one.

We can explicitly separate out that code path into a function that is
never inline, since the panicking case should always be rare.

rust-highfive · 2016-01-22T16:14:09Z

r? @nikomatsakis

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

bluss · 2016-01-22T16:31:57Z

Playpen code to compare these in artificial setting: https://play.rust-lang.org/?gist=94e1ab9ed151c75a5a97&version=nightly

Option::unwrap is already good. Look at the optimized versions of option_expect and option_expect_new to compare them. option_expect_new now works just like Option::unwrap does, with less code even in the Some case too.

nagisa · 2016-01-22T16:33:56Z

Looks pretty good to me (if not clear yet: I really, really like this :)). For Result::expect this saves ~80 instructions (improvement around the 65% mark) for the optimised assembly in the call location.

EDIT: still needs same treatment for Result::unwrap_err

nikomatsakis · 2016-01-22T16:48:46Z

Seems like a good idea to me. (Though I still dislike Option::expect with the passion of...well, not that much passion really. But a little.)

bluss · 2016-01-22T16:54:49Z

With this change I might even become an Option.expect() convert.

alexcrichton · 2016-01-22T17:03:52Z

src/libcore/option.rs

+    #[cold]
+    fn expect_failed(msg: &str) -> ! {
+        panic!("{}", msg)
+    }


Could this be moved outside of Option<T>? That should mean it's not monomorphized and should be even smaller I believe

Good catch, didn't think of that. It applies to everything I think.

alexcrichton · 2016-01-22T17:06:53Z

Nice ideas! I had a few more other ideas about monomorphization and seeing if we can't codegen these in libstd, but otherwise I'm always excited about changes like these.

bluss · 2016-01-22T17:08:16Z

Maybe Result's unwrap_failed<E> should move out of the impl block too, is it unnecessarily monomorphized per T otherwise?

brson · 2016-01-22T17:43:48Z

Neat find.

arthurprs · 2016-01-22T17:43:52Z

👏 I like this

alexcrichton · 2016-01-22T17:50:15Z

src/libcore/result.rs

@@ -685,7 +685,7 @@ impl<T, E: fmt::Debug> Result<T, E> {
        match self {
            Ok(t) => t,
            Err(e) =>
-                panic!("called `Result::unwrap()` on an `Err` value: {:?}", e)
+                unwrap_failed("called `Result::unwrap()` on an `Err` value: {:?}", e),


I think the trailing {:?} here can be removed

(same below as well

It's supposed to be more consistent with a trailing comma for every case. Not sure what rustfmt usually does.

oh.. reading.. how do I?

I see. That's an important change.

Oh I think that the trailing : {:?} part of the string can be removed in both cases, that's covered by the format string at the very bottom

bluss · 2016-01-22T17:55:20Z

I will rebase to squash all the fixups together into one commit.

…p etc Option::expect, Result::unwrap, unwrap_err, expect These methods are marked inline, but insert a big chunk of formatting code, as well as other error path related code, such as deallocating a std::io::Error if you have one. We can explicitly separate out that code path into a function that is never inline, since the panicking case should always be rare.

Move functions out of their impl blocks; avoids unnecessary monomorphization by type parameters that are irrelevant to the message output.

bluss · 2016-01-22T18:24:56Z

r? @alexcrichton

This is done, unless we want to measure the difference of something a bit more interesting. I don't really have the resources to build all of rust multiple times though.

alexcrichton · 2016-01-22T18:42:44Z

@bors: r+ 257bff3

Use cold functions for panic formatting Option::expect, Result::unwrap, expect These methods are marked inline, but insert a big chunk of formatting code, as well as other error path related code, such as deallocating a std::io::Error if you have one. We can explicitly separate out that code path into a function that is never inline, since the panicking case should always be rare.

bors · 2016-01-23T00:33:24Z

⌛ Testing commit 257bff3 with merge d63b8e5...

bors · 2016-01-23T02:23:45Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

hudson-ayers · 2021-09-30T18:53:01Z

I realize this is a very old PR, but I wanted to point out one downside of this change:

With this change, for expect() messages, even if your panic handler does nothing (i.e. panic_immediate_abort), the messages passed to expect() are still embedded in your binary.

Consider the two following code snippets:

fn check(opt: Option<u32>) -> u32 {
    opt.expect("This option should always be Some by the time this function is called.")
}

fn check(opt: Option<u32>) -> u32 {
    match opt {
        Some(val) => return val,
        None => panic!("This option should always be Some by the time this function is called.")
    }
}

I would typically assume these two functions should be identical, but in a binary with no panic handler only the former pays the code size cost of the message being embedded in the binary.

Of course, in a binary that has many calls to expect() and does have an actual panic handler, this change generates substantial savings. But in severely size-constrained binaries with no panic handler, it is a shame that messages passed to expect() cannot be optimized out.

rust-highfive assigned nikomatsakis Jan 22, 2016

bluss mentioned this pull request Jan 22, 2016

Implement owning constructors taking dimensions and strides rust-ndarray/ndarray#60

Merged

bluss force-pushed the expect-out-cold branch from e03d5e5 to 9100917 Compare January 22, 2016 17:03

alexcrichton reviewed Jan 22, 2016
View reviewed changes

bluss force-pushed the expect-out-cold branch from 1a84e70 to 9ff3c2d Compare January 22, 2016 17:23

alexcrichton reviewed Jan 22, 2016
View reviewed changes

bluss added 2 commits January 22, 2016 19:06

Move cold panic functions in Option and Result

257bff3

Move functions out of their impl blocks; avoids unnecessary monomorphization by type parameters that are irrelevant to the message output.

bluss force-pushed the expect-out-cold branch from 80766e6 to 257bff3 Compare January 22, 2016 18:08

rust-highfive assigned alexcrichton and unassigned nikomatsakis Jan 22, 2016

bors merged commit 257bff3 into rust-lang:master Jan 23, 2016

bluss deleted the expect-out-cold branch January 23, 2016 08:41

Use cold functions for panic formatting Option::expect, Result::unwra… #31116

Use cold functions for panic formatting Option::expect, Result::unwra… #31116

Uh oh!

Conversation

bluss commented Jan 22, 2016

Uh oh!

rust-highfive commented Jan 22, 2016

Uh oh!

bluss commented Jan 22, 2016

Uh oh!

nagisa commented Jan 22, 2016

Uh oh!

nikomatsakis commented Jan 22, 2016

Uh oh!

bluss commented Jan 22, 2016

Uh oh!

alexcrichton Jan 22, 2016

Choose a reason for hiding this comment

Uh oh!

bluss Jan 22, 2016

Choose a reason for hiding this comment

Uh oh!

alexcrichton commented Jan 22, 2016

Uh oh!

bluss commented Jan 22, 2016

Uh oh!

brson commented Jan 22, 2016

Uh oh!

arthurprs commented Jan 22, 2016

Uh oh!

alexcrichton Jan 22, 2016

Choose a reason for hiding this comment

Uh oh!

alexcrichton Jan 22, 2016

Choose a reason for hiding this comment

Uh oh!

bluss Jan 22, 2016

Choose a reason for hiding this comment

Uh oh!

bluss Jan 22, 2016

Choose a reason for hiding this comment

Uh oh!

alexcrichton Jan 22, 2016

Choose a reason for hiding this comment

Uh oh!

bluss commented Jan 22, 2016

Uh oh!

bluss commented Jan 22, 2016

Uh oh!

alexcrichton commented Jan 22, 2016

Uh oh!

bors commented Jan 23, 2016

Uh oh!

bors commented Jan 23, 2016

Uh oh!

hudson-ayers commented Sep 30, 2021

Uh oh!

Uh oh!