From 07fd17f7014de6dc209c7dc4de159a2a5acea173 Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Sun, 27 Sep 2020 15:19:42 +0200 Subject: [PATCH 1/3] Only use LOCAL_{STDOUT,STDERR} when set_{print/panic} is used. The thread local LOCAL_STDOUT and LOCAL_STDERR are only used by the test crate to capture output from tests when running them in the same process in differen threads. However, every program will check these variables on every print, even outside of testing. This involves allocating a thread local key, and registering a thread local destructor. This can be somewhat expensive. This change keeps a global flag (LOCAL_STREAMS) which will be set to true when either of these local streams is used. (So, effectively only in test and benchmark runs.) When this flag is off, these thread locals are not even looked at and therefore will not be initialized on the first output on every thread, which also means no thread local destructors will be registered. --- library/std/src/io/stdio.rs | 63 +++++++++++++++++++++++-------------- library/std/src/lib.rs | 1 + 2 files changed, 41 insertions(+), 23 deletions(-) diff --git a/library/std/src/io/stdio.rs b/library/std/src/io/stdio.rs index 61ccc6f13c8..814e0dfda54 100644 --- a/library/std/src/io/stdio.rs +++ b/library/std/src/io/stdio.rs @@ -9,21 +9,24 @@ use crate::cell::RefCell; use crate::fmt; use crate::io::{self, BufReader, Initializer, IoSlice, IoSliceMut, LineWriter}; use crate::lazy::SyncOnceCell; +use crate::sync::atomic::{AtomicBool, Ordering}; use crate::sync::{Mutex, MutexGuard}; use crate::sys::stdio; use crate::sys_common; use crate::sys_common::remutex::{ReentrantMutex, ReentrantMutexGuard}; use crate::thread::LocalKey; +static LOCAL_STREAMS: AtomicBool = AtomicBool::new(false); + thread_local! { - /// Stdout used by print! and println! macros + /// Used by the test crate to capture the output of the print! and println! macros. static LOCAL_STDOUT: RefCell>> = { RefCell::new(None) } } thread_local! { - /// Stderr used by eprint! and eprintln! macros, and panics + /// Used by the test crate to capture the output of the eprint! and eprintln! macros, and panics. static LOCAL_STDERR: RefCell>> = { RefCell::new(None) } @@ -890,10 +893,14 @@ impl fmt::Debug for StderrLock<'_> { #[doc(hidden)] pub fn set_panic(sink: Option>) -> Option> { use crate::mem; - LOCAL_STDERR.with(move |slot| mem::replace(&mut *slot.borrow_mut(), sink)).and_then(|mut s| { - let _ = s.flush(); - Some(s) - }) + let s = LOCAL_STDERR.with(move |slot| mem::replace(&mut *slot.borrow_mut(), sink)).and_then( + |mut s| { + let _ = s.flush(); + Some(s) + }, + ); + LOCAL_STREAMS.store(true, Ordering::Release); + s } /// Resets the thread-local stdout handle to the specified writer @@ -913,10 +920,14 @@ pub fn set_panic(sink: Option>) -> Option>) -> Option> { use crate::mem; - LOCAL_STDOUT.with(move |slot| mem::replace(&mut *slot.borrow_mut(), sink)).and_then(|mut s| { - let _ = s.flush(); - Some(s) - }) + let s = LOCAL_STDOUT.with(move |slot| mem::replace(&mut *slot.borrow_mut(), sink)).and_then( + |mut s| { + let _ = s.flush(); + Some(s) + }, + ); + LOCAL_STREAMS.store(true, Ordering::Release); + s } /// Write `args` to output stream `local_s` if possible, `global_s` @@ -937,20 +948,26 @@ fn print_to( ) where T: Write, { - let result = local_s - .try_with(|s| { - // Note that we completely remove a local sink to write to in case - // our printing recursively panics/prints, so the recursive - // panic/print goes to the global sink instead of our local sink. - let prev = s.borrow_mut().take(); - if let Some(mut w) = prev { - let result = w.write_fmt(args); - *s.borrow_mut() = Some(w); - return result; - } - global_s().write_fmt(args) + let result = LOCAL_STREAMS + .load(Ordering::Acquire) + .then(|| { + local_s + .try_with(|s| { + // Note that we completely remove a local sink to write to in case + // our printing recursively panics/prints, so the recursive + // panic/print goes to the global sink instead of our local sink. + let prev = s.borrow_mut().take(); + if let Some(mut w) = prev { + let result = w.write_fmt(args); + *s.borrow_mut() = Some(w); + return result; + } + global_s().write_fmt(args) + }) + .ok() }) - .unwrap_or_else(|_| global_s().write_fmt(args)); + .flatten() + .unwrap_or_else(|| global_s().write_fmt(args)); if let Err(e) = result { panic!("failed printing to {}: {}", label, e); diff --git a/library/std/src/lib.rs b/library/std/src/lib.rs index d03428dd082..410860b4a5d 100644 --- a/library/std/src/lib.rs +++ b/library/std/src/lib.rs @@ -226,6 +226,7 @@ #![feature(asm)] #![feature(associated_type_bounds)] #![feature(atomic_mut_ptr)] +#![feature(bool_to_option)] #![feature(box_syntax)] #![feature(c_variadic)] #![feature(cfg_accessible)] From ed3ead013f40c65e2972c794a71b756237a31b46 Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Sun, 27 Sep 2020 15:32:07 +0200 Subject: [PATCH 2/3] Relax memory ordering of LOCAL_STREAMS and document it. --- library/std/src/io/stdio.rs | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/library/std/src/io/stdio.rs b/library/std/src/io/stdio.rs index 814e0dfda54..df83c329829 100644 --- a/library/std/src/io/stdio.rs +++ b/library/std/src/io/stdio.rs @@ -16,8 +16,6 @@ use crate::sys_common; use crate::sys_common::remutex::{ReentrantMutex, ReentrantMutexGuard}; use crate::thread::LocalKey; -static LOCAL_STREAMS: AtomicBool = AtomicBool::new(false); - thread_local! { /// Used by the test crate to capture the output of the print! and println! macros. static LOCAL_STDOUT: RefCell>> = { @@ -32,6 +30,20 @@ thread_local! { } } +/// Flag to indicate LOCAL_STDOUT and/or LOCAL_STDERR is used. +/// +/// If both are None and were never set on any thread, this flag is set to +/// false, and both LOCAL_STDOUT and LOCAL_STDOUT can be safely ignored on all +/// threads, saving some time and memory registering an unused thread local. +/// +/// Note about memory ordering: This contains information about whether two +/// thread local variables might be in use. Although this is a global flag, the +/// memory ordering between threads does not matter: we only want this flag to +/// have a consistent order between set_print/set_panic and print_to *within +/// the same thread*. Within the same thread, things always have a perfectly +/// consistent order. So Ordering::Relaxed is fine. +static LOCAL_STREAMS: AtomicBool = AtomicBool::new(false); + /// A handle to a raw instance of the standard input stream of this process. /// /// This handle is not synchronized or buffered in any fashion. Constructed via @@ -899,7 +911,7 @@ pub fn set_panic(sink: Option>) -> Option>) -> Option( T: Write, { let result = LOCAL_STREAMS - .load(Ordering::Acquire) + .load(Ordering::Relaxed) .then(|| { local_s .try_with(|s| { From de597fca40d129435c53a69c6662d7bfac29771d Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Sun, 27 Sep 2020 15:35:35 +0200 Subject: [PATCH 3/3] Optimize set_{panic,print}(None). --- library/std/src/io/stdio.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/library/std/src/io/stdio.rs b/library/std/src/io/stdio.rs index df83c329829..36b49401591 100644 --- a/library/std/src/io/stdio.rs +++ b/library/std/src/io/stdio.rs @@ -905,6 +905,10 @@ impl fmt::Debug for StderrLock<'_> { #[doc(hidden)] pub fn set_panic(sink: Option>) -> Option> { use crate::mem; + if sink.is_none() && !LOCAL_STREAMS.load(Ordering::Relaxed) { + // LOCAL_STDERR is definitely None since LOCAL_STREAMS is false. + return None; + } let s = LOCAL_STDERR.with(move |slot| mem::replace(&mut *slot.borrow_mut(), sink)).and_then( |mut s| { let _ = s.flush(); @@ -932,6 +936,10 @@ pub fn set_panic(sink: Option>) -> Option>) -> Option> { use crate::mem; + if sink.is_none() && !LOCAL_STREAMS.load(Ordering::Relaxed) { + // LOCAL_STDOUT is definitely None since LOCAL_STREAMS is false. + return None; + } let s = LOCAL_STDOUT.with(move |slot| mem::replace(&mut *slot.borrow_mut(), sink)).and_then( |mut s| { let _ = s.flush();