From b0d1cde558a0ecb8517974f8ef31c7d4f8b65770 Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Tue, 19 Sep 2017 10:19:49 -0700 Subject: [PATCH 1/3] Make the thread_state machinery behave the same across debug and opt builds. I don't need this per se, but it seems like a footgun for the methods to return incorrect information depending on the build type. I don't see anywhere where the overhead would be at all significant. MozReview-Commit-ID: G1qyUFhI0aB --- components/style/thread_state.rs | 31 +++++-------------------------- 1 file changed, 5 insertions(+), 26 deletions(-) diff --git a/components/style/thread_state.rs b/components/style/thread_state.rs index ab0672e2503..035d1e939b8 100644 --- a/components/style/thread_state.rs +++ b/components/style/thread_state.rs @@ -2,11 +2,8 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ -//! Supports dynamic assertions in debug builds about what sort of thread is -//! running and what state it's in. -//! -//! In release builds, `get` returns 0. All of the other functions inline -//! away to nothing. +//! Supports dynamic assertions in about what sort of thread is running and +//! what state it's in. #![deny(missing_docs)] @@ -37,20 +34,13 @@ macro_rules! thread_types ( ( $( $fun:ident = $flag:ident ; )* ) => ( } $( - #[cfg(debug_assertions)] #[allow(missing_docs)] pub fn $fun(self) -> bool { self.contains($flag) } - #[cfg(not(debug_assertions))] - #[allow(missing_docs)] - pub fn $fun(self) -> bool { - true - } )* } - #[cfg(debug_assertions)] static TYPES: &'static [ThreadState] = &[ $( $flag ),* ]; )); @@ -60,7 +50,6 @@ thread_types! { is_layout = LAYOUT; } -#[cfg(debug_assertions)] mod imp { use std::cell::RefCell; use super::{TYPES, ThreadState}; @@ -89,14 +78,14 @@ mod imp { }); // Exactly one of the thread type flags should be set. - assert_eq!(1, TYPES.iter().filter(|&&ty| state.contains(ty)).count()); + debug_assert_eq!(1, TYPES.iter().filter(|&&ty| state.contains(ty)).count()); state } /// Enter into a given temporary state. Panics if re-entring. pub fn enter(x: ThreadState) { let state = get(); - assert!(!state.intersects(x)); + debug_assert!(!state.intersects(x)); STATE.with(|ref k| { *k.borrow_mut() = Some(state | x); }) @@ -105,19 +94,9 @@ mod imp { /// Exit a given temporary state. pub fn exit(x: ThreadState) { let state = get(); - assert!(state.contains(x)); + debug_assert!(state.contains(x)); STATE.with(|ref k| { *k.borrow_mut() = Some(state & !x); }) } } - -#[cfg(not(debug_assertions))] -#[allow(missing_docs)] -mod imp { - use super::ThreadState; - #[inline(always)] pub fn initialize(_: ThreadState) { } - #[inline(always)] pub fn get() -> ThreadState { ThreadState::empty() } - #[inline(always)] pub fn enter(_: ThreadState) { } - #[inline(always)] pub fn exit(_: ThreadState) { } -} From 531397ff15eed3da0839fc0029d48114c95bece2 Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Tue, 19 Sep 2017 12:03:49 -0700 Subject: [PATCH 2/3] Eliminate the now-unnecessary internal mod in thread_state. MozReview-Commit-ID: 2d7lvQx7jOf --- components/style/thread_state.rs | 93 +++++++++++++++----------------- 1 file changed, 44 insertions(+), 49 deletions(-) diff --git a/components/style/thread_state.rs b/components/style/thread_state.rs index 035d1e939b8..3c06666a057 100644 --- a/components/style/thread_state.rs +++ b/components/style/thread_state.rs @@ -7,7 +7,7 @@ #![deny(missing_docs)] -pub use self::imp::{enter, exit, get, initialize}; +use std::cell::RefCell; bitflags! { /// A thread state flag, used for multiple assertions. @@ -50,53 +50,48 @@ thread_types! { is_layout = LAYOUT; } -mod imp { - use std::cell::RefCell; - use super::{TYPES, ThreadState}; +thread_local!(static STATE: RefCell> = RefCell::new(None)); - thread_local!(static STATE: RefCell> = RefCell::new(None)); - - /// Initialize the current thread state. - pub fn initialize(x: ThreadState) { - STATE.with(|ref k| { - if let Some(ref s) = *k.borrow() { - panic!("Thread state already initialized as {:?}", s); - } - *k.borrow_mut() = Some(x); - }); - get(); // check the assertion below - } - - /// Get the current thread state. - pub fn get() -> ThreadState { - let state = STATE.with(|ref k| { - match *k.borrow() { - // This is one of the layout threads, that use rayon. - None => super::LAYOUT | super::IN_WORKER, - Some(s) => s, - } - }); - - // Exactly one of the thread type flags should be set. - debug_assert_eq!(1, TYPES.iter().filter(|&&ty| state.contains(ty)).count()); - state - } - - /// Enter into a given temporary state. Panics if re-entring. - pub fn enter(x: ThreadState) { - let state = get(); - debug_assert!(!state.intersects(x)); - STATE.with(|ref k| { - *k.borrow_mut() = Some(state | x); - }) - } - - /// Exit a given temporary state. - pub fn exit(x: ThreadState) { - let state = get(); - debug_assert!(state.contains(x)); - STATE.with(|ref k| { - *k.borrow_mut() = Some(state & !x); - }) - } +/// Initializes the current thread state. +pub fn initialize(x: ThreadState) { + STATE.with(|ref k| { + if let Some(ref s) = *k.borrow() { + panic!("Thread state already initialized as {:?}", s); + } + *k.borrow_mut() = Some(x); + }); + get(); // check the assertion below +} + +/// Gets the current thread state. +pub fn get() -> ThreadState { + let state = STATE.with(|ref k| { + match *k.borrow() { + // This is one of the layout threads, that use rayon. + None => LAYOUT | IN_WORKER, + Some(s) => s, + } + }); + + // Exactly one of the thread type flags should be set. + debug_assert_eq!(1, TYPES.iter().filter(|&&ty| state.contains(ty)).count()); + state +} + +/// Enters into a given temporary state. Panics if re-entring. +pub fn enter(x: ThreadState) { + let state = get(); + debug_assert!(!state.intersects(x)); + STATE.with(|ref k| { + *k.borrow_mut() = Some(state | x); + }) +} + +/// Exits a given temporary state. +pub fn exit(x: ThreadState) { + let state = get(); + debug_assert!(state.contains(x)); + STATE.with(|ref k| { + *k.borrow_mut() = Some(state & !x); + }) } From 90e9bbbadc6b02596a1cd9a6fdaa9e52cfb26745 Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Tue, 19 Sep 2017 12:13:34 -0700 Subject: [PATCH 3/3] Explicitly register rayon threads, rather than assuming that as the default. MozReview-Commit-ID: E4kUyy8HjmV --- components/layout_thread/lib.rs | 3 ++- components/style/gecko/global_style_data.rs | 2 ++ components/style/thread_state.rs | 14 ++++++-------- 3 files changed, 10 insertions(+), 9 deletions(-) diff --git a/components/layout_thread/lib.rs b/components/layout_thread/lib.rs index c9a80b7ddd8..64ef126b79e 100644 --- a/components/layout_thread/lib.rs +++ b/components/layout_thread/lib.rs @@ -461,7 +461,8 @@ impl LayoutThread { ScaleFactor::new(opts::get().device_pixels_per_px.unwrap_or(1.0))); let configuration = - rayon::Configuration::new().num_threads(layout_threads); + rayon::Configuration::new().num_threads(layout_threads) + .start_handler(|_| thread_state::initialize_layout_worker_thread()); let parallel_traversal = if layout_threads > 1 { Some(rayon::ThreadPool::new(configuration).expect("ThreadPool creation failed")) } else { diff --git a/components/style/gecko/global_style_data.rs b/components/style/gecko/global_style_data.rs index a4812b95a4f..4a6dcc21805 100644 --- a/components/style/gecko/global_style_data.rs +++ b/components/style/gecko/global_style_data.rs @@ -15,6 +15,7 @@ use shared_lock::SharedRwLock; use std::cmp; use std::env; use std::ffi::CString; +use thread_state; /// Global style data pub struct GlobalStyleData { @@ -39,6 +40,7 @@ fn thread_name(index: usize) -> String { } fn thread_startup(index: usize) { + thread_state::initialize_layout_worker_thread(); unsafe { Gecko_SetJemallocThreadLocalArena(true); } diff --git a/components/style/thread_state.rs b/components/style/thread_state.rs index 3c06666a057..894cdb4a813 100644 --- a/components/style/thread_state.rs +++ b/components/style/thread_state.rs @@ -40,9 +40,6 @@ macro_rules! thread_types ( ( $( $fun:ident = $flag:ident ; )* ) => ( } )* } - - static TYPES: &'static [ThreadState] = - &[ $( $flag ),* ]; )); thread_types! { @@ -60,21 +57,22 @@ pub fn initialize(x: ThreadState) { } *k.borrow_mut() = Some(x); }); - get(); // check the assertion below +} + +/// Initializes the current thread as a layout worker thread. +pub fn initialize_layout_worker_thread() { + initialize(LAYOUT | IN_WORKER); } /// Gets the current thread state. pub fn get() -> ThreadState { let state = STATE.with(|ref k| { match *k.borrow() { - // This is one of the layout threads, that use rayon. - None => LAYOUT | IN_WORKER, + None => ThreadState::empty(), // Unknown thread. Some(s) => s, } }); - // Exactly one of the thread type flags should be set. - debug_assert_eq!(1, TYPES.iter().filter(|&&ty| state.contains(ty)).count()); state }