From 069c40f788ef69ee5523c1f88c898c849080c594 Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Fri, 9 Oct 2015 21:38:24 -0700 Subject: [PATCH] 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 }