mirror of
https://github.com/mozilla/gecko-dev.git
synced 2024-10-15 14:25:52 +00:00
servo: Merge #18248 - stylo: Check stack depth in invalidation machinery and re-enable limits (from bholley:more_stack_limits); r=bholley
https://bugzilla.mozilla.org/show_bug.cgi?id=1376884 Source-Repo: https://github.com/servo/servo Source-Revision: 3c42792efade03a31b9eeca8be457992cf83722c --HG-- extra : subtree_source : https%3A//hg.mozilla.org/projects/converted-servo-linear extra : subtree_revision : f02582172bef70c26d97eb8bff9304956f3f911a
This commit is contained in:
parent
3bc8ec26f0
commit
b1eb48ffa3
@ -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),
|
||||
}
|
||||
}
|
||||
|
||||
|
@ -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() }
|
||||
|
@ -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
|
||||
};
|
||||
|
@ -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() {
|
||||
|
@ -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
|
||||
|
@ -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)) {
|
||||
|
@ -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);
|
||||
}
|
||||
}
|
||||
|
Loading…
Reference in New Issue
Block a user