diff options
| -rw-r--r-- | components/style/context.rs | 54 | ||||
| -rw-r--r-- | components/style/data.rs | 8 | ||||
| -rw-r--r-- | components/style/gecko/global_style_data.rs | 6 | ||||
| -rw-r--r-- | components/style/invalidation/element/invalidator.rs | 26 | ||||
| -rw-r--r-- | components/style/parallel.rs | 16 | ||||
| -rw-r--r-- | components/style/traversal.rs | 10 | ||||
| -rw-r--r-- | ports/geckolib/glue.rs | 2 |
7 files changed, 81 insertions, 41 deletions
diff --git a/components/style/context.rs b/components/style/context.rs index 4e166929b9d..d7db7a5ab61 100644 --- a/components/style/context.rs +++ b/components/style/context.rs @@ -16,7 +16,7 @@ use euclid::Size2D; use fnv::FnvHashMap; use font_metrics::FontMetricsProvider; #[cfg(feature = "gecko")] use gecko_bindings::structs; -use parallel::STYLE_THREAD_STACK_SIZE_KB; +use parallel::{STACK_SAFETY_MARGIN_KB, STYLE_THREAD_STACK_SIZE_KB}; #[cfg(feature = "servo")] use parking_lot::RwLock; use properties::ComputedValues; #[cfg(feature = "servo")] use properties::PropertyId; @@ -628,24 +628,35 @@ impl StackLimitChecker { pub fn limit_exceeded(&self) -> bool { let curr_sp = StackLimitChecker::get_sp(); - // Try to assert if we're called from a different thread than the - // one that originally created this object. This is a bit subtle - // and relies on wraparound behaviour of unsigned integers. - // - // * If we're called from a thread whose stack has a higher address - // than the one that created this object, then - // |curr_sp - self.lower_limit| will (almost certainly) be larger - // than the thread stack size, so the check will fail. - // - // * If we're called from a thread whose stack has a lower address - // than the one that created this object, then - // |curr_sp - self.lower_limit| will be negative, which will look - // like a very large unsigned value, so the check will also fail. + // Do some sanity-checking to ensure that our invariants hold, even in + // the case where we've exceeded the soft limit. // // The correctness of depends on the assumption that no stack wraps // around the end of the address space. - //debug_assert!(curr_sp - self.lower_limit - // <= STYLE_THREAD_STACK_SIZE_KB * 1024); + if cfg!(debug_assertions) { + // Compute the actual bottom of the stack by subtracting our safety + // margin from our soft limit. Note that this will be slightly below + // the actual bottom of the stack, because there are a few initial + // frames on the stack before we do the measurement that computes + // the limit. + let stack_bottom = self.lower_limit - STACK_SAFETY_MARGIN_KB * 1024; + + // The bottom of the stack should be below the current sp. If it + // isn't, that means we've either waited too long to check the limit + // and burned through our safety margin (in which case we probably + // would have segfaulted by now), or we're using a limit computed for + // a different thread. + debug_assert!(stack_bottom < curr_sp); + + // Compute the distance between the current sp and the bottom of + // the stack, and compare it against the current stack. It should be + // no further from us than the total stack size. We allow some slop + // to handle the fact that stack_bottom is a bit further than the + // bottom of the stack, as discussed above. + let distance_to_stack_bottom = curr_sp - stack_bottom; + let max_allowable_distance = (STYLE_THREAD_STACK_SIZE_KB + 10) * 1024; + debug_assert!(distance_to_stack_bottom <= max_allowable_distance); + } // The actual bounds check. curr_sp <= self.lower_limit @@ -714,7 +725,7 @@ impl<E: TElement> ThreadLocalStyleContext<E> { current_element_info: None, font_metrics_provider: E::FontMetricsProvider::create_from(shared), stack_limit_checker: StackLimitChecker::new( - (STYLE_THREAD_STACK_SIZE_KB - 40) * 1024), + (STYLE_THREAD_STACK_SIZE_KB - STACK_SAFETY_MARGIN_KB) * 1024), } } @@ -729,15 +740,8 @@ impl<E: TElement> ThreadLocalStyleContext<E> { statistics: TraversalStatistics::default(), current_element_info: None, font_metrics_provider: E::FontMetricsProvider::create_from(shared), - // Threads in the styling pool have small stacks, and we have to - // be careful not to run out of stack during recursion in - // parallel.rs. Therefore set up a stack limit checker, in - // which we reserve 40KB of stack as a safety buffer. Currently - // the stack size is 128KB, so this allows 88KB for recursive - // DOM traversal, which encompasses 53 levels of recursion before - // the limiter kicks in, on x86_64-Linux. See Gecko bug 1376883. stack_limit_checker: StackLimitChecker::new( - (STYLE_THREAD_STACK_SIZE_KB - 40) * 1024), + (STYLE_THREAD_STACK_SIZE_KB - STACK_SAFETY_MARGIN_KB) * 1024), } } diff --git a/components/style/data.rs b/components/style/data.rs index ff4a214ef5a..7d9a8ba6751 100644 --- a/components/style/data.rs +++ b/components/style/data.rs @@ -4,7 +4,7 @@ //! Per-node data used in style calculation. -use context::SharedStyleContext; +use context::{SharedStyleContext, StackLimitChecker}; use dom::TElement; use invalidation::element::restyle_hints::RestyleHint; use properties::ComputedValues; @@ -327,8 +327,9 @@ impl ElementData { pub fn invalidate_style_if_needed<'a, E: TElement>( &mut self, element: E, - shared_context: &SharedStyleContext) - { + shared_context: &SharedStyleContext, + stack_limit_checker: Option<&StackLimitChecker>, + ) { // In animation-only restyle we shouldn't touch snapshot at all. if shared_context.traversal_flags.for_animation_only() { return; @@ -349,6 +350,7 @@ impl ElementData { element, Some(self), shared_context, + stack_limit_checker, ); invalidator.invalidate(); unsafe { element.set_handled_snapshot() } diff --git a/components/style/gecko/global_style_data.rs b/components/style/gecko/global_style_data.rs index 61c763609e7..a4812b95a4f 100644 --- a/components/style/gecko/global_style_data.rs +++ b/components/style/gecko/global_style_data.rs @@ -9,6 +9,7 @@ use gecko_bindings::bindings; use gecko_bindings::bindings::{Gecko_RegisterProfilerThread, Gecko_UnregisterProfilerThread}; use gecko_bindings::bindings::Gecko_SetJemallocThreadLocalArena; use num_cpus; +use parallel::STYLE_THREAD_STACK_SIZE_KB; use rayon; use shared_lock::SharedRwLock; use std::cmp; @@ -92,9 +93,8 @@ lazy_static! { .breadth_first() .thread_name(thread_name) .start_handler(thread_startup) - .exit_handler(thread_shutdown); - // Set thread stack size to 128KB. See Gecko bug 1376883. - //.stack_size(STYLE_THREAD_STACK_SIZE_KB * 1024); + .exit_handler(thread_shutdown) + .stack_size(STYLE_THREAD_STACK_SIZE_KB * 1024); let pool = rayon::ThreadPool::new(configuration).ok(); pool }; diff --git a/components/style/invalidation/element/invalidator.rs b/components/style/invalidation/element/invalidator.rs index a42af0872c4..00c3e3f8954 100644 --- a/components/style/invalidation/element/invalidator.rs +++ b/components/style/invalidation/element/invalidator.rs @@ -6,7 +6,7 @@ //! element styles need to be invalidated. use Atom; -use context::SharedStyleContext; +use context::{SharedStyleContext, StackLimitChecker}; use data::ElementData; use dom::{TElement, TNode}; use element_state::{ElementState, IN_VISITED_OR_UNVISITED_STATE}; @@ -40,6 +40,7 @@ pub struct TreeStyleInvalidator<'a, 'b: 'a, E> // descendants if an element has no data, though. data: Option<&'a mut ElementData>, shared_context: &'a SharedStyleContext<'b>, + stack_limit_checker: Option<&'a StackLimitChecker>, } type InvalidationVector = SmallVec<[Invalidation; 10]>; @@ -130,11 +131,13 @@ impl<'a, 'b: 'a, E> TreeStyleInvalidator<'a, 'b, E> element: E, data: Option<&'a mut ElementData>, shared_context: &'a SharedStyleContext<'b>, + stack_limit_checker: Option<&'a StackLimitChecker>, ) -> Self { Self { - element: element, - data: data, - shared_context: shared_context, + element, + data, + shared_context, + stack_limit_checker, } } @@ -276,7 +279,8 @@ impl<'a, 'b: 'a, E> TreeStyleInvalidator<'a, 'b, E> let mut sibling_invalidator = TreeStyleInvalidator::new( sibling, sibling_data, - self.shared_context + self.shared_context, + self.stack_limit_checker, ); let mut invalidations_for_descendants = InvalidationVector::new(); @@ -338,7 +342,8 @@ impl<'a, 'b: 'a, E> TreeStyleInvalidator<'a, 'b, E> let mut child_invalidator = TreeStyleInvalidator::new( child, child_data, - self.shared_context + self.shared_context, + self.stack_limit_checker, ); let mut invalidations_for_descendants = InvalidationVector::new(); @@ -448,12 +453,21 @@ impl<'a, 'b: 'a, E> TreeStyleInvalidator<'a, 'b, E> match self.data { None => return false, Some(ref data) => { + // FIXME(emilio): Only needs to check RESTYLE_DESCENDANTS, + // really. if data.restyle.hint.contains_subtree() { return false; } } } + if let Some(checker) = self.stack_limit_checker { + if checker.limit_exceeded() { + self.data.as_mut().unwrap().restyle.hint.insert(RESTYLE_DESCENDANTS); + return true; + } + } + let mut any_descendant = false; if let Some(anon_content) = self.element.xbl_binding_anonymous_content() { diff --git a/components/style/parallel.rs b/components/style/parallel.rs index 3d7498a4f5b..ffc02674bcb 100644 --- a/components/style/parallel.rs +++ b/components/style/parallel.rs @@ -34,6 +34,22 @@ use traversal::{DomTraversal, PerLevelTraversalData}; /// The minimum stack size for a thread in the styling pool, in kilobytes. pub const STYLE_THREAD_STACK_SIZE_KB: usize = 128; +/// The stack margin. If we get this deep in the stack, we will skip recursive +/// optimizations to ensure that there is sufficient room for non-recursive work. +/// +/// When measured with 128KB stacks and 40KB margin, we could support 53 +/// levels of recursion before the limiter kicks in, on x86_64-Linux [1]. +/// +/// We currently use 45KB margins, because that seems to be the minimum amount +/// of head room required by an unoptimized debug build, as measured on [2]. We +/// could probably get away with a smaller margin on optimized release builds, +/// but that would be a pain, and the extra padding reduces stability risk. +/// +/// [1] See Gecko bug 1376883 for more discussion on the measurements. +/// [2] layout/style/crashtests/1383981-3.html +/// +pub const STACK_SAFETY_MARGIN_KB: usize = 45; + /// The maximum number of child nodes that we will process as a single unit. /// /// Larger values will increase style sharing cache hits and general DOM diff --git a/components/style/traversal.rs b/components/style/traversal.rs index 07359379105..a3fe06f3fef 100644 --- a/components/style/traversal.rs +++ b/components/style/traversal.rs @@ -139,7 +139,7 @@ pub trait DomTraversal<E: TElement> : Sync { fn pre_traverse( root: E, shared_context: &SharedStyleContext, - traversal_flags: TraversalFlags + traversal_flags: TraversalFlags, ) -> PreTraverseToken { // If this is an unstyled-only traversal, the caller has already verified // that there's something to traverse, and we don't need to do any @@ -155,7 +155,7 @@ pub trait DomTraversal<E: TElement> : Sync { if let Some(ref mut data) = data { // Invalidate our style, and the one of our siblings and descendants // as needed. - data.invalidate_style_if_needed(root, shared_context); + data.invalidate_style_if_needed(root, shared_context, None); // Make sure we don't have any stale RECONSTRUCTED_ANCESTOR bits from // the last traversal (at a potentially-higher root). From the @@ -828,7 +828,11 @@ where // as needed. // // NB: This will be a no-op if there's no snapshot. - child_data.invalidate_style_if_needed(child, &context.shared); + child_data.invalidate_style_if_needed( + child, + &context.shared, + Some(&context.thread_local.stack_limit_checker) + ); } if D::element_needs_traversal(child, flags, child_data.map(|d| &*d), Some(data)) { diff --git a/ports/geckolib/glue.rs b/ports/geckolib/glue.rs index a2ed3dcc59b..29ad0948f14 100644 --- a/ports/geckolib/glue.rs +++ b/ports/geckolib/glue.rs @@ -3701,6 +3701,6 @@ pub extern "C" fn Servo_ProcessInvalidations(set: RawServoStyleSetBorrowed, let mut data = data.as_mut().map(|d| &mut **d); if let Some(ref mut data) = data { - data.invalidate_style_if_needed(element, &shared_style_context); + data.invalidate_style_if_needed(element, &shared_style_context, None); } } |