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.

Sign up for GitHub

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

Jump to bottom

Add a fast path for std::thread::panicking. #72617

Merged
merged 2 commits into from
Jun 26, 2020
Merged

Add a fast path for std::thread::panicking. #72617

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f03cf99
Add a fast path for `std::thread::panicking`.
eduardosm May 26, 2020
771a1d8
Make `std::panicking::panic_count::is_zero` inline and move the slow …
eduardosm Jun 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion src/libstd/panicking.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,16 +258,25 @@ pub mod panic_count {
LOCAL_PANIC_COUNT.with(|c| c.get())
}

#[inline]
pub fn is_zero() -> bool {
Copy link
Member

Choose a reason for hiding this comment

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

This function should be #[inline], and a separate #[cold] function should be used for the slow path that is not inlined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should the std::panicking::panicking function be also made #[inline]? Otherwise I'm not sure there will be much benefit.

The current call stack is:

  • std::thread::panicking, which is #[inline]
  • std::panicking::panicking, which is not #[inline]
  • std::panicking::panic_count::is_zero

Copy link
Member

Choose a reason for hiding this comment

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

Ah yes, that should probably be inline too.

if GLOBAL_PANIC_COUNT.load(Ordering::Relaxed) == 0 {
// Fast path: if `GLOBAL_PANIC_COUNT` is zero, all threads
// (including the current one) will have `LOCAL_PANIC_COUNT`
// equal to zero, so TLS access can be avoided.
true
} else {
LOCAL_PANIC_COUNT.with(|c| c.get() == 0)
is_zero_slow_path()
}
}

// Slow path is in a separate function to reduce the amount of code
// inlined from `is_zero`.
#[inline(never)]
Copy link
Member

Choose a reason for hiding this comment

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

#[cold] should be enough, as a general rule we avoid using both cold and inline(never).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did that way because that is how it is done here:

rust/src/libcore/option.rs

Lines 1262 to 1267 in 0b66a89

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

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough, I guess it doesn't really matter in practice.

#[cold]
fn is_zero_slow_path() -> bool {
LOCAL_PANIC_COUNT.with(|c| c.get() == 0)
}
}

#[cfg(test)]
Expand Down Expand Up @@ -350,6 +359,7 @@ pub unsafe fn r#try<R, F: FnOnce() -> R>(f: F) -> Result<R, Box<dyn Any + Send>>
}

/// Determines whether the current thread is unwinding because of panic.
#[inline]
pub fn panicking() -> bool {
!panic_count::is_zero()
}
Expand Down