From 630b9f8fa0b953838b1778a3eca331c7e970cbc9 Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Fri, 9 Oct 2015 21:01:38 -0700 Subject: [PATCH 1/4] Implement Hash and Eq for JS and LayoutJS. --- components/script/dom/bindings/js.rs | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/components/script/dom/bindings/js.rs b/components/script/dom/bindings/js.rs index a2ec90033ba..9a4f17b426a 100644 --- a/components/script/dom/bindings/js.rs +++ b/components/script/dom/bindings/js.rs @@ -35,6 +35,7 @@ use layout_interface::TrustedNodeAddress; use script_task::STACK_ROOTS; use std::cell::UnsafeCell; use std::default::Default; +use std::hash::{Hash, Hasher}; use std::mem; use std::ops::Deref; use std::ptr; @@ -147,12 +148,24 @@ impl PartialEq for JS { } } +impl Eq for JS {} + impl PartialEq for LayoutJS { fn eq(&self, other: &LayoutJS) -> bool { self.ptr == other.ptr } } +impl Eq for LayoutJS {} + +impl Hash for JS { + fn hash(&self, state: &mut H) { self.ptr.hash(state) } +} + +impl Hash for LayoutJS { + fn hash(&self, state: &mut H) { self.ptr.hash(state) } +} + impl Clone for JS { #[inline] #[allow(unrooted_must_root)] From 85596b55106775d1eef948c39708c1b02499a9a4 Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Tue, 20 Oct 2015 15:40:47 -0700 Subject: [PATCH 2/4] Use an RAII guard to join the script task. --- components/layout/layout_task.rs | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/components/layout/layout_task.rs b/components/layout/layout_task.rs index 9c3a3e7ea4c..f3fd3546671 100644 --- a/components/layout/layout_task.rs +++ b/components/layout/layout_task.rs @@ -67,7 +67,7 @@ use std::cell::Cell; use std::collections::HashMap; use std::collections::hash_state::DefaultState; use std::mem::transmute; -use std::ops::{Deref, DerefMut}; +use std::ops::{Deref, DerefMut, Drop}; use std::sync::atomic::{AtomicUsize, Ordering}; use std::sync::mpsc::{channel, Sender, Receiver}; use std::sync::{Arc, Mutex, MutexGuard}; @@ -1125,6 +1125,17 @@ impl LayoutTask { fn handle_reflow<'a>(&'a self, data: &ScriptReflow, possibly_locked_rw_data: &mut Option>) { + + // Make sure that every return path from this method joins the script task, + // otherwise the script task will panic. + struct AutoJoinScriptTask<'a> { data: &'a ScriptReflow }; + impl<'a> Drop for AutoJoinScriptTask<'a> { + fn drop(&mut self) { + self.data.script_join_chan.send(()).unwrap(); + } + }; + let _ajst = AutoJoinScriptTask { data: data }; + // FIXME: Isolate this transmutation into a "bridge" module. // FIXME(rust#16366): The following line had to be moved because of a // rustc bug. It should be in the next unsafe block. @@ -1245,9 +1256,6 @@ impl LayoutTask { ReflowQueryType::NoQuery => {} } } - - // Tell script that we're done. - data.script_join_chan.send(()).unwrap(); } fn set_visible_rects<'a>(&'a self, From 441c84d75d412d24281b67821662ee50e8b1152b Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Mon, 12 Oct 2015 17:43:49 -0700 Subject: [PATCH 3/4] Pass the document instead of the documentElement to reflow. --- components/layout/layout_task.rs | 29 ++++++++++++++----------- components/layout/wrapper.rs | 31 +++++++++++++++++++++++++++ components/script/dom/document.rs | 5 ++--- components/script/dom/window.rs | 10 +-------- components/script/layout_interface.rs | 2 +- 5 files changed, 51 insertions(+), 26 deletions(-) diff --git a/components/layout/layout_task.rs b/components/layout/layout_task.rs index f3fd3546671..4ea12e58796 100644 --- a/components/layout/layout_task.rs +++ b/components/layout/layout_task.rs @@ -52,6 +52,7 @@ use query::{LayoutRPCImpl, process_content_box_request, process_content_boxes_re use query::{MarginPadding, MarginRetrievingFragmentBorderBoxIterator, PositionProperty}; use query::{PositionRetrievingFragmentBorderBoxIterator, Side}; use script::dom::bindings::js::LayoutJS; +use script::dom::document::Document; use script::dom::node::{LayoutData, Node}; use script::layout_interface::Animation; use script::layout_interface::{LayoutChan, LayoutRPC, OffsetParentResponse}; @@ -89,8 +90,7 @@ use util::opts; use util::task::spawn_named_with_send_on_failure; use util::task_state; use util::workqueue::WorkQueue; -use wrapper::LayoutNode; -use wrapper::ThreadSafeLayoutNode; +use wrapper::{LayoutDocument, LayoutNode, ThreadSafeLayoutNode}; /// The number of screens of data we're allowed to generate display lists for in each direction. pub const DISPLAY_PORT_SIZE_FACTOR: i32 = 8; @@ -1137,13 +1137,16 @@ impl LayoutTask { let _ajst = AutoJoinScriptTask { data: data }; // FIXME: Isolate this transmutation into a "bridge" module. - // FIXME(rust#16366): The following line had to be moved because of a - // rustc bug. It should be in the next unsafe block. - let mut node: LayoutJS = unsafe { - LayoutJS::from_trusted_node_address(data.document_root) + let mut doc: LayoutJS = unsafe { + LayoutJS::from_trusted_node_address(data.document).downcast::().unwrap() }; - let node: &mut LayoutNode = unsafe { - transmute(&mut node) + let doc: &mut LayoutDocument = unsafe { + transmute(&mut doc) + }; + + let mut node: LayoutNode = match doc.root_node() { + None => return, + Some(x) => x, }; debug!("layout: received layout request for: {}", self.url.serialize()); @@ -1187,11 +1190,11 @@ impl LayoutTask { let needs_reflow = screen_size_changed && !needs_dirtying; unsafe { if needs_dirtying { - LayoutTask::dirty_all_nodes(node); + LayoutTask::dirty_all_nodes(&mut node); } } if needs_reflow { - if let Some(mut flow) = self.try_get_layout_root(*node) { + if let Some(mut flow) = self.try_get_layout_root(node) { LayoutTask::reflow_all_nodes(flow_ref::deref_mut(&mut flow)); } } @@ -1213,16 +1216,16 @@ impl LayoutTask { let rw_data = &mut *rw_data; match rw_data.parallel_traversal { None => { - sequential::traverse_dom_preorder(*node, &shared_layout_context); + sequential::traverse_dom_preorder(node, &shared_layout_context); } Some(ref mut traversal) => { - parallel::traverse_dom_preorder(*node, &shared_layout_context, traversal); + parallel::traverse_dom_preorder(node, &shared_layout_context, traversal); } } }); // Retrieve the (possibly rebuilt) root flow. - rw_data.root_flow = self.try_get_layout_root((*node).clone()); + rw_data.root_flow = self.try_get_layout_root(node.clone()); } // Send new canvas renderers to the paint task diff --git a/components/layout/wrapper.rs b/components/layout/wrapper.rs index c59fb978e38..d42fa386ec2 100644 --- a/components/layout/wrapper.rs +++ b/components/layout/wrapper.rs @@ -43,6 +43,7 @@ use script::dom::bindings::codegen::InheritTypes::{HTMLElementTypeId, NodeTypeId use script::dom::bindings::conversions::Castable; use script::dom::bindings::js::LayoutJS; use script::dom::characterdata::LayoutCharacterDataHelpers; +use script::dom::document::{Document, LayoutDocumentHelpers}; use script::dom::element; use script::dom::element::{Element, LayoutElementHelpers, RawLayoutElementHelpers}; use script::dom::htmlcanvaselement::{LayoutHTMLCanvasElementHelpers, HTMLCanvasData}; @@ -105,6 +106,12 @@ impl<'ln> LayoutNode<'ln> { } } + pub fn is_element(&self) -> bool { + unsafe { + self.node.is_element_for_layout() + } + } + pub fn dump(self) { self.dump_indent(0); } @@ -336,6 +343,30 @@ impl<'a> Iterator for LayoutTreeIterator<'a> { } } +// A wrapper around documents that ensures ayout can only ever access safe properties. +#[derive(Copy, Clone)] +pub struct LayoutDocument<'le> { + document: LayoutJS, + chain: PhantomData<&'le ()>, +} + +impl<'le> LayoutDocument<'le> { + pub fn as_node(&self) -> LayoutNode<'le> { + LayoutNode { + node: self.document.upcast(), + chain: PhantomData, + } + } + + pub fn root_node(&self) -> Option> { + let mut node = self.as_node().first_child(); + while node.is_some() && !node.unwrap().is_element() { + node = node.unwrap().next_sibling(); + } + node + } +} + /// A wrapper around elements that ensures layout can only ever access safe properties. #[derive(Copy, Clone)] pub struct LayoutElement<'le> { diff --git a/components/script/dom/document.rs b/components/script/dom/document.rs index a05679086e9..b8819fc4c86 100644 --- a/components/script/dom/document.rs +++ b/components/script/dom/document.rs @@ -1175,15 +1175,14 @@ pub enum DocumentSource { NotFromParser, } +#[allow(unsafe_code)] pub trait LayoutDocumentHelpers { - #[allow(unsafe_code)] unsafe fn is_html_document_for_layout(&self) -> bool; } +#[allow(unsafe_code)] impl LayoutDocumentHelpers for LayoutJS { - #[allow(unrooted_must_root)] #[inline] - #[allow(unsafe_code)] unsafe fn is_html_document_for_layout(&self) -> bool { (*self.unsafe_get()).is_html_document } diff --git a/components/script/dom/window.rs b/components/script/dom/window.rs index afae2923bf5..761ede6edf8 100644 --- a/components/script/dom/window.rs +++ b/components/script/dom/window.rs @@ -885,14 +885,6 @@ impl Window { /// /// TODO(pcwalton): Only wait for style recalc, since we have off-main-thread layout. pub fn force_reflow(&self, goal: ReflowGoal, query_type: ReflowQueryType, reason: ReflowReason) { - let document = self.Document(); - let root = document.r().GetDocumentElement(); - let root = match root.r() { - Some(root) => root, - None => return, - }; - let root = root.upcast::(); - let window_size = match self.window_size.get() { Some(window_size) => window_size, None => return, @@ -923,7 +915,7 @@ impl Window { goal: goal, page_clip_rect: self.page_clip_rect.get(), }, - document_root: root.to_trusted_node_address(), + document: self.Document().r().upcast::().to_trusted_node_address(), window_size: window_size, script_chan: self.control_chan.clone(), script_join_chan: join_chan, diff --git a/components/script/layout_interface.rs b/components/script/layout_interface.rs index 9647ddd4376..5c41a04363c 100644 --- a/components/script/layout_interface.rs +++ b/components/script/layout_interface.rs @@ -174,7 +174,7 @@ pub struct ScriptReflow { /// General reflow data. pub reflow_info: Reflow, /// The document node. - pub document_root: TrustedNodeAddress, + pub document: TrustedNodeAddress, /// The channel through which messages can be sent back to the script task. pub script_chan: Sender, /// The current window size. From 069c40f788ef69ee5523c1f88c898c849080c594 Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Fri, 9 Oct 2015 21:38:24 -0700 Subject: [PATCH 4/4] Check modified event state from layout and dirty it there. This adds some overhead, but also provides the small performance benefit of avoiding dirtying in the case where an event state is toggled an even number of times between reflows. The main benefit here though is that it sets us up to be smarter about what we mark as dirty using restyle hints. --- components/layout/layout_task.rs | 8 +++++ components/layout/wrapper.rs | 49 +++++++++++++++++++++++++- components/script/dom/bindings/cell.rs | 7 ++++ components/script/dom/document.rs | 34 +++++++++++++++++- components/script/dom/element.rs | 5 ++- components/script/dom/window.rs | 12 ++----- 6 files changed, 102 insertions(+), 13 deletions(-) diff --git a/components/layout/layout_task.rs b/components/layout/layout_task.rs index 4ea12e58796..638488452f9 100644 --- a/components/layout/layout_task.rs +++ b/components/layout/layout_task.rs @@ -1199,6 +1199,14 @@ impl LayoutTask { } } + let event_state_changes = doc.drain_event_state_changes(); + if !needs_dirtying { + for &(el, state) in event_state_changes.iter() { + assert!(!state.is_empty()); + el.note_event_state_change(); + } + } + // Create a layout context for use throughout the following passes. let mut shared_layout_context = self.build_shared_layout_context(&*rw_data, screen_size_changed, diff --git a/components/layout/wrapper.rs b/components/layout/wrapper.rs index d42fa386ec2..38c23cd0b47 100644 --- a/components/layout/wrapper.rs +++ b/components/layout/wrapper.rs @@ -45,7 +45,7 @@ use script::dom::bindings::js::LayoutJS; use script::dom::characterdata::LayoutCharacterDataHelpers; use script::dom::document::{Document, LayoutDocumentHelpers}; use script::dom::element; -use script::dom::element::{Element, LayoutElementHelpers, RawLayoutElementHelpers}; +use script::dom::element::{Element, EventState, LayoutElementHelpers, RawLayoutElementHelpers}; use script::dom::htmlcanvaselement::{LayoutHTMLCanvasElementHelpers, HTMLCanvasData}; use script::dom::htmliframeelement::HTMLIFrameElement; use script::dom::htmlimageelement::LayoutHTMLImageElementHelpers; @@ -59,6 +59,7 @@ use selectors::parser::{AttrSelector, NamespaceConstraint}; use smallvec::VecLike; use std::borrow::ToOwned; use std::cell::{Ref, RefMut}; +use std::iter::FromIterator; use std::marker::PhantomData; use std::mem; use std::sync::Arc; @@ -365,6 +366,17 @@ impl<'le> LayoutDocument<'le> { } node } + + pub fn drain_event_state_changes(&self) -> Vec<(LayoutElement, EventState)> { + unsafe { + let changes = self.document.drain_event_state_changes(); + Vec::from_iter(changes.iter().map(|&(el, state)| + (LayoutElement { + element: el, + chain: PhantomData, + }, state))) + } + } } /// A wrapper around elements that ensures layout can only ever access safe properties. @@ -387,6 +399,41 @@ impl<'le> LayoutElement<'le> { chain: PhantomData, } } + + /// Properly marks nodes as dirty in response to event state changes. + /// + /// Currently this implementation is very conservative, and basically mirrors node::dirty_impl. + /// With restyle hints, we can do less work here. + pub fn note_event_state_change(&self) { + let node = self.as_node(); + + // Bail out if we're already dirty. This won't be valid when we start doing more targeted + // dirtying with restyle hints. + if node.is_dirty() { return } + + // Dirty descendants. + fn dirty_subtree(node: LayoutNode) { + // Stop if this subtree is already dirty. This won't be valid with restyle hints, see above. + if node.is_dirty() { return } + + unsafe { + node.set_dirty(true); + node.set_dirty_descendants(true); + } + + for kid in node.children() { + dirty_subtree(kid); + } + } + dirty_subtree(node); + + let mut curr = node; + while let Some(parent) = curr.parent_node() { + if parent.has_dirty_descendants() { break } + unsafe { parent.set_dirty_descendants(true); } + curr = parent; + } + } } fn as_element<'le>(node: LayoutJS) -> Option> { diff --git a/components/script/dom/bindings/cell.rs b/components/script/dom/bindings/cell.rs index 9ec04e358e5..dabc88284b6 100644 --- a/components/script/dom/bindings/cell.rs +++ b/components/script/dom/bindings/cell.rs @@ -94,6 +94,13 @@ impl DOMRefCell { _ => None, } } + + /// Version of the above that we use during restyle while the script task + /// is blocked. + pub fn borrow_mut_for_layout(&self) -> RefMut { + debug_assert!(task_state::get().is_layout()); + self.value.borrow_mut() + } } impl JSTraceable for DOMRefCell { diff --git a/components/script/dom/document.rs b/components/script/dom/document.rs index b8819fc4c86..f9028e3616e 100644 --- a/components/script/dom/document.rs +++ b/components/script/dom/document.rs @@ -32,7 +32,7 @@ use dom::customevent::CustomEvent; use dom::documentfragment::DocumentFragment; use dom::documenttype::DocumentType; use dom::domimplementation::DOMImplementation; -use dom::element::{Element, ElementCreator}; +use dom::element::{Element, ElementCreator, EventState}; use dom::event::{Event, EventBubbles, EventCancelable}; use dom::eventtarget::{EventTarget}; use dom::htmlanchorelement::HTMLAnchorElement; @@ -174,6 +174,8 @@ pub struct Document { /// This field is set to the document itself for inert documents. /// https://html.spec.whatwg.org/multipage/#appropriate-template-contents-owner-document appropriate_template_contents_owner_document: MutNullableHeap>, + // The collection of EventStates that have been changed since the last restyle. + event_state_changes: DOMRefCell, EventState>>, } impl PartialEq for Document { @@ -301,6 +303,11 @@ impl Document { } } + pub fn needs_reflow(&self) -> bool { + self.GetDocumentElement().is_some() && + (self.upcast::().get_has_dirty_descendants() || !self.event_state_changes.borrow().is_empty()) + } + /// Returns the first `base` element in the DOM that has an `href` attribute. pub fn base_element(&self) -> Option> { self.base_element.get_rooted() @@ -1178,6 +1185,7 @@ pub enum DocumentSource { #[allow(unsafe_code)] pub trait LayoutDocumentHelpers { unsafe fn is_html_document_for_layout(&self) -> bool; + unsafe fn drain_event_state_changes(&self) -> Vec<(LayoutJS, EventState)>; } #[allow(unsafe_code)] @@ -1186,6 +1194,15 @@ impl LayoutDocumentHelpers for LayoutJS { unsafe fn is_html_document_for_layout(&self) -> bool { (*self.unsafe_get()).is_html_document } + + #[inline] + #[allow(unrooted_must_root)] + unsafe fn drain_event_state_changes(&self) -> Vec<(LayoutJS, EventState)> { + let mut changes = (*self.unsafe_get()).event_state_changes.borrow_mut_for_layout(); + let drain = changes.drain(); + let layout_drain = drain.map(|(k, v)| (k.to_layout(), v)); + Vec::from_iter(layout_drain) + } } impl Document { @@ -1251,6 +1268,7 @@ impl Document { reflow_timeout: Cell::new(None), base_element: Default::default(), appropriate_template_contents_owner_document: Default::default(), + event_state_changes: DOMRefCell::new(HashMap::new()), } } @@ -1315,6 +1333,20 @@ impl Document { pub fn get_element_by_id(&self, id: &Atom) -> Option> { self.idmap.borrow().get(&id).map(|ref elements| (*elements)[0].root()) } + + pub fn record_event_state_change(&self, el: &Element, which: EventState) { + let mut map = self.event_state_changes.borrow_mut(); + let empty; + { + let states = map.entry(JS::from_ref(el)) + .or_insert(EventState::empty()); + states.toggle(which); + empty = states.is_empty(); + } + if empty { + map.remove(&JS::from_ref(el)); + } + } } diff --git a/components/script/dom/element.rs b/components/script/dom/element.rs index 1be58932305..0bdca0714a7 100644 --- a/components/script/dom/element.rs +++ b/components/script/dom/element.rs @@ -1836,6 +1836,9 @@ impl Element { fn set_state(&self, which: EventState, value: bool) { let mut state = self.event_state.get(); + if state.contains(which) == value { + return + } match value { true => state.insert(which), false => state.remove(which), @@ -1843,7 +1846,7 @@ impl Element { self.event_state.set(state); let node = self.upcast::(); - node.dirty(NodeDamage::NodeStyleDamaged); + node.owner_doc().record_event_state_change(self, which); } pub fn get_active_state(&self) -> bool { diff --git a/components/script/dom/window.rs b/components/script/dom/window.rs index 761ede6edf8..abe693696fd 100644 --- a/components/script/dom/window.rs +++ b/components/script/dom/window.rs @@ -957,16 +957,8 @@ impl Window { /// /// TODO(pcwalton): Only wait for style recalc, since we have off-main-thread layout. pub fn reflow(&self, goal: ReflowGoal, query_type: ReflowQueryType, reason: ReflowReason) { - let document = self.Document(); - let root = document.r().GetDocumentElement(); - let root = match root.r() { - Some(root) => root, - None => return, - }; - - let root = root.upcast::(); - if query_type == ReflowQueryType::NoQuery && !root.get_has_dirty_descendants() { - debug!("root has no dirty descendants; avoiding reflow (reason {:?})", reason); + if query_type == ReflowQueryType::NoQuery && !self.Document().needs_reflow() { + debug!("Document doesn't need reflow - skipping it (reason {:?})", reason); return }