Optimize thread::panicking() #56469

ghost · 2018-12-03T16:53:06Z

Change a thread_local! to faster #[thread_local].

This avoids the useless check of whether the PANIC_COUNT variable has been initialized on every call to PANIC_COUNT.with().

I made additional changes:

Don't increment PANIC_COUNT by 0 in thread::panicking(), just read it.
~~Add a few #[inline]s just to make sure these small functions get inlined.~~

Benchmark that proves replacing thread_local! with #[thread_local] is worth it:

#![feature(test)]
#![feature(thread_local)]

extern crate test;

use test::Bencher;

// Current implementation of `thread::panicking()`.

pub fn update_panic_count1(amt: isize) -> usize {
    use std::cell::Cell;
    thread_local! { static PANIC_COUNT: Cell<usize> = Cell::new(0) }

    PANIC_COUNT.with(|c| {
        let next = (c.get() as isize + amt) as usize;
        c.set(next);
        return next
    })
}

pub fn panicking1() -> bool {
    update_panic_count1(0) != 0
}

#[bench]
fn test1(b: &mut Bencher) {
    b.iter(|| {
        ::test::black_box(panicking1());
    });
}

// Same implementation with `#[thread_local]` instead of `thread_local!`.

pub fn update_panic_count2(amt: isize) -> usize {
    use std::cell::Cell;

    #[thread_local]
    static PANIC_COUNT: Cell<usize> = Cell::new(0);

    let next = (PANIC_COUNT.get() as isize + amt) as usize;
    PANIC_COUNT.set(next);
    next
}

pub fn panicking2() -> bool {
    update_panic_count2(0) != 0
}

#[bench]
fn test2(b: &mut Bencher) {
    b.iter(|| {
        ::test::black_box(panicking2());
    });
}

Results (on my x86-64 machines):

test test1 ... bench:           2 ns/iter (+/- 0)
test test2 ... bench:           0 ns/iter (+/- 0)

cc @Amanieu - yay, faster mutex poisoning?
r? @alexcrichton

sfackler · 2018-12-03T17:17:31Z

IIRC #[thread_local] is broken on some platforms, like Windows MSVC: #44391

ghost · 2018-12-03T18:06:54Z

@sfackler I suppose it's only broken when combined with inlining?

@eddyb Any thoughts on this? Should I just remove the #[inline]s introduced in this PR and call it a day?

eddyb · 2018-12-03T18:12:48Z

@stjepang Looking back at #43931, I'd say that's it, yes, no #[inline] on the function directly accessing the #[thread_local] static.

ghost · 2018-12-03T18:20:57Z

@eddyb All right, I removed #[inline] attributes and added a comment saying why.

ghost · 2018-12-03T18:23:29Z

@Amanieu ran the benchmark from the original comment on an aarch64 machine and got:

test1: 4 ns/iter
test2: 1 ns/iter

ghost · 2018-12-03T18:59:23Z

@eddyb I wonder, should we perhaps mark functions accessing a #[thread_local] with #[inline(never)]? If so, should we do the same to function __getit() in the implementation of thread_local!?

rust-highfive · 2018-12-03T20:02:53Z

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:0588bd14:start=1543861321680735937,finish=1543861325488020669,duration=3807284732
$ 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
---
travis_time:start:test_codegen
Check compiletest suite=codegen mode=codegen (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[00:52:33] 
[00:52:33] running 120 tests
[00:52:36] i..ii...iii..iiii.....i...i..........i..iii.............i.....i.....ii...i..i.ii..............i...ii 100/120
[00:52:36] ..ii.i.....iiii.....
[00:52:36] 
[00:52:36]  finished in 3.267
[00:52:36] travis_fold:end:test_codegen

---
travis_time:start:test_debuginfo
Check compiletest suite=debuginfo mode=debuginfo-both (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[00:52:50] 
[00:52:50] running 118 tests
[00:53:12] .iiiii...i.....i..i...i..i.i..i.i..i.....i..i....i..........iiii.........i.i....i...i.......ii.i.i.i 100/118
[00:53:16] ......iii.i.....ii
[00:53:16] 
[00:53:16]  finished in 25.902
[00:53:16] travis_fold:end:test_debuginfo

---
[01:09:34] thread '<unnamed>' panicked at 'called `Result::unwrap()` on an `Err` value: RecvError', src/libcore/result.rs:1009:5
[01:09:34] .................................................................................................... 600/768
[01:09:35] thread '<unnamed>' panicked at 'explicit panic', src/libstd/sync/mutex.rs:637:13
[01:09:35] thread '<unnamed>' panicked at 'test panic in inner thread to poison mutex', src/libstd/sync/mutex.rs:592:13
[01:09:35] thread '<unnamed>' panicked at 'assertion failed: m.is_poisoned()', src/libstd/sync/mutex.rs:595:9
[01:09:35] thread '<unnamed>' panicked at 'test panic in inner thread to poison mutex', src/libstd/sync/mutex.rs:569:13
[01:09:35] thread '<unnamed>' panicked at 'assertion failed: m.is_poisoned()', src/libstd/sync/mutex.rs:572:9
[01:09:35] thread '<unnamed>' panicked at 'assertion failed: `(left == right)`
[01:09:35]   left: `1`,
[01:09:35]  right: `2`', src/libstd/sync/mutex.rs:661:13
[01:09:35]  right: `2`', src/libstd/sync/mutex.rs:661:13
[01:09:35] thread '<unnamed>' panicked at 'assertion failed: arc.lock().is_err()', src/libstd/sync/mutex.rs:663:9
[01:09:35] thread '<unnamed>' panicked at 'Once instance has previously been poisoned', src/libstd/sync/once.rs:372:21
[01:09:35] thread '<unnamed>' panicked at 'explicit panic', src/libstd/sync/once.rs:610:28
[01:09:35] thread '<unnamed>' panicked at 'test panic in inner thread to poison RwLock', src/libstd/sync/rwlock.rs:797:13
[01:09:35] thread '<unnamed>' panicked at 'test panic in inner thread to poison RwLock', src/libstd/sync/rwlock.rs:797:13
[01:09:35] thread '<unnamed>' panicked at 'assertion failed: m.is_poisoned()', src/libstd/sync/rwlock.rs:800:9
[01:09:35] thread '<unnamed>' panicked at 'test panic in inner thread to poison RwLock', src/libstd/sync/rwlock.rs:774:13
[01:09:35] thread '<unnamed>' panicked at 'assertion failed: m.is_poisoned()', src/libstd/sync/rwlock.rs:777:9
[01:09:35] thread '<unnamed>' panicked at 'explicit panic', src/libstd/sync/rwlock.rs:641:13
[01:09:35] thread '<unnamed>' panicked at 'explicit panic', src/libstd/sync/rwlock.rs:652:13
[01:09:35] thread '<unnamed>' panicked at 'explicit panic', src/libstd/sync/rwlock.rs:617:13
[01:09:35] thread '<unnamed>' panicked at 'explicit panic', src/libstd/sync/rwlock.rs:617:13
[01:09:35] thread '<unnamed>' panicked at 'assertion failed: arc.read().is_err()', src/libstd/sync/rwlock.rs:619:9
[01:09:35] thread '<unnamed>' panicked at 'explicit panic', src/libstd/sync/rwlock.rs:629:13
[01:09:35] thread '<unnamed>' panicked at 'assertion failed: arc.write().is_err()', src/libstd/sync/rwlock.rs:631:9
[01:09:36] thread '<unnamed>' panicked at 'What the answer to my lifetimes dilemma is?', src/libstd/sys_common/remutex.rs:241:13
[01:09:36] ..........................F..F...F.........F..F....FF..............................F................ 700/768
[01:09:36] thread '<unnamed>' panicked at 'index 2 and/or 4 in `"aé 💩"` do not lie on character boundary', src/libstd/sys_common/wtf8.rs:784:5
[01:09:36] thread '<unnamed>' panicked at 'index 5 and/or 8 in `"aé 💩"` do not lie on character boundary', src/libstd/sys_common/wtf8.rs:784:5
[01:09:36] thread '<unnamed>' panicked at 'assertion failed: is_code_point_boundary(self, new_len)', src/libstd/sys_common/wtf8.rs:329:9
[01:09:36] thread '<unnamed>' panicked at 'assertion failed: is_code_point_boundary(self, new_len)', src/libstd/sys_common/wtf8.rs:329:9

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 · 2018-12-03T22:05:32Z

Ah yeah #[thread_local] isn't as portable as thread_local!(...) (as @sfackler mentioned for MSVC, it's also for OSX 10.6, things like wasm, probably other architectures, etc). Additionally the lack of #[inline] here was actually to improve code generation historically! (that may have changed now though)

rust-highfive · 2018-12-04T17:29:25Z

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:2f0a4f2e:start=1543937980588088074,finish=1543938069088120753,duration=88500032679
$ 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
---
travis_time:start:test_codegen
Check compiletest suite=codegen mode=codegen (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[00:57:54] 
[00:57:54] running 120 tests
[00:57:57] i..ii...iii..iiii.....i...i..........i..iii.............i.....i.....ii...i..i.ii..............i...ii 100/120
[00:57:57] ..ii.i.....iiii.....
[00:57:57] 
[00:57:57]  finished in 3.587
[00:57:57] travis_fold:end:test_codegen

---
travis_time:start:test_debuginfo
Check compiletest suite=debuginfo mode=debuginfo-both (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[00:58:13] 
[00:58:13] running 118 tests
[00:58:39] .iiiii...i.....i..i...i..i.i..i.i..i.....i..i....i..........iiii.........i.i....i...i.......ii.i.i.i 100/118
[00:58:43] ......iii.i.....ii
[00:58:43] 
[00:58:43]  finished in 30.396
[00:58:43] travis_fold:end:test_debuginfo

---
[01:17:03] thread '<unnamed>' panicked at 'called `Result::unwrap()` on an `Err` value: RecvError', src/libcore/result.rs:1009:5
[01:17:03] .................................................................................................... 600/768
[01:17:03] thread '<unnamed>' panicked at 'explicit panic', src/libstd/sync/mutex.rs:637:13
[01:17:03] thread '<unnamed>' panicked at 'test panic in inner thread to poison mutex', src/libstd/sync/mutex.rs:592:13
[01:17:03] thread '<unnamed>' panicked at 'assertion failed: m.is_poisoned()', src/libstd/sync/mutex.rs:595:9
[01:17:03] thread '<unnamed>' panicked at 'test panic in inner thread to poison mutex', src/libstd/sync/mutex.rs:569:13
[01:17:03] thread '<unnamed>' panicked at 'assertion failed: m.is_poisoned()', src/libstd/sync/mutex.rs:572:9
[01:17:03] thread '<unnamed>' panicked at 'assertion failed: `(left == right)`
[01:17:03]   left: `1`,
[01:17:03]  right: `2`', src/libstd/sync/mutex.rs:661:13
[01:17:03]  right: `2`', src/libstd/sync/mutex.rs:661:13
[01:17:03] thread '<unnamed>' panicked at 'assertion failed: arc.lock().is_err()', src/libstd/sync/mutex.rs:663:9
[01:17:03] thread '<unnamed>' panicked at 'Once instance has previously been poisoned', src/libstd/sync/once.rs:372:21
[01:17:03] thread '<unnamed>' panicked at 'explicit panic', src/libstd/sync/once.rs:610:28
[01:17:03] thread '<unnamed>' panicked at 'test panic in inner thread to poison RwLock', src/libstd/sync/rwlock.rs:797:13
[01:17:03] thread '<unnamed>' panicked at 'test panic in inner thread to poison RwLock', src/libstd/sync/rwlock.rs:797:13
[01:17:03] thread '<unnamed>' panicked at 'assertion failed: m.is_poisoned()', src/libstd/sync/rwlock.rs:800:9
[01:17:03] thread '<unnamed>' panicked at 'test panic in inner thread to poison RwLock', src/libstd/sync/rwlock.rs:774:13
[01:17:03] thread '<unnamed>' panicked at 'assertion failed: m.is_poisoned()', src/libstd/sync/rwlock.rs:777:9
[01:17:03] thread '<unnamed>' panicked at 'explicit panic', src/libstd/sync/rwlock.rs:641:13
[01:17:03] thread '<unnamed>' panicked at 'explicit panic', src/libstd/sync/rwlock.rs:652:13
[01:17:03] thread '<unnamed>' panicked at 'explicit panic', src/libstd/sync/rwlock.rs:617:13
[01:17:03] thread '<unnamed>' panicked at 'explicit panic', src/libstd/sync/rwlock.rs:617:13
[01:17:03] thread '<unnamed>' panicked at 'assertion failed: arc.read().is_err()', src/libstd/sync/rwlock.rs:619:9
[01:17:03] thread '<unnamed>' panicked at 'explicit panic', src/libstd/sync/rwlock.rs:629:13
[01:17:03] thread '<unnamed>' panicked at 'assertion failed: arc.write().is_err()', src/libstd/sync/rwlock.rs:631:9
[01:17:04] thread '<unnamed>' panicked at 'What the answer to my lifetimes dilemma is?', src/libstd/sys_common/remutex.rs:241:13
[01:17:04] ..........................F..F...F.........F..F....FF..............................F................ 700/768
[01:17:04] thread '<unnamed>' panicked at 'index 2 and/or 4 in `"aé 💩"` do not lie on character boundary', src/libstd/sys_common/wtf8.rs:784:5
[01:17:04] thread '<unnamed>' panicked at 'index 5 and/or 8 in `"aé 💩"` do not lie on character boundary', src/libstd/sys_common/wtf8.rs:784:5
[01:17:04] thread '<unnamed>' panicked at 'assertion failed: is_code_point_boundary(self, new_len)', src/libstd/sys_common/wtf8.rs:329:9
[01:17:04] thread '<unnamed>' panicked at 'assertion failed: is_code_point_boundary(self, new_len)', src/libstd/sys_common/wtf8.rs:329:9

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)

rust-highfive · 2018-12-04T18:59:28Z

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:08596814:start=1543945657198482195,finish=1543945719534306199,duration=62335824004
$ 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
---
travis_time:start:test_codegen
Check compiletest suite=codegen mode=codegen (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[00:54:13] 
[00:54:13] running 120 tests
[00:54:16] i..ii...iii..iiii.....i...i..........i..iii.............i.....i.....ii...i..i.ii..............i...ii 100/120
[00:54:16] ..ii.i.....iiii.....
[00:54:16] 
[00:54:16]  finished in 3.325
[00:54:16] travis_fold:end:test_codegen

---
travis_time:start:test_debuginfo
Check compiletest suite=debuginfo mode=debuginfo-both (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[00:54:30] 
[00:54:30] running 118 tests
[00:54:52] .iiiii...i.....i..i...i..i.i..i.i..i.....i..i....i..........iiii.........i.i....i...i.......ii.i.i.i 100/118
[00:54:56] ......iii.i.....ii
[00:54:56] 
[00:54:56]  finished in 25.714
[00:54:56] travis_fold:end:test_debuginfo

---
[01:10:08] travis_fold:start:test_stage1-std
travis_time:start:test_stage1-std
Testing std stage1 (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[01:10:08]    Compiling std v0.0.0 (/checkout/src/libstd)
[01:10:31] error: function is never used: `with_panic_count`
[01:10:31]     |
[01:10:31]     |
[01:10:31] 232 | fn with_panic_count<R>(f: impl FnOnce(&Cell<usize>) -> R) -> R {
[01:10:31]     |
[01:10:31]     = note: `-D dead-code` implied by `-D warnings`
[01:10:31] 
[01:10:32] error: aborting due to previous error
[01:10:32] error: aborting due to previous error
[01:10:32] 
[01:10:32] error: Could not compile `std`.
[01:10:32] 
[01:10:32] To learn more, run the command again with --verbose.
[01:10:32] 
[01:10:32] 
[01:10:32] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "test" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--features" "panic-unwind backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "-p" "std" "--" "--quiet"
[01:10:32] 
[01:10:32] 
[01:10:32] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[01:10:32] Build completed unsuccessfully in 0:26:40
[01:10:32] Build completed unsuccessfully in 0:26:40
[01:10:32] make: *** [check] Error 1
[01:10:32] Makefile:58: recipe for target 'check' failed
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:0ec432d7
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Tue Dec  4 18:59:20 UTC 2018
---
travis_time:end:02a809b2:start=1543949962367072489,finish=1543949962372219194,duration=5146705
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:05dacfd7
$ 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:0be5c1a4
travis_time:start:0be5c1a4
$ 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:036fe9de
$ 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 · 2018-12-04T19:16:15Z

That looks like a reasonable strategy to me! This makes me think though that we may want to extend the thread_local! macro or add a new macro though. I suspect this isn't the only use case where thread_local! has a constant initializer where the branch on fetch wants to be optimized away.

I think that if we remove the branch the current thread_local! will optimize to desired implementation, so we largely just need to figure out how to remove the Option in initialization (perhaps when there's no dtor as well).

I suspect that'd be nice to have for more use cases eventually than just this one panic count case, and we could probably use it in a few places in libstd!

rust-highfive · 2018-12-04T20:24:25Z

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:0b3bda27:start=1543950677488543630,finish=1543950732835752600,duration=55347208970
$ 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
---
travis_time:start:test_codegen
Check compiletest suite=codegen mode=codegen (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[00:55:08] 
[00:55:08] running 120 tests
[00:55:11] i..ii...iii..iiii.....i...i..........i..iii.............i.....i.....ii...i..i.ii..............i...ii 100/120
[00:55:11] ..ii.i.....iiii.....
[00:55:11] 
[00:55:11]  finished in 3.417
[00:55:11] travis_fold:end:test_codegen

---
travis_time:start:test_debuginfo
Check compiletest suite=debuginfo mode=debuginfo-both (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[00:55:26] 
[00:55:26] running 118 tests
[00:55:51] .iiiii...i.....i..i...i..i.i..i.i..i.....i..i....i..........iiii.........i.i....i...i.......ii.i.i.i 100/118
[00:55:55] ......iii.i.....ii
[00:55:55] 
[00:55:55]  finished in 29.721
[00:55:55] travis_fold:end:test_debuginfo

---
[01:11:41]    Compiling std v0.0.0 (/checkout/src/libstd)
[01:11:45] error: unused import: `Cell`
[01:11:45]   --> src/libstd/panicking.rs:25:12
[01:11:45]    |
[01:11:45] 25 | use cell::{Cell, RefCell};
[01:11:45]    |
[01:11:45]    = note: `-D unused-imports` implied by `-D warnings`
[01:11:45] 
travis_time:end:02e8a810:start=1543950741895192427,finish=1543955059019440207,duration=4317124247780
---
travis_time:end:034f1f70:start=1543955061691907135,finish=1543955061793721149,duration=101814014
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:08a2ba78
$ 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:30641268
$ 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)

ghost · 2018-12-04T21:18:55Z

I'm not sure if this PR is worth it anymore... :( It seems it reduces the time needed to lock a Mutex from 23 ns to 22 ns, which is not that much of a win.

@alexcrichton

This makes me think though that we may want to extend the thread_local! macro or add a new macro though.

Yeah, perhaps that might be a good thing to do! We could have something like the following syntax for constant-initialized thread-locals:

thread_local! {
    static ref FOO: Foo = const { Foo };
}

Or do you think we'd do that somehow differently?

Amanieu · 2018-12-04T21:21:41Z

I'm not sure if this PR is worth it anymore... :( It seems it reduces the time needed to lock a Mutex from 23 ns to 22 ns, which is not that much of a win.

parking_lot is going to replace the standard Mutex type soon: #56410. You might see a bigger win once that is merged and we can actually inline the entire lock/unlock fast paths.

ghost · 2018-12-04T21:29:01Z

@Amanieu All right, I'll benchmark again once that gets merged.

Oh, and I also just realized we call thread::panicking() twice per Mutex::lock() - before and after locking. It'd probably be wise to grab a reference to the thread-local Cell<usize> once per lock() in order to avoid hitting TLS twice.

Also, the poison flag is an AtomicBool per every Mutex instance. I wonder if we could squeeze the poison bit somewhere inside parking_lot's internal state.

alexcrichton · 2018-12-06T14:37:03Z

Ah ok! @stjepang should we perhaps close this and wait for the parking lot PR instead then?

ghost · 2018-12-06T14:42:50Z

Sure! Closing.

Optimize thread::panicking()

42dba9d

rust-highfive assigned alexcrichton Dec 3, 2018

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

Remove #[inline] attributes

7dce3dc

Use thread_local! on platforms that don't support #[thread_local]

c092af9

Revert changes in rust_begin_panic

b48a110

Disable with_panic_count in test mode

bc0de72

Fix unused warning

81c9f8a

ghost closed this Dec 6, 2018

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize thread::panicking() #56469

Optimize thread::panicking() #56469

ghost commented Dec 3, 2018 •

edited by ghost

Loading

sfackler commented Dec 3, 2018

ghost commented Dec 3, 2018

eddyb commented Dec 3, 2018

ghost commented Dec 3, 2018

ghost commented Dec 3, 2018 •

edited by ghost

Loading

ghost commented Dec 3, 2018

rust-highfive commented Dec 3, 2018

alexcrichton commented Dec 3, 2018

rust-highfive commented Dec 4, 2018

rust-highfive commented Dec 4, 2018

alexcrichton commented Dec 4, 2018

rust-highfive commented Dec 4, 2018

ghost commented Dec 4, 2018

Amanieu commented Dec 4, 2018

ghost commented Dec 4, 2018 •

edited by ghost

Loading

alexcrichton commented Dec 6, 2018

ghost commented Dec 6, 2018

Optimize thread::panicking() #56469

Optimize thread::panicking() #56469

Conversation

ghost commented Dec 3, 2018 • edited by ghost Loading

sfackler commented Dec 3, 2018

ghost commented Dec 3, 2018

eddyb commented Dec 3, 2018

ghost commented Dec 3, 2018

ghost commented Dec 3, 2018 • edited by ghost Loading

ghost commented Dec 3, 2018

rust-highfive commented Dec 3, 2018

alexcrichton commented Dec 3, 2018

rust-highfive commented Dec 4, 2018

rust-highfive commented Dec 4, 2018

alexcrichton commented Dec 4, 2018

rust-highfive commented Dec 4, 2018

ghost commented Dec 4, 2018

Amanieu commented Dec 4, 2018

ghost commented Dec 4, 2018 • edited by ghost Loading

alexcrichton commented Dec 6, 2018

ghost commented Dec 6, 2018

ghost commented Dec 3, 2018 •

edited by ghost

Loading

ghost commented Dec 3, 2018 •

edited by ghost

Loading

ghost commented Dec 4, 2018 •

edited by ghost

Loading