From a3020419d9eace898727f5b778e661b61a1bb8df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Fri, 22 Jul 2016 16:48:08 -0700 Subject: [PATCH 01/10] stylo: Don't traverse the whole dom every restyle, propagate the dirty flag down the DOM. This commit adds hooks to the Servo style traversal to avoid traversing all the DOM for every restyle. Additionally it changes the behavior of the dirty flag to be propagated top down, to prevent extra overhead when an element is dirtied. This commit doesn't aim to change the behavior on Servo just yet, since Servo might rely on a full bottom up reconstruction of the flows. I'll need to double check and implement that separately. --- components/style/data.rs | 4 ++-- components/style/parallel.rs | 32 ++++++++++++++++++++------------ components/style/sequential.rs | 19 ++++++++++++++----- components/style/traversal.rs | 28 ++++++++++++++-------------- ports/geckolib/traversal.rs | 16 ++++++++++++++++ 5 files changed, 66 insertions(+), 33 deletions(-) diff --git a/components/style/data.rs b/components/style/data.rs index 906848998fb..44920c72c98 100644 --- a/components/style/data.rs +++ b/components/style/data.rs @@ -37,13 +37,13 @@ impl PrivateStyleData { #[cfg_attr(feature = "servo", derive(HeapSizeOf))] pub struct DomParallelInfo { /// The number of children that still need work done. - pub children_count: AtomicIsize, + pub children_to_process: AtomicIsize, } impl DomParallelInfo { pub fn new() -> DomParallelInfo { DomParallelInfo { - children_count: AtomicIsize::new(0), + children_to_process: AtomicIsize::new(0), } } } diff --git a/components/style/parallel.rs b/components/style/parallel.rs index 0bc840f7492..889f7f0a15b 100644 --- a/components/style/parallel.rs +++ b/components/style/parallel.rs @@ -63,25 +63,34 @@ fn top_down_dom(unsafe_nodes: UnsafeNodeList, // Get a real layout node. let node = unsafe { N::from_unsafe(&unsafe_node) }; + if !context.should_process(node) { + continue; + } + // Perform the appropriate traversal. context.process_preorder(node); - let child_count = node.children_count(); + // Possibly enqueue the children. + let mut children_to_process = 0isize; + for kid in node.children() { + context.pre_process_child_hook(node, kid); + if context.should_process(kid) { + children_to_process += 1; + discovered_child_nodes.push(kid.to_unsafe()) + } + } // Reset the count of children. { let data = node.mutate_data().unwrap(); - data.parallel.children_count.store(child_count as isize, - Ordering::Relaxed); + data.parallel.children_to_process + .store(children_to_process, + Ordering::Relaxed); } - // Possibly enqueue the children. - if child_count != 0 { - for kid in node.children() { - discovered_child_nodes.push(kid.to_unsafe()) - } - } else { - // If there were no more children, start walking back up. + + // If there were no more children, start walking back up. + if children_to_process == 0 { bottom_up_dom::(unsafe_nodes.1, unsafe_node, proxy) } } @@ -128,7 +137,7 @@ fn bottom_up_dom(root: OpaqueNode, if parent_data .parallel - .children_count + .children_to_process .fetch_sub(1, Ordering::Relaxed) != 1 { // Get out of here and find another node to work on. break @@ -138,4 +147,3 @@ fn bottom_up_dom(root: OpaqueNode, node = parent; } } - diff --git a/components/style/sequential.rs b/components/style/sequential.rs index a5a9b519e57..56b8a563d27 100644 --- a/components/style/sequential.rs +++ b/components/style/sequential.rs @@ -9,20 +9,29 @@ use traversal::DomTraversalContext; pub fn traverse_dom(root: N, shared: &C::SharedContext) - where N: TNode, - C: DomTraversalContext { + where N: TNode, + C: DomTraversalContext +{ fn doit<'a, N, C>(context: &'a C, node: N) - where N: TNode, C: DomTraversalContext { + where N: TNode, + C: DomTraversalContext + { + debug_assert!(context.should_process(node)); context.process_preorder(node); for kid in node.children() { - doit::(context, kid); + context.pre_process_child_hook(node, kid); + if context.should_process(node) { + doit::(context, kid); + } } context.process_postorder(node); } let context = C::new(shared, root.opaque()); - doit::(&context, root); + if context.should_process(root) { + doit::(&context, root); + } } diff --git a/components/style/traversal.rs b/components/style/traversal.rs index 76f8539c72f..e4b1d8f11de 100644 --- a/components/style/traversal.rs +++ b/components/style/traversal.rs @@ -147,6 +147,12 @@ pub trait DomTraversalContext { fn process_preorder(&self, node: N); /// Process `node` on the way up, after its children have been processed. fn process_postorder(&self, node: N); + + /// Returns if the node should be processed by the preorder traversal. + fn should_process(&self, _node: N) -> bool { true } + + /// Do an action over the child before pushing him to the work queue. + fn pre_process_child_hook(&self, _parent: N, _kid: N) {} } /// Calculates the style for a single node. @@ -245,21 +251,15 @@ pub fn recalc_style_at<'a, N, C>(context: &'a C, put_thread_local_bloom_filter(bf, &unsafe_layout_node, context.shared_context()); // Mark the node as DIRTY_ON_VIEWPORT_SIZE_CHANGE is it uses viewport percentage units. - match node.as_element() { - Some(element) => { - match *element.style_attribute() { - Some(ref property_declaration_block) => { - if property_declaration_block.declarations().any(|d| d.0.has_viewport_percentage()) { - unsafe { - node.set_dirty_on_viewport_size_changed(); - } - node.set_descendants_dirty_on_viewport_size_changed(); - } - }, - None => {} + if let Some(element) = node.as_element() { + if let Some(ref property_declaration_block) = *element.style_attribute() { + if property_declaration_block.declarations().any(|d| d.0.has_viewport_percentage()) { + unsafe { + node.set_dirty_on_viewport_size_changed(); + } + node.set_descendants_dirty_on_viewport_size_changed(); } - }, - None => {} + } } } diff --git a/ports/geckolib/traversal.rs b/ports/geckolib/traversal.rs index 87b63e83e15..6a3667e0b96 100644 --- a/ports/geckolib/traversal.rs +++ b/ports/geckolib/traversal.rs @@ -6,6 +6,7 @@ use context::StandaloneStyleContext; use std::mem; use style::context::SharedStyleContext; use style::dom::OpaqueNode; +use style::dom::TNode; use style::traversal::{DomTraversalContext, recalc_style_at}; use wrapper::GeckoNode; @@ -35,5 +36,20 @@ impl<'lc, 'ln> DomTraversalContext> for RecalcStyleOnly<'lc> { recalc_style_at(&self.context, self.root, node); } + fn should_process(&self, node: GeckoNode<'ln>) -> bool { + node.is_dirty() || node.has_dirty_descendants() + } + fn process_postorder(&self, _: GeckoNode<'ln>) {} + + fn pre_process_child_hook(&self, parent: GeckoNode<'ln>, kid: GeckoNode<'ln>) { + // NOTE: At this point is completely safe to modify either the parent or + // the child, since we have exclusive access to them. + if parent.is_dirty() { + unsafe { + kid.set_dirty(true); + parent.set_dirty_descendants(true); + } + } + } } From 96ea1a335cab7a55e7d9bdbc988c6d636e2ac001 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Fri, 22 Jul 2016 16:59:19 -0700 Subject: [PATCH 02/10] stylo: don't double check that the root node is dirty in restyle_tree, since now we ensure it in the caller. --- components/style/traversal.rs | 6 +++++- ports/geckolib/glue.rs | 14 +++++++------- ports/geckolib/traversal.rs | 6 ++++-- 3 files changed, 16 insertions(+), 10 deletions(-) diff --git a/components/style/traversal.rs b/components/style/traversal.rs index e4b1d8f11de..69521c1557a 100644 --- a/components/style/traversal.rs +++ b/components/style/traversal.rs @@ -148,7 +148,11 @@ pub trait DomTraversalContext { /// Process `node` on the way up, after its children have been processed. fn process_postorder(&self, node: N); - /// Returns if the node should be processed by the preorder traversal. + /// Returns if the node should be processed by the preorder traversal (and + /// then by the post-order one). + /// + /// Note that this is true unconditionally for servo, since it requires to + /// bubble the widths bottom-up for all the DOM. fn should_process(&self, _node: N) -> bool { true } /// Do an action over the child before pushing him to the work queue. diff --git a/ports/geckolib/glue.rs b/ports/geckolib/glue.rs index 95f316f5cc1..288e65431e5 100644 --- a/ports/geckolib/glue.rs +++ b/ports/geckolib/glue.rs @@ -113,13 +113,13 @@ fn restyle_subtree(node: GeckoNode, raw_data: *mut RawServoStyleSet) { timer: Timer::new(), }; - if node.is_dirty() || node.has_dirty_descendants() { - if per_doc_data.num_threads == 1 { - sequential::traverse_dom::(node, &shared_style_context); - } else { - parallel::traverse_dom::(node, &shared_style_context, - &mut per_doc_data.work_queue); - } + // We ensure this is true before calling Servo_RestyleSubtree() + debug_assert!(node.is_dirty() || node.has_dirty_descendants()); + if per_doc_data.num_threads == 1 { + sequential::traverse_dom::(node, &shared_style_context); + } else { + parallel::traverse_dom::(node, &shared_style_context, + &mut per_doc_data.work_queue); } } diff --git a/ports/geckolib/traversal.rs b/ports/geckolib/traversal.rs index 6a3667e0b96..562153baf07 100644 --- a/ports/geckolib/traversal.rs +++ b/ports/geckolib/traversal.rs @@ -36,12 +36,14 @@ impl<'lc, 'ln> DomTraversalContext> for RecalcStyleOnly<'lc> { recalc_style_at(&self.context, self.root, node); } + fn process_postorder(&self, _: GeckoNode<'ln>) {} + + /// In Gecko we use this traversal just for restyling, so we can stop once + /// we know there aren't more dirty nodes under ourselves. fn should_process(&self, node: GeckoNode<'ln>) -> bool { node.is_dirty() || node.has_dirty_descendants() } - fn process_postorder(&self, _: GeckoNode<'ln>) {} - fn pre_process_child_hook(&self, parent: GeckoNode<'ln>, kid: GeckoNode<'ln>) { // NOTE: At this point is completely safe to modify either the parent or // the child, since we have exclusive access to them. From d81fe27b114c12a9fdb292b66ad52f9c1def3f1f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Fri, 22 Jul 2016 19:33:53 -0700 Subject: [PATCH 03/10] style: Avoid propagating the restyle flag all through the dom when a node gets dirty. This puts us in pair with stylo. --- components/layout/traversal.rs | 6 ++++-- components/script/dom/node.rs | 14 +------------- components/style/traversal.rs | 18 ++++++++++++++++-- ports/geckolib/traversal.rs | 18 ------------------ 4 files changed, 21 insertions(+), 35 deletions(-) diff --git a/components/layout/traversal.rs b/components/layout/traversal.rs index 1362f1cb7b2..b1087e945d7 100644 --- a/components/layout/traversal.rs +++ b/components/layout/traversal.rs @@ -77,7 +77,9 @@ impl<'lc, N> DomTraversalContext for RecalcStyleAndConstructFlows<'lc> recalc_style_at(&self.context, self.root, node); } - fn process_postorder(&self, node: N) { construct_flows_at(&self.context, self.root, node); } + fn process_postorder(&self, node: N) { + construct_flows_at(&self.context, self.root, node); + } } /// A bottom-up, parallelizable traversal. @@ -96,7 +98,7 @@ fn construct_flows_at<'a, N: LayoutNode>(context: &'a LayoutContext<'a>, root: O // Always reconstruct if incremental layout is turned off. let nonincremental_layout = opts::get().nonincremental_layout; - if nonincremental_layout || node.has_dirty_descendants() { + if nonincremental_layout || node.is_dirty() || node.has_dirty_descendants() { let mut flow_constructor = FlowConstructor::new(context); if nonincremental_layout || !flow_constructor.repair_if_possible(&tnode) { flow_constructor.process(&tnode); diff --git a/components/script/dom/node.rs b/components/script/dom/node.rs index 37495be3ba3..0e5e360c6e1 100644 --- a/components/script/dom/node.rs +++ b/components/script/dom/node.rs @@ -479,19 +479,7 @@ impl Node { return } - // 2. Dirty descendants. - fn dirty_subtree(node: &Node) { - // Stop if this subtree is already dirty. - if node.is_dirty() { return } - - node.set_flag(IS_DIRTY | HAS_DIRTY_DESCENDANTS, true); - - for kid in node.children() { - dirty_subtree(kid.r()); - } - } - - dirty_subtree(self); + self.set_flag(IS_DIRTY, true); // 4. Dirty ancestors. for ancestor in self.ancestors() { diff --git a/components/style/traversal.rs b/components/style/traversal.rs index 69521c1557a..c0d197896af 100644 --- a/components/style/traversal.rs +++ b/components/style/traversal.rs @@ -153,10 +153,24 @@ pub trait DomTraversalContext { /// /// Note that this is true unconditionally for servo, since it requires to /// bubble the widths bottom-up for all the DOM. - fn should_process(&self, _node: N) -> bool { true } + fn should_process(&self, node: N) -> bool { + node.is_dirty() || node.has_dirty_descendants() + } /// Do an action over the child before pushing him to the work queue. - fn pre_process_child_hook(&self, _parent: N, _kid: N) {} + /// + /// By default, propagate the IS_DIRTY flag down the tree. + #[allow(unsafe_code)] + fn pre_process_child_hook(&self, parent: N, kid: N) { + // NOTE: At this point is completely safe to modify either the parent or + // the child, since we have exclusive access to both of them. + if parent.is_dirty() { + unsafe { + kid.set_dirty(true); + parent.set_dirty_descendants(true); + } + } + } } /// Calculates the style for a single node. diff --git a/ports/geckolib/traversal.rs b/ports/geckolib/traversal.rs index 562153baf07..87b63e83e15 100644 --- a/ports/geckolib/traversal.rs +++ b/ports/geckolib/traversal.rs @@ -6,7 +6,6 @@ use context::StandaloneStyleContext; use std::mem; use style::context::SharedStyleContext; use style::dom::OpaqueNode; -use style::dom::TNode; use style::traversal::{DomTraversalContext, recalc_style_at}; use wrapper::GeckoNode; @@ -37,21 +36,4 @@ impl<'lc, 'ln> DomTraversalContext> for RecalcStyleOnly<'lc> { } fn process_postorder(&self, _: GeckoNode<'ln>) {} - - /// In Gecko we use this traversal just for restyling, so we can stop once - /// we know there aren't more dirty nodes under ourselves. - fn should_process(&self, node: GeckoNode<'ln>) -> bool { - node.is_dirty() || node.has_dirty_descendants() - } - - fn pre_process_child_hook(&self, parent: GeckoNode<'ln>, kid: GeckoNode<'ln>) { - // NOTE: At this point is completely safe to modify either the parent or - // the child, since we have exclusive access to them. - if parent.is_dirty() { - unsafe { - kid.set_dirty(true); - parent.set_dirty_descendants(true); - } - } - } } From e6958d3947adedda7f3f52f26fae321f0b4d0ad4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Sun, 24 Jul 2016 04:26:54 -0700 Subject: [PATCH 04/10] script: Fix a few load related bugs. This is what was making me hit the new test failures. So turns out that when the DOMContentLoaded event is fired we fired no messages to the constellation, but we fired the DOMLoad message from the DocumentProgressHandler, effectively after having dispatched the Load message from script thread. This also fixes the possibility of a subframe navigation not blocking the load event of the parent document, for example. --- components/compositing/compositor.rs | 2 +- components/constellation/constellation.rs | 10 ++++++---- components/script/dom/document.rs | 22 +++++++++++++++------- components/script/script_thread.rs | 2 -- 4 files changed, 22 insertions(+), 14 deletions(-) diff --git a/components/compositing/compositor.rs b/components/compositing/compositor.rs index da9272d08b9..60b64a03047 100644 --- a/components/compositing/compositor.rs +++ b/components/compositing/compositor.rs @@ -2114,7 +2114,7 @@ impl IOCompositor { } // Check if there are any pending frames. If so, the image is not stable yet. - if self.pending_subpages.len() > 0 { + if !self.pending_subpages.is_empty() { return Err(NotReadyToPaint::PendingSubpages(self.pending_subpages.len())); } diff --git a/components/constellation/constellation.rs b/components/constellation/constellation.rs index 7b1ae044921..f954fda7668 100644 --- a/components/constellation/constellation.rs +++ b/components/constellation/constellation.rs @@ -1441,6 +1441,7 @@ impl Constellation let root = self.root_frame_id.is_none() || self.root_frame_id == Some(frame_id); self.compositor_proxy.send(ToCompositorMsg::LoadComplete(back, forward, root)); } + self.handle_subframe_loaded(pipeline_id); } fn handle_dom_load(&mut self, pipeline_id: PipelineId) { @@ -1455,8 +1456,6 @@ impl Constellation if webdriver_reset { self.webdriver.load_channel = None; } - - self.handle_subframe_loaded(pipeline_id); } fn handle_traverse_history_msg(&mut self, @@ -2073,7 +2072,7 @@ impl Constellation } // If there are pending loads, wait for those to complete. - if self.pending_frames.len() > 0 { + if !self.pending_frames.is_empty() { return ReadyToSave::PendingFrames; } @@ -2089,7 +2088,10 @@ impl Constellation let pipeline_id = frame.current.0; let pipeline = match self.pipelines.get(&pipeline_id) { - None => { warn!("Pipeline {:?} screenshot while closing.", pipeline_id); continue; }, + None => { + warn!("Pipeline {:?} screenshot while closing.", pipeline_id); + continue; + }, Some(pipeline) => pipeline, }; diff --git a/components/script/dom/document.rs b/components/script/dom/document.rs index 28edce553ad..14e80de6346 100644 --- a/components/script/dom/document.rs +++ b/components/script/dom/document.rs @@ -376,6 +376,7 @@ impl Document { // that workable. match self.GetDocumentElement() { Some(root) => { + root.upcast::().is_dirty() || root.upcast::().has_dirty_descendants() || !self.modified_elements.borrow().is_empty() } @@ -1371,6 +1372,7 @@ impl Document { } pub fn finish_load(&self, load: LoadType) { + debug!("Document got finish_load: {:?}", load); // The parser might need the loader, so restrict the lifetime of the borrow. { let mut loader = self.loader.borrow_mut(); @@ -1396,9 +1398,9 @@ impl Document { // If we don't have a parser, and the reflow timer has been reset, explicitly // trigger a reflow. if let LoadType::Stylesheet(_) = load { - self.window().reflow(ReflowGoal::ForDisplay, - ReflowQueryType::NoQuery, - ReflowReason::StylesheetLoaded); + self.window.reflow(ReflowGoal::ForDisplay, + ReflowQueryType::NoQuery, + ReflowReason::StylesheetLoaded); } } @@ -1487,6 +1489,8 @@ impl Document { return; } self.domcontentloaded_dispatched.set(true); + assert!(self.ReadyState() != DocumentReadyState::Complete, + "Complete before DOMContentLoaded?"); update_with_current_time_ms(&self.dom_content_loaded_event_start); @@ -1497,14 +1501,17 @@ impl Document { ReflowQueryType::NoQuery, ReflowReason::DOMContentLoaded); + let pipeline_id = self.window.pipeline(); + let event = ConstellationMsg::DOMLoad(pipeline_id); + self.window.constellation_chan().send(event).unwrap(); + update_with_current_time_ms(&self.dom_content_loaded_event_end); } pub fn notify_constellation_load(&self) { let pipeline_id = self.window.pipeline(); - let event = ConstellationMsg::DOMLoad(pipeline_id); - self.window.constellation_chan().send(event).unwrap(); - + let load_event = ConstellationMsg::LoadComplete(pipeline_id); + self.window.constellation_chan().send(load_event).unwrap(); } pub fn set_current_parser(&self, script: Option) { @@ -2913,11 +2920,12 @@ impl DocumentProgressHandler { // http://w3c.github.io/navigation-timing/#widl-PerformanceNavigationTiming-loadEventEnd update_with_current_time_ms(&document.load_event_end); - document.notify_constellation_load(); window.reflow(ReflowGoal::ForDisplay, ReflowQueryType::NoQuery, ReflowReason::DocumentLoaded); + + document.notify_constellation_load(); } } diff --git a/components/script/script_thread.rs b/components/script/script_thread.rs index cf0cebf5a65..fbfedae13e9 100644 --- a/components/script/script_thread.rs +++ b/components/script/script_thread.rs @@ -1193,8 +1193,6 @@ impl ScriptThread { // https://html.spec.whatwg.org/multipage/#the-end step 7 let handler = box DocumentProgressHandler::new(Trusted::new(doc)); self.dom_manipulation_task_source.queue(handler, GlobalRef::Window(doc.window())).unwrap(); - - self.constellation_chan.send(ConstellationMsg::LoadComplete(pipeline)).unwrap(); } fn collect_reports(&self, reports_chan: ReportsChan) { From 702445a3ec4d406472a618750f5dab64c38459bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Sun, 24 Jul 2016 14:04:17 -0700 Subject: [PATCH 05/10] script: Don't hold the stderr lock while doing sync operations with the constellation. Otherwise if you enable debug logging, you deadlock. --- components/script/dom/window.rs | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/components/script/dom/window.rs b/components/script/dom/window.rs index 2237514bd3d..018c616bca5 100644 --- a/components/script/dom/window.rs +++ b/components/script/dom/window.rs @@ -450,13 +450,15 @@ impl WindowMethods for Window { // Right now, just print to the console // Ensure that stderr doesn't trample through the alert() we use to // communicate test results (see executorservo.py in wptrunner). - let stderr = stderr(); - let mut stderr = stderr.lock(); - let stdout = stdout(); - let mut stdout = stdout.lock(); - writeln!(&mut stdout, "ALERT: {}", s).unwrap(); - stdout.flush().unwrap(); - stderr.flush().unwrap(); + { + let stderr = stderr(); + let mut stderr = stderr.lock(); + let stdout = stdout(); + let mut stdout = stdout.lock(); + writeln!(&mut stdout, "ALERT: {}", s).unwrap(); + stdout.flush().unwrap(); + stderr.flush().unwrap(); + } let (sender, receiver) = ipc::channel().unwrap(); self.constellation_chan().send(ConstellationMsg::Alert(self.pipeline(), s.to_string(), sender)).unwrap(); From 354dc660292b798bb0beafcc39a98e2519518e53 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Sun, 24 Jul 2016 12:43:21 -0700 Subject: [PATCH 06/10] Some debugging improvements and code style nits across gfx and constellation. * Propagate the RUST_LOG env var to the child process * Add debug information when a recv() fails in paint thread's select!. --- components/constellation/constellation.rs | 7 +++++-- components/constellation/pipeline.rs | 8 ++++++++ components/gfx/paint_thread.rs | 7 +++---- components/script/dom/document.rs | 1 + 4 files changed, 17 insertions(+), 6 deletions(-) diff --git a/components/constellation/constellation.rs b/components/constellation/constellation.rs index f954fda7668..2d9fbac74d9 100644 --- a/components/constellation/constellation.rs +++ b/components/constellation/constellation.rs @@ -1323,10 +1323,13 @@ impl Constellation } } - fn handle_alert(&mut self, pipeline_id: PipelineId, message: String, sender: IpcSender) { + fn handle_alert(&mut self, + pipeline_id: PipelineId, + message: String, + sender: IpcSender) { let display_alert_dialog = if PREFS.is_mozbrowser_enabled() { let parent_pipeline_info = self.pipelines.get(&pipeline_id).and_then(|source| source.parent_info); - if let Some(_) = parent_pipeline_info { + if parent_pipeline_info.is_some() { let root_pipeline_id = self.root_frame_id .and_then(|root_frame_id| self.frames.get(&root_frame_id)) .map(|root_frame| root_frame.current.0); diff --git a/components/constellation/pipeline.rs b/components/constellation/pipeline.rs index dd2a665f6ac..3c076e904e0 100644 --- a/components/constellation/pipeline.rs +++ b/components/constellation/pipeline.rs @@ -501,6 +501,10 @@ impl UnprivilegedPipelineContent { command.env("RUST_BACKTRACE", value); } + if let Ok(value) = env::var("RUST_LOG") { + command.env("RUST_LOG", value); + } + let profile = content_process_sandbox_profile(); ChildProcess::Sandboxed(Sandbox::new(profile).start(&mut command) .expect("Failed to start sandboxed child process!")) @@ -515,6 +519,10 @@ impl UnprivilegedPipelineContent { child_process.env("RUST_BACKTRACE", value); } + if let Ok(value) = env::var("RUST_LOG") { + child_process.env("RUST_LOG", value); + } + ChildProcess::Unsandboxed(child_process.spawn() .expect("Failed to start unsandboxed child process!")) }; diff --git a/components/gfx/paint_thread.rs b/components/gfx/paint_thread.rs index b4ad2abd299..6fad77dbcf3 100644 --- a/components/gfx/paint_thread.rs +++ b/components/gfx/paint_thread.rs @@ -379,8 +379,7 @@ impl PaintThread where C: PaintListener + Send + 'static { font_cache_thread: FontCacheThread, time_profiler_chan: time::ProfilerChan, mem_profiler_chan: mem::ProfilerChan) { - thread::spawn_named(format!("PaintThread {:?}", id), - move || { + thread::spawn_named(format!("PaintThread {:?}", id), move || { thread_state::initialize(thread_state::PAINT); PipelineId::install(id); @@ -425,9 +424,9 @@ impl PaintThread where C: PaintListener + Send + 'static { let chrome_to_paint = &self.chrome_to_paint_port; select! { msg = layout_to_paint.recv() => - Msg::FromLayout(msg.unwrap()), + Msg::FromLayout(msg.expect("expected message from layout")), msg = chrome_to_paint.recv() => - Msg::FromChrome(msg.unwrap()) + Msg::FromChrome(msg.expect("expected message from chrome")) } }; diff --git a/components/script/dom/document.rs b/components/script/dom/document.rs index 14e80de6346..73ca4f16f3c 100644 --- a/components/script/dom/document.rs +++ b/components/script/dom/document.rs @@ -2915,6 +2915,7 @@ impl DocumentProgressHandler { // http://w3c.github.io/navigation-timing/#widl-PerformanceNavigationTiming-loadEventStart update_with_current_time_ms(&document.load_event_start); + debug!("About to dispatch load for {:?}", document.url()); let _ = wintarget.dispatch_event_with_target(document.upcast(), &event); // http://w3c.github.io/navigation-timing/#widl-PerformanceNavigationTiming-loadEventEnd From c3a727ebda5bc9a0d58c8895c7d4626790a9e45d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Mon, 25 Jul 2016 21:16:53 -0700 Subject: [PATCH 07/10] style: Remove a few more unuseful traversals now we can. --- components/layout_thread/lib.rs | 18 ++++++---- components/script/dom/node.rs | 40 +++++++++++++++------- components/script/layout_wrapper.rs | 9 ----- components/style/dom.rs | 53 +++++++++-------------------- components/style/traversal.rs | 17 ++++----- 5 files changed, 64 insertions(+), 73 deletions(-) diff --git a/components/layout_thread/lib.rs b/components/layout_thread/lib.rs index 9d576e3bd02..dc1d7b179eb 100644 --- a/components/layout_thread/lib.rs +++ b/components/layout_thread/lib.rs @@ -1097,12 +1097,16 @@ impl LayoutThread { .unwrap(); } if data.document_stylesheets.iter().any(|sheet| sheet.dirty_on_viewport_size_change) { - for node in node.traverse_preorder() { + let mut iter = node.traverse_preorder(); + + let mut next = iter.next(); + while let Some(node) = next { if node.needs_dirty_on_viewport_size_changed() { - node.dirty_self(); - node.dirty_descendants(); - // TODO(shinglyu): We can skip the traversal if the descendants were already - // dirtied + // NB: The dirty bit is propagated down the tree. + unsafe { node.set_dirty(true); } + next = iter.next_skipping_children(); + } else { + next = iter.next(); } } } @@ -1114,7 +1118,9 @@ impl LayoutThread { let needs_reflow = viewport_size_changed && !needs_dirtying; unsafe { if needs_dirtying { - LayoutThread::dirty_all_nodes(node); + // NB: The dirty flag is propagated down during the restyle + // process. + node.set_dirty(true); } } if needs_reflow { diff --git a/components/script/dom/node.rs b/components/script/dom/node.rs index 0e5e360c6e1..2769b7503bc 100644 --- a/components/script/dom/node.rs +++ b/components/script/dom/node.rs @@ -1287,6 +1287,31 @@ impl TreeIterator { depth: 0, } } + + pub fn next_skipping_children(&mut self) -> Option> { + let current = match self.current.take() { + None => return None, + Some(current) => current, + }; + + self.next_skipping_children_impl(current) + } + + fn next_skipping_children_impl(&mut self, current: Root) -> Option> { + for ancestor in current.inclusive_ancestors() { + if self.depth == 0 { + break; + } + if let Some(next_sibling) = ancestor.GetNextSibling() { + self.current = Some(next_sibling); + return Some(current); + } + self.depth -= 1; + } + debug_assert!(self.depth == 0); + self.current = None; + Some(current) + } } impl Iterator for TreeIterator { @@ -1303,19 +1328,8 @@ impl Iterator for TreeIterator { self.depth += 1; return Some(current); }; - for ancestor in current.inclusive_ancestors() { - if self.depth == 0 { - break; - } - if let Some(next_sibling) = ancestor.GetNextSibling() { - self.current = Some(next_sibling); - return Some(current); - } - self.depth -= 1; - } - debug_assert!(self.depth == 0); - self.current = None; - Some(current) + + self.next_skipping_children_impl(current) } } diff --git a/components/script/layout_wrapper.rs b/components/script/layout_wrapper.rs index df44774dd5a..bfd7a8273d9 100644 --- a/components/script/layout_wrapper.rs +++ b/components/script/layout_wrapper.rs @@ -199,15 +199,6 @@ impl<'ln> TNode for ServoLayoutNode<'ln> { self.node.set_flag(DIRTY_ON_VIEWPORT_SIZE_CHANGE, true); } - fn set_descendants_dirty_on_viewport_size_changed(&self) { - for ref child in self.children() { - unsafe { - child.set_dirty_on_viewport_size_changed(); - } - child.set_descendants_dirty_on_viewport_size_changed(); - } - } - fn can_be_fragmented(&self) -> bool { unsafe { self.node.get_flag(CAN_BE_FRAGMENTED) } } diff --git a/components/style/dom.rs b/components/style/dom.rs index 5c5f3905d8d..c694ca75732 100644 --- a/components/style/dom.rs +++ b/components/style/dom.rs @@ -111,33 +111,10 @@ pub trait TNode : Sized + Copy + Clone { unsafe fn set_dirty_descendants(&self, value: bool); - fn dirty_self(&self) { - unsafe { - self.set_dirty(true); - self.set_dirty_descendants(true); - } - } - - fn dirty_descendants(&self) { - for ref child in self.children() { - child.dirty_self(); - child.dirty_descendants(); - } - } - fn needs_dirty_on_viewport_size_changed(&self) -> bool; unsafe fn set_dirty_on_viewport_size_changed(&self); - fn set_descendants_dirty_on_viewport_size_changed(&self) { - for ref child in self.children() { - unsafe { - child.set_dirty_on_viewport_size_changed(); - } - child.set_descendants_dirty_on_viewport_size_changed(); - } - } - fn can_be_fragmented(&self) -> bool; unsafe fn set_can_be_fragmented(&self, value: bool); @@ -215,7 +192,7 @@ pub trait TElement : Sized + Copy + Clone + ElementExt + PresentationalHintsSynt fn attr_equals(&self, namespace: &Namespace, attr: &Atom, value: &Atom) -> bool; /// Properly marks nodes as dirty in response to restyle hints. - fn note_restyle_hint(&self, mut hint: RestyleHint) { + fn note_restyle_hint(&self, hint: RestyleHint) { // Bail early if there's no restyling to do. if hint.is_empty() { return; @@ -233,23 +210,21 @@ pub trait TElement : Sized + Copy + Clone + ElementExt + PresentationalHintsSynt // Process hints. if hint.contains(RESTYLE_SELF) { - node.dirty_self(); + unsafe { node.set_dirty(true); } + // XXX(emilio): For now, dirty implies dirty descendants if found. + } else if hint.contains(RESTYLE_DESCENDANTS) { + let mut current = node.first_child(); + while let Some(node) = current { + unsafe { node.set_dirty(true); } + current = node.next_sibling(); + } + } - // FIXME(bholley, #8438): We currently need to RESTYLE_DESCENDANTS in the - // RESTYLE_SELF case in order to make sure "inherit" style structs propagate - // properly. See the explanation in the github issue. - hint.insert(RESTYLE_DESCENDANTS); - } - if hint.contains(RESTYLE_DESCENDANTS) { - unsafe { node.set_dirty_descendants(true); } - node.dirty_descendants(); - } if hint.contains(RESTYLE_LATER_SIBLINGS) { let mut next = ::selectors::Element::next_sibling_element(self); while let Some(sib) = next { let sib_node = sib.as_node(); - sib_node.dirty_self(); - sib_node.dirty_descendants(); + unsafe { sib_node.set_dirty(true) }; next = ::selectors::Element::next_sibling_element(&sib); } } @@ -262,12 +237,16 @@ pub struct TreeIterator where ConcreteNode: TNode { impl TreeIterator where ConcreteNode: TNode { fn new(root: ConcreteNode) -> TreeIterator { - let mut stack = vec!(); + let mut stack = vec![]; stack.push(root); TreeIterator { stack: stack, } } + + pub fn next_skipping_children(&mut self) -> Option { + self.stack.pop() + } } impl Iterator for TreeIterator diff --git a/components/style/traversal.rs b/components/style/traversal.rs index c0d197896af..efdf96b0c35 100644 --- a/components/style/traversal.rs +++ b/components/style/traversal.rs @@ -268,16 +268,17 @@ pub fn recalc_style_at<'a, N, C>(context: &'a C, // NB: flow construction updates the bloom filter on the way up. put_thread_local_bloom_filter(bf, &unsafe_layout_node, context.shared_context()); - // Mark the node as DIRTY_ON_VIEWPORT_SIZE_CHANGE is it uses viewport percentage units. - if let Some(element) = node.as_element() { - if let Some(ref property_declaration_block) = *element.style_attribute() { - if property_declaration_block.declarations().any(|d| d.0.has_viewport_percentage()) { - unsafe { - node.set_dirty_on_viewport_size_changed(); + // Mark the node as DIRTY_ON_VIEWPORT_SIZE_CHANGE is it uses viewport + // percentage units. + if !node.needs_dirty_on_viewport_size_changed() { + if let Some(element) = node.as_element() { + if let Some(ref property_declaration_block) = *element.style_attribute() { + if property_declaration_block.declarations().any(|d| d.0.has_viewport_percentage()) { + unsafe { + node.set_dirty_on_viewport_size_changed(); + } } - node.set_descendants_dirty_on_viewport_size_changed(); } } } } - From 572b3c31bd7df82bd47b10381c3bff80e75c42b0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Mon, 25 Jul 2016 21:26:49 -0700 Subject: [PATCH 08/10] style: Add a comment about pre_process_children_hook. --- components/style/parallel.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/components/style/parallel.rs b/components/style/parallel.rs index 889f7f0a15b..b82727657c3 100644 --- a/components/style/parallel.rs +++ b/components/style/parallel.rs @@ -73,6 +73,12 @@ fn top_down_dom(unsafe_nodes: UnsafeNodeList, // Possibly enqueue the children. let mut children_to_process = 0isize; for kid in node.children() { + // Trigger the hook pre-adding the kid to the list. This can (and in + // fact uses to) change the result of the should_process operation. + // + // As of right now, this hook takes care of propagating the restyle + // flag down the tree. In the future, more accurate behavior is + // probably going to be needed. context.pre_process_child_hook(node, kid); if context.should_process(kid) { children_to_process += 1; From 36376461f4e1a2f4b3db7f940029df7dcf24be20 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Wed, 27 Jul 2016 10:34:36 -0700 Subject: [PATCH 09/10] script: Unify LoadComplete and DOMLoad messages. See the PR in which this commit landed and also https://github.com/servo/servo/pull/6415#issuecomment-122294169 --- components/constellation/constellation.rs | 22 +++++++--------------- components/script/dom/document.rs | 6 +----- components/script_traits/script_msg.rs | 7 ++----- 3 files changed, 10 insertions(+), 25 deletions(-) diff --git a/components/constellation/constellation.rs b/components/constellation/constellation.rs index 2d9fbac74d9..d2f045de0d1 100644 --- a/components/constellation/constellation.rs +++ b/components/constellation/constellation.rs @@ -828,11 +828,6 @@ impl Constellation debug!("constellation got load complete message"); self.handle_load_complete_msg(pipeline_id) } - // The DOM load event fired on a document - FromScriptMsg::DOMLoad(pipeline_id) => { - debug!("constellation got dom load message"); - self.handle_dom_load(pipeline_id) - } // Handle a forward or back request FromScriptMsg::TraverseHistory(pipeline_id, direction) => { debug!("constellation got traverse history message from script"); @@ -1438,16 +1433,6 @@ impl Constellation } fn handle_load_complete_msg(&mut self, pipeline_id: PipelineId) { - if let Some(frame_id) = self.get_top_level_frame_for_pipeline(Some(pipeline_id)) { - let forward = !self.joint_session_future(frame_id).is_empty(); - let back = !self.joint_session_past(frame_id).is_empty(); - let root = self.root_frame_id.is_none() || self.root_frame_id == Some(frame_id); - self.compositor_proxy.send(ToCompositorMsg::LoadComplete(back, forward, root)); - } - self.handle_subframe_loaded(pipeline_id); - } - - fn handle_dom_load(&mut self, pipeline_id: PipelineId) { let mut webdriver_reset = false; if let Some((expected_pipeline_id, ref reply_chan)) = self.webdriver.load_channel { debug!("Sending load to WebDriver"); @@ -1459,6 +1444,13 @@ impl Constellation if webdriver_reset { self.webdriver.load_channel = None; } + if let Some(frame_id) = self.get_top_level_frame_for_pipeline(Some(pipeline_id)) { + let forward = !self.joint_session_future(frame_id).is_empty(); + let back = !self.joint_session_past(frame_id).is_empty(); + let root = self.root_frame_id.is_none() || self.root_frame_id == Some(frame_id); + self.compositor_proxy.send(ToCompositorMsg::LoadComplete(back, forward, root)); + } + self.handle_subframe_loaded(pipeline_id); } fn handle_traverse_history_msg(&mut self, diff --git a/components/script/dom/document.rs b/components/script/dom/document.rs index 73ca4f16f3c..78ccbbdf1f5 100644 --- a/components/script/dom/document.rs +++ b/components/script/dom/document.rs @@ -1497,14 +1497,10 @@ impl Document { let window = self.window(); window.dom_manipulation_task_source().queue_event(self.upcast(), atom!("DOMContentLoaded"), EventBubbles::Bubbles, EventCancelable::NotCancelable, window); + window.reflow(ReflowGoal::ForDisplay, ReflowQueryType::NoQuery, ReflowReason::DOMContentLoaded); - - let pipeline_id = self.window.pipeline(); - let event = ConstellationMsg::DOMLoad(pipeline_id); - self.window.constellation_chan().send(event).unwrap(); - update_with_current_time_ms(&self.dom_content_loaded_event_end); } diff --git a/components/script_traits/script_msg.rs b/components/script_traits/script_msg.rs index 3ba4494432b..cc8df29d3d5 100644 --- a/components/script_traits/script_msg.rs +++ b/components/script_traits/script_msg.rs @@ -70,10 +70,6 @@ pub enum ScriptMsg { CreateWebGLPaintThread(Size2D, GLContextAttributes, IpcSender, GLLimits), String>>), - /// Dispatched after the DOM load event has fired on a document - /// Causes a `load` event to be dispatched to any enclosing frame context element - /// for the given pipeline. - DOMLoad(PipelineId), /// Notifies the constellation that this frame has received focus. Focus(PipelineId), /// Re-send a mouse button event that was sent to the parent window. @@ -84,7 +80,8 @@ pub enum ScriptMsg { GetClipboardContents(IpcSender), /// tag finished parsing HeadParsed, - /// All pending loads are complete. + /// All pending loads are complete, and the `load` event for this pipeline + /// has been dispatched. LoadComplete(PipelineId), /// A new load has been requested. LoadUrl(PipelineId, LoadData), From f6043a23942816989c51798c069c11e8e8726882 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Wed, 27 Jul 2016 10:35:34 -0700 Subject: [PATCH 10/10] layout: Remove now unused dirty_all_nodes function in layout thread. --- components/layout_thread/lib.rs | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/components/layout_thread/lib.rs b/components/layout_thread/lib.rs index dc1d7b179eb..0d7c9dce2bc 100644 --- a/components/layout_thread/lib.rs +++ b/components/layout_thread/lib.rs @@ -1474,16 +1474,6 @@ impl LayoutThread { } } - unsafe fn dirty_all_nodes(node: N) { - for node in node.traverse_preorder() { - // TODO(cgaebel): mark nodes which are sensitive to media queries as - // "changed": - // > node.set_changed(true); - node.set_dirty(true); - node.set_dirty_descendants(true); - } - } - fn reflow_all_nodes(flow: &mut Flow) { debug!("reflowing all nodes!"); flow::mut_base(flow).restyle_damage.insert(REPAINT | STORE_OVERFLOW | REFLOW);