restructure thread_local! for better codegen (especially on macos)

src/libstd/thread/local.rs

@@ -2,10 +2,7 @@
 
 #![unstable(feature = "thread_local_internals", issue = "0")]
 
-use crate::cell::UnsafeCell;
 use crate::fmt;
-use crate::hint;
-use crate::mem;
 
 /// A thread local storage key which owns its contents.
 ///
@@ -92,10 +89,7 @@ pub struct LocalKey<T: 'static> {
     // trivially devirtualizable by LLVM because the value of `inner` never
     // changes and the constant should be readonly within a crate. This mainly
     // only runs into problems when TLS statics are exported across crates.
-    inner: unsafe fn() -> Option<&'static UnsafeCell<Option<T>>>,
+    inner: unsafe fn() -> Option<&'static T>,
-
-    // initialization routine to invoke to create a value
-    init: fn() -> T,
 }
 
 #[stable(feature = "std_debug", since = "1.16.0")]
@@ -159,10 +153,7 @@ macro_rules! __thread_local_inner {
             #[inline]
             fn __init() -> $t { $init }
 
-            unsafe fn __getit() -> $crate::option::Option<
+            unsafe fn __getit() -> $crate::option::Option<&'static $t> {
-                &'static $crate::cell::UnsafeCell<
-                    $crate::option::Option<$t>>>
-            {
                 #[cfg(all(target_arch = "wasm32", not(target_feature = "atomics")))]
                 static __KEY: $crate::thread::__StaticLocalKeyInner<$t> =
                     $crate::thread::__StaticLocalKeyInner::new();
@@ -182,11 +173,11 @@ macro_rules! __thread_local_inner {
                 static __KEY: $crate::thread::__OsLocalKeyInner<$t> =
                     $crate::thread::__OsLocalKeyInner::new();
 
-                __KEY.get()
+                __KEY.get(__init)
             }
 
             unsafe {
-                $crate::thread::LocalKey::new(__getit, __init)
+                $crate::thread::LocalKey::new(__getit)
             }
         }
     };
@@ -221,11 +212,9 @@ impl<T: 'static> LocalKey<T> {
     #[unstable(feature = "thread_local_internals",
                reason = "recently added to create a key",
                issue = "0")]
-    pub const unsafe fn new(inner: unsafe fn() -> Option<&'static UnsafeCell<Option<T>>>,
+    pub const unsafe fn new(inner: unsafe fn() -> Option<&'static T>) -> LocalKey<T> {
-                            init: fn() -> T) -> LocalKey<T> {
         LocalKey {
             inner,
-            init,
         }
     }
 
@@ -246,11 +235,56 @@ impl<T: 'static> LocalKey<T> {
                                  after it is destroyed")
     }
 
-    unsafe fn init(&self, slot: &UnsafeCell<Option<T>>) -> &T {
+    /// Acquires a reference to the value in this TLS key.
+    ///
+    /// This will lazily initialize the value if this thread has not referenced
+    /// this key yet. If the key has been destroyed (which may happen if this is called
+    /// in a destructor), this function will return an [`AccessError`](struct.AccessError.html).
+    ///
+    /// # Panics
+    ///
+    /// This function will still `panic!()` if the key is uninitialized and the
+    /// key's initializer panics.
+    #[stable(feature = "thread_local_try_with", since = "1.26.0")]
+    pub fn try_with<F, R>(&'static self, f: F) -> Result<R, AccessError>
+    where
+        F: FnOnce(&T) -> R,
+    {
+        unsafe {
+            let thread_local = (self.inner)().ok_or(AccessError {
+                _private: (),
+            })?;
+            Ok(f(thread_local))
+        }
+    }
+}
+
+mod lazy {
+    use crate::cell::UnsafeCell;
+    use crate::mem;
+    use crate::hint;
+
+    pub struct LazyKeyInner<T> {
+        inner: UnsafeCell<Option<T>>,
+    }
+
+    impl<T> LazyKeyInner<T> {
+        pub const fn new() -> LazyKeyInner<T> {
+            LazyKeyInner {
+                inner: UnsafeCell::new(None),
+            }
+        }
+
+        #[inline]
+        pub unsafe fn get(&self) -> Option<&'static T> {
+            (*self.inner.get()).as_ref()
+        }
+
+        pub unsafe fn initialize<F: FnOnce() -> T>(&self, init: F) -> &'static T {
             // Execute the initialization up front, *then* move it into our slot,
             // just in case initialization fails.
-        let value = (self.init)();
+            let value = init();
-        let ptr = slot.get();
+            let ptr = self.inner.get();
 
             // note that this can in theory just be `*ptr = Some(value)`, but due to
             // the compiler will currently codegen that pattern with something like:
@@ -277,29 +311,8 @@ impl<T: 'static> LocalKey<T> {
             }
         }
 
-    /// Acquires a reference to the value in this TLS key.
+        pub unsafe fn take(&mut self) -> Option<T> {
-    ///
+            (*self.inner.get()).take()
-    /// This will lazily initialize the value if this thread has not referenced
-    /// this key yet. If the key has been destroyed (which may happen if this is called
-    /// in a destructor), this function will return an [`AccessError`](struct.AccessError.html).
-    ///
-    /// # Panics
-    ///
-    /// This function will still `panic!()` if the key is uninitialized and the
-    /// key's initializer panics.
-    #[stable(feature = "thread_local_try_with", since = "1.26.0")]
-    pub fn try_with<F, R>(&'static self, f: F) -> Result<R, AccessError>
-    where
-        F: FnOnce(&T) -> R,
-    {
-        unsafe {
-            let slot = (self.inner)().ok_or(AccessError {
-                _private: (),
-            })?;
-            Ok(f(match *slot.get() {
-                Some(ref inner) => inner,
-                None => self.init(slot),
-            }))
         }
     }
 }
@@ -309,11 +322,12 @@ impl<T: 'static> LocalKey<T> {
 #[doc(hidden)]
 #[cfg(all(target_arch = "wasm32", not(target_feature = "atomics")))]
 pub mod statik {
+    use super::lazy::LazyKeyInner;
     use crate::cell::UnsafeCell;
     use crate::fmt;
 
     pub struct Key<T> {
-        inner: UnsafeCell<Option<T>>,
+        inner: LazyKeyInner<T>,
     }
 
     unsafe impl<T> Sync for Key<T> { }
@@ -327,12 +341,17 @@ pub mod statik {
     impl<T> Key<T> {
         pub const fn new() -> Key<T> {
             Key {
-                inner: UnsafeCell::new(None),
+                inner: LazyKeyInner::new(),
             }
         }
 
-        pub unsafe fn get(&self) -> Option<&'static UnsafeCell<Option<T>>> {
+        #[inline]
-            Some(&*(&self.inner as *const _))
+        pub unsafe fn get(&self, init: fn() -> T) -> Option<&'static T> {
+            let value = match self.inner.get() {
+                Some(ref value) => value,
+                None => self.inner.initialize(init),
+            };
+            Some(value)
         }
     }
 }
@@ -340,19 +359,33 @@ pub mod statik {
 #[doc(hidden)]
 #[cfg(target_thread_local)]
 pub mod fast {
-    use crate::cell::{Cell, UnsafeCell};
+    use super::lazy::LazyKeyInner;
+    use crate::cell::Cell;
     use crate::fmt;
     use crate::mem;
-    use crate::ptr;
     use crate::sys::fast_thread_local::register_dtor;
 
+    #[derive(Copy, Clone)]
+    enum DtorState {
+        Unregistered,
+        Registered,
+        RunningOrHasRun,
+    }
+
     pub struct Key<T> {
-        inner: UnsafeCell<Option<T>>,
+        // If `LazyKeyInner::get` returns `None`, that indicates either:
+        //   * The value has never been initialized
+        //   * The value is being recursively initialized
+        //   * The value has already been destroyed or is being destroyed
+        // To determine which kind of `None`, check `dtor_state`.
+        //
+        // This is very optimizer friendly for the fast path - initialized but
+        // not yet dropped.
+        inner: LazyKeyInner<T>,
 
         // Metadata to keep track of the state of the destructor. Remember that
-        // these variables are thread-local, not global.
+        // this variable is thread-local, not global.
-        dtor_registered: Cell<bool>,
+        dtor_state: Cell<DtorState>,
-        dtor_running: Cell<bool>,
     }
 
     impl<T> fmt::Debug for Key<T> {
@@ -364,45 +397,84 @@ pub mod fast {
     impl<T> Key<T> {
         pub const fn new() -> Key<T> {
             Key {
-                inner: UnsafeCell::new(None),
+                inner: LazyKeyInner::new(),
-                dtor_registered: Cell::new(false),
+                dtor_state: Cell::new(DtorState::Unregistered),
-                dtor_running: Cell::new(false)
             }
         }
 
-        pub unsafe fn get(&self) -> Option<&'static UnsafeCell<Option<T>>> {
+        #[inline]
-            if mem::needs_drop::<T>() && self.dtor_running.get() {
+        pub unsafe fn get<F: FnOnce() -> T>(&self, init: F) -> Option<&'static T> {
-                return None
+            match self.inner.get() {
+                Some(val) => Some(val),
+                None => {
+                    if mem::needs_drop::<T>() {
+                        self.try_initialize_drop(init)
+                    } else {
+                        Some(self.try_initialize_nodrop(init))
+                    }
+                }
             }
-            self.register_dtor();
-            Some(&*(&self.inner as *const _))
         }
 
-        unsafe fn register_dtor(&self) {
+        // `try_initialize_nodrop` is only called once per fast thread local
-            if !mem::needs_drop::<T>() || self.dtor_registered.get() {
+        // variable, except in corner cases where it is being recursively
-                return
+        // initialized.
+        //
+        // Macos: Inlining this function causes two `tlv_get_addr` calls to be
+        // performed for every call to `Key::get`.
+        // LLVM issue: https://bugs.llvm.org/show_bug.cgi?id=41722
+        #[inline(never)]
+        #[cold]
+        unsafe fn try_initialize_nodrop<F: FnOnce() -> T>(&self, init: F) -> &'static T {
+            self.inner.initialize(init)
         }
 
+        // `try_initialize_drop` is only called once per fast thread local
+        // variable, except in corner cases where thread_local dtors reference
+        // other thread_local's, or it is being recursively initialized.
+        #[inline(never)]
+        #[cold]
+        unsafe fn try_initialize_drop<F: FnOnce() -> T>(&self, init: F) -> Option<&'static T> {
+            // We don't put a `needs_drop` check around this and call it a day
+            // because this function is not inlined. Unwrapping code gets
+            // generated for callers of `LocalKey::with` even if we always
+            // return `Some` here.
+            match self.dtor_state.get() {
+                DtorState::Unregistered => {
+                    // dtor registration happens before initialization.
                     register_dtor(self as *const _ as *mut u8,
                                 destroy_value::<T>);
-            self.dtor_registered.set(true);
+                    self.dtor_state.set(DtorState::Registered);
+                }
+                DtorState::Registered => {
+                    // recursively initialized
+                }
+                DtorState::RunningOrHasRun => {
+                    return None
+                }
+            }
+
+            Some(self.inner.initialize(init))
         }
     }
 
     unsafe extern fn destroy_value<T>(ptr: *mut u8) {
         let ptr = ptr as *mut Key<T>;
-        // Right before we run the user destructor be sure to flag the
-        // destructor as running for this thread so calls to `get` will return
-        // `None`.
-        (*ptr).dtor_running.set(true);
 
-        ptr::drop_in_place((*ptr).inner.get());
+        // Right before we run the user destructor be sure to set the
+        // `Option<T>` to `None`, and `dtor_state` to `RunningOrHasRun`. This
+        // causes future calls to `get` to run `try_initialize_drop` again,
+        // which will now fail, and return `None`.
+        let value = (*ptr).inner.take();
+        (*ptr).dtor_state.set(DtorState::RunningOrHasRun);
+        drop(value);
     }
 }
 
 #[doc(hidden)]
 pub mod os {
-    use crate::cell::{Cell, UnsafeCell};
+    use super::lazy::LazyKeyInner;
+    use crate::cell::Cell;
     use crate::fmt;
     use crate::marker;
     use crate::ptr;
@@ -423,8 +495,8 @@ pub mod os {
     unsafe impl<T> Sync for Key<T> { }
 
     struct Value<T: 'static> {
+        inner: LazyKeyInner<T>,
         key: &'static Key<T>,
-        value: UnsafeCell<Option<T>>,
     }
 
     impl<T: 'static> Key<T> {
@@ -435,24 +507,43 @@ pub mod os {
             }
         }
 
-        pub unsafe fn get(&'static self) -> Option<&'static UnsafeCell<Option<T>>> {
+        pub unsafe fn get(&'static self, init: fn() -> T) -> Option<&'static T> {
             let ptr = self.os.get() as *mut Value<T>;
-            if !ptr.is_null() {
+            if ptr as usize > 1 {
-                if ptr as usize == 1 {
+                match (*ptr).inner.get() {
-                    return None
+                    Some(ref value) => return Some(value),
+                    None => {},
                 }
-                return Some(&(*ptr).value);
+            }
+            self.try_initialize(init)
         }
 
+        // `try_initialize` is only called once per os thread local variable,
+        // except in corner cases where thread_local dtors reference other
+        // thread_local's, or it is being recursively initialized.
+        unsafe fn try_initialize(&'static self, init: fn() -> T) -> Option<&'static T> {
+            let ptr = self.os.get() as *mut Value<T>;
+            if ptr as usize == 1 {
+                // destructor is running
+                return None
+            }
+
+            let ptr = if ptr.is_null() {
                 // If the lookup returned null, we haven't initialized our own
                 // local copy, so do that now.
                 let ptr: Box<Value<T>> = box Value {
+                    inner: LazyKeyInner::new(),
                     key: self,
-                value: UnsafeCell::new(None),
                 };
                 let ptr = Box::into_raw(ptr);
                 self.os.set(ptr as *mut u8);
-            Some(&(*ptr).value)
+                ptr
+            } else {
+                // recursive initialization
+                ptr
+            };
+
+            Some((*ptr).inner.initialize(init))
         }
     }
 

src/test/ui/issues/issue-43733.rs

@@ -13,11 +13,9 @@ static __KEY: std::thread::__FastLocalKeyInner<Foo> =
 static __KEY: std::thread::__OsLocalKeyInner<Foo> =
     std::thread::__OsLocalKeyInner::new();
 
-fn __getit() -> std::option::Option<
+fn __getit(init: fn() -> Foo) -> std::option::Option<&'static Foo>
-    &'static std::cell::UnsafeCell<
-        std::option::Option<Foo>>>
 {
-    __KEY.get() //~ ERROR call to unsafe function is unsafe
+    __KEY.get(init) //~ ERROR call to unsafe function is unsafe
 }
 
 static FOO: std::thread::LocalKey<Foo> =

src/test/ui/issues/issue-43733.stderr

@@ -1,13 +1,13 @@
 error[E0133]: call to unsafe function is unsafe and requires unsafe function or block
-  --> $DIR/issue-43733.rs:20:5
+  --> $DIR/issue-43733.rs:18:5
    |
-LL |     __KEY.get()
+LL |     __KEY.get(init)
-   |     ^^^^^^^^^^^ call to unsafe function
+   |     ^^^^^^^^^^^^^^^ call to unsafe function
    |
    = note: consult the function's documentation for information on how to avoid undefined behavior
 
 error[E0133]: call to unsafe function is unsafe and requires unsafe function or block
-  --> $DIR/issue-43733.rs:24:5
+  --> $DIR/issue-43733.rs:22:5
    |
 LL |     std::thread::LocalKey::new(__getit, Default::default);
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ call to unsafe function