-
Notifications
You must be signed in to change notification settings - Fork 14.1k
Improve TLS codegen by marking the panic/init path as cold #143511
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,7 +4,7 @@ use crate::sys::thread_local::{abort_on_dtor_unwind, destructors}; | |
|
|
||
| #[derive(Clone, Copy)] | ||
| enum State { | ||
| Initial, | ||
| Uninitialized, | ||
| Alive, | ||
| Destroyed, | ||
| } | ||
|
|
@@ -17,7 +17,7 @@ pub struct Storage<T> { | |
|
|
||
| impl<T> Storage<T> { | ||
| pub const fn new(val: T) -> Storage<T> { | ||
| Storage { state: Cell::new(State::Initial), val: UnsafeCell::new(val) } | ||
| Storage { state: Cell::new(State::Uninitialized), val: UnsafeCell::new(val) } | ||
| } | ||
|
|
||
| /// Gets a pointer to the TLS value. If the TLS variable has been destroyed, | ||
|
|
@@ -30,16 +30,22 @@ impl<T> Storage<T> { | |
| /// The `self` reference must remain valid until the TLS destructor is run. | ||
| #[inline] | ||
| pub unsafe fn get(&self) -> *const T { | ||
| match self.state.get() { | ||
| State::Alive => self.val.get(), | ||
| State::Destroyed => ptr::null(), | ||
| State::Initial => unsafe { self.initialize() }, | ||
| if let State::Alive = self.state.get() { | ||
| self.val.get() | ||
| } else { | ||
| unsafe { self.get_or_init_slow() } | ||
|
Comment on lines
+33
to
+36
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this is beneficial – the returned pointer is later compared against null in |
||
| } | ||
| } | ||
|
|
||
| #[cold] | ||
| unsafe fn initialize(&self) -> *const T { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be marked as
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had that in an earlier PR, see #143511 (comment). |
||
| // Register the destructor | ||
| unsafe fn get_or_init_slow(&self) -> *const T { | ||
| match self.state.get() { | ||
| State::Uninitialized => {} | ||
| State::Alive => return self.val.get(), | ||
| State::Destroyed => return ptr::null(), | ||
| } | ||
|
|
||
| // Register the destructor. | ||
|
|
||
| // SAFETY: | ||
| // The caller guarantees that `self` will be valid until thread destruction. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -68,6 +68,7 @@ impl<T: 'static> Storage<T> { | |
| /// | ||
| /// The resulting pointer may not be used after reentrant inialialization | ||
| /// or thread destruction has occurred. | ||
| #[inline] | ||
| pub fn get(&'static self, i: Option<&mut Option<T>>, f: impl FnOnce() -> T) -> *const T { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While you're at it, I think it might be beneficial to inline the |
||
| let key = self.key.force(); | ||
| let ptr = unsafe { get(key) as *mut Value<T> }; | ||
|
|
@@ -84,6 +85,7 @@ impl<T: 'static> Storage<T> { | |
| /// # Safety | ||
| /// * `key` must be the result of calling `self.key.force()` | ||
| /// * `ptr` must be the current value associated with `key`. | ||
| #[cold] | ||
| unsafe fn try_initialize( | ||
| key: Key, | ||
| ptr: *mut Value<T>, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uninitializedisn't a great name, the value itself is very much initialised. It's just the destructor that's not registered yet.