From 5e9127e2b268d1775f5a1938dba431ec9578ee8f Mon Sep 17 00:00:00 2001 From: Patrick Walton Date: Tue, 28 Oct 2014 11:12:09 -0700 Subject: [PATCH 1/7] script: Use a 16-element `SmallVec` for the root collection. In my tests the size of the root collection never exceeded 7, so 16 seems like a nice conservative number. --- components/script/dom/bindings/js.rs | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/components/script/dom/bindings/js.rs b/components/script/dom/bindings/js.rs index 14f717f7455..2cd327397ad 100644 --- a/components/script/dom/bindings/js.rs +++ b/components/script/dom/bindings/js.rs @@ -53,7 +53,8 @@ use js::jsapi::JSObject; use layout_interface::TrustedNodeAddress; use script_task::StackRoots; -use std::cell::{Cell, RefCell}; +use servo_util::smallvec::{SmallVec, SmallVec16}; +use std::cell::{Cell, UnsafeCell}; use std::default::Default; use std::kinds::marker::ContravariantLifetime; use std::mem; @@ -419,14 +420,14 @@ impl, U: Reflectable> TemporaryPushable for Vec> { /// An opaque, LIFO rooting mechanism. pub struct RootCollection { - roots: RefCell>, + roots: UnsafeCell>, } impl RootCollection { /// Create an empty collection of roots pub fn new() -> RootCollection { RootCollection { - roots: RefCell::new(vec!()), + roots: UnsafeCell::new(SmallVec16::new()), } } @@ -438,17 +439,23 @@ impl RootCollection { /// Track a stack-based root to ensure LIFO root ordering fn root<'a, 'b, T: Reflectable>(&self, untracked: &Root<'a, 'b, T>) { - let mut roots = self.roots.borrow_mut(); - roots.push(untracked.js_ptr); - debug!(" rooting {:?}", untracked.js_ptr); + unsafe { + let roots = self.roots.get(); + (*roots).push(untracked.js_ptr); + debug!(" rooting {:?}", untracked.js_ptr); + } } /// Stop tracking a stack-based root, asserting if LIFO root ordering has been violated fn unroot<'a, 'b, T: Reflectable>(&self, rooted: &Root<'a, 'b, T>) { - let mut roots = self.roots.borrow_mut(); - debug!("unrooting {:?} (expecting {:?}", roots.last().unwrap(), rooted.js_ptr); - assert!(*roots.last().unwrap() == rooted.js_ptr); - roots.pop().unwrap(); + unsafe { + let roots = self.roots.get(); + debug!("unrooting {:?} (expecting {:?}", + (*roots).as_slice().last().unwrap(), + rooted.js_ptr); + assert!(*(*roots).as_slice().last().unwrap() == rooted.js_ptr); + (*roots).pop().unwrap(); + } } } From 6f577c7c84f8cedf83a6f4c71625190ba8cbd19e Mon Sep 17 00:00:00 2001 From: Patrick Walton Date: Tue, 28 Oct 2014 11:14:06 -0700 Subject: [PATCH 2/7] script: Avoid a temporary when constructing `` and `` elements. Was a huge improvement to RoboHornet. --- components/script/dom/htmltabledatacellelement.rs | 10 +++++++--- components/script/dom/htmltablerowelement.rs | 8 +++++--- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/components/script/dom/htmltabledatacellelement.rs b/components/script/dom/htmltabledatacellelement.rs index 535f2e4ffd5..a2a73bd86f8 100644 --- a/components/script/dom/htmltabledatacellelement.rs +++ b/components/script/dom/htmltabledatacellelement.rs @@ -32,9 +32,13 @@ impl HTMLTableDataCellElement { } #[allow(unrooted_must_root)] - pub fn new(localName: DOMString, prefix: Option, document: JSRef) -> Temporary { - let element = HTMLTableDataCellElement::new_inherited(localName, prefix, document); - Node::reflect_node(box element, document, HTMLTableDataCellElementBinding::Wrap) + pub fn new(localName: DOMString, prefix: Option, document: JSRef) + -> Temporary { + Node::reflect_node(box HTMLTableDataCellElement::new_inherited(localName, + prefix, + document), + document, + HTMLTableDataCellElementBinding::Wrap) } } diff --git a/components/script/dom/htmltablerowelement.rs b/components/script/dom/htmltablerowelement.rs index 64bd1e66c11..3edf4a0e64f 100644 --- a/components/script/dom/htmltablerowelement.rs +++ b/components/script/dom/htmltablerowelement.rs @@ -32,9 +32,11 @@ impl HTMLTableRowElement { } #[allow(unrooted_must_root)] - pub fn new(localName: DOMString, prefix: Option, document: JSRef) -> Temporary { - let element = HTMLTableRowElement::new_inherited(localName, prefix, document); - Node::reflect_node(box element, document, HTMLTableRowElementBinding::Wrap) + pub fn new(localName: DOMString, prefix: Option, document: JSRef) + -> Temporary { + Node::reflect_node(box HTMLTableRowElement::new_inherited(localName, prefix, document), + document, + HTMLTableRowElementBinding::Wrap) } } From e034c1cee22e4cf11dbf3e4fccf6815513080bfe Mon Sep 17 00:00:00 2001 From: Patrick Walton Date: Tue, 28 Oct 2014 11:16:20 -0700 Subject: [PATCH 3/7] script: Use `String::new()` instead of the formatting infrastructure when constructing DOM unique IDs. This was showing up in the RoboHornet profile. --- components/script/dom/node.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/script/dom/node.rs b/components/script/dom/node.rs index d13feb5193e..52591258387 100644 --- a/components/script/dom/node.rs +++ b/components/script/dom/node.rs @@ -1156,7 +1156,7 @@ impl Node { layout_data: LayoutDataRef::new(), - unique_id: DOMRefCell::new("".to_string()), + unique_id: DOMRefCell::new(String::new()), } } From 6a7a96a86cfa676834ac227d8da9a2a7f9870f2e Mon Sep 17 00:00:00 2001 From: Patrick Walton Date: Tue, 28 Oct 2014 11:17:26 -0700 Subject: [PATCH 4/7] script: Don't dirty nodes that are already dirty. --- components/script/dom/node.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/components/script/dom/node.rs b/components/script/dom/node.rs index 52591258387..af27aa9d4c3 100644 --- a/components/script/dom/node.rs +++ b/components/script/dom/node.rs @@ -622,6 +622,10 @@ impl<'a> NodeHelpers<'a> for JSRef<'a, Node> { // 1. Dirty self. self.set_has_changed(true); + if self.get_is_dirty() { + return + } + // 2. Dirty descendants. fn dirty_subtree(node: JSRef) { // Stop if this subtree is already dirty. From 8ab354ac085d8d70d7ccf992b5262e1969e3949a Mon Sep 17 00:00:00 2001 From: Patrick Walton Date: Tue, 28 Oct 2014 11:18:43 -0700 Subject: [PATCH 5/7] script: Don't create a temporary vector on the heap when inserting non-fragment nodes. --- components/script/dom/node.rs | 68 ++++++++++++++++++++--------------- 1 file changed, 39 insertions(+), 29 deletions(-) diff --git a/components/script/dom/node.rs b/components/script/dom/node.rs index af27aa9d4c3..ba423715def 100644 --- a/components/script/dom/node.rs +++ b/components/script/dom/node.rs @@ -1352,29 +1352,8 @@ impl Node { parent: JSRef, child: Option>, suppress_observers: SuppressObserver) { - // XXX assert owner_doc - // Step 1-3: ranges. - // Step 4. - let mut nodes = match node.type_id() { - DocumentFragmentNodeTypeId => node.children().collect(), - _ => vec!(node.clone()), - }; - - // Step 5: DocumentFragment, mutation records. - // Step 6: DocumentFragment. - match node.type_id() { - DocumentFragmentNodeTypeId => { - for c in node.children() { - Node::remove(c, node, Suppressed); - } - }, - _ => (), - } - - // Step 7: mutation records. - // Step 8. - for node in nodes.iter_mut() { - parent.add_child(*node, child); + fn do_insert(node: JSRef, parent: JSRef, child: Option>) { + parent.add_child(node, child); let is_in_doc = parent.is_in_doc(); for kid in node.traverse_preorder() { let mut flags = kid.flags.get(); @@ -1387,14 +1366,45 @@ impl Node { } } - // Step 9. - match suppress_observers { - Unsuppressed => { - for node in nodes.iter() { - node.node_inserted(); + fn fire_observer_if_necessary(node: JSRef, suppress_observers: SuppressObserver) { + match suppress_observers { + Unsuppressed => node.node_inserted(), + Suppressed => () + } + } + + // XXX assert owner_doc + // Step 1-3: ranges. + + match node.type_id() { + DocumentFragmentNodeTypeId => { + // Step 4. + // Step 5: DocumentFragment, mutation records. + // Step 6: DocumentFragment. + for c in node.children() { + Node::remove(c, node, Suppressed); + } + + // Step 7: mutation records. + // Step 8. + for node in node.children() { + do_insert(node, parent, child); + } + + for node in node.children() { + fire_observer_if_necessary(node, suppress_observers); } } - Suppressed => () + _ => { + // Step 4. + // Step 5: DocumentFragment, mutation records. + // Step 6: DocumentFragment. + // Step 7: mutation records. + // Step 8. + do_insert(node, parent, child); + // Step 9. + fire_observer_if_necessary(node, suppress_observers); + } } } From a94e13f8886211b2ead8d52b3cfe43cd0c8ff998 Mon Sep 17 00:00:00 2001 From: Patrick Walton Date: Tue, 28 Oct 2014 11:24:41 -0700 Subject: [PATCH 6/7] script: Use an FNV hash to hash event listeners. The security properties of SipHash are irrelevant for event listeners and the creation of the random number generator was showing up high in the profiles. --- components/script/dom/bindings/trace.rs | 6 ++++-- components/script/dom/eventtarget.rs | 5 +++-- components/script/lib.rs | 2 +- 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/components/script/dom/bindings/trace.rs b/components/script/dom/bindings/trace.rs index 930fd59edbe..c4f59491c57 100644 --- a/components/script/dom/bindings/trace.rs +++ b/components/script/dom/bindings/trace.rs @@ -39,7 +39,7 @@ use msg::constellation_msg::{PipelineId, SubpageId, WindowSizeData}; use net::image_cache_task::ImageCacheTask; use script_traits::ScriptControlChan; use std::collections::hashmap::HashMap; -use collections::hash::Hash; +use collections::hash::{Hash, Hasher}; use style::PropertyDeclarationBlock; use std::comm::{Receiver, Sender}; use string_cache::{Atom, Namespace}; @@ -170,7 +170,9 @@ impl JSTraceable for Option { } } -impl JSTraceable for HashMap { +impl JSTraceable for HashMap where K: Eq + Hash + JSTraceable, + V: JSTraceable, + H: Hasher { #[inline] fn trace(&self, trc: *mut JSTracer) { for e in self.iter() { diff --git a/components/script/dom/eventtarget.rs b/components/script/dom/eventtarget.rs index 233047691ea..0ced9c9c41e 100644 --- a/components/script/dom/eventtarget.rs +++ b/components/script/dom/eventtarget.rs @@ -18,6 +18,7 @@ use dom::xmlhttprequest::XMLHttpRequestId; use dom::virtualmethods::VirtualMethods; use js::jsapi::{JS_CompileUCFunction, JS_GetFunctionObject, JS_CloneFunctionObject}; use js::jsapi::{JSContext, JSObject}; +use servo_util::fnv::FnvHasher; use servo_util::str::DOMString; use libc::{c_char, size_t}; use std::ptr; @@ -69,7 +70,7 @@ pub struct EventListenerEntry { pub struct EventTarget { type_id: EventTargetTypeId, reflector_: Reflector, - handlers: DOMRefCell>>, + handlers: DOMRefCell, FnvHasher>>, } impl EventTarget { @@ -77,7 +78,7 @@ impl EventTarget { EventTarget { type_id: type_id, reflector_: Reflector::new(), - handlers: DOMRefCell::new(HashMap::new()), + handlers: DOMRefCell::new(HashMap::with_hasher(FnvHasher)), } } diff --git a/components/script/lib.rs b/components/script/lib.rs index e03801f5217..02a96942927 100644 --- a/components/script/lib.rs +++ b/components/script/lib.rs @@ -5,7 +5,7 @@ #![comment = "The Servo Parallel Browser Project"] #![license = "MPL"] -#![feature(globs, macro_rules, struct_variant, phase, unsafe_destructor)] +#![feature(default_type_params, globs, macro_rules, struct_variant, phase, unsafe_destructor)] #![deny(unused_imports, unused_variable)] #![allow(non_snake_case)] From b245a2475f89f01c0e52780213903904b4e8513f Mon Sep 17 00:00:00 2001 From: Patrick Walton Date: Fri, 31 Oct 2014 10:24:37 -0700 Subject: [PATCH 7/7] script: Fix busted document fragment appending code --- components/script/dom/node.rs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/components/script/dom/node.rs b/components/script/dom/node.rs index ba423715def..d3dad1049c4 100644 --- a/components/script/dom/node.rs +++ b/components/script/dom/node.rs @@ -1381,18 +1381,20 @@ impl Node { // Step 4. // Step 5: DocumentFragment, mutation records. // Step 6: DocumentFragment. - for c in node.children() { - Node::remove(c, node, Suppressed); + let mut kids = Vec::new(); + for kid in node.children() { + kids.push(kid.clone()); + Node::remove(kid, node, Suppressed); } // Step 7: mutation records. // Step 8. - for node in node.children() { - do_insert(node, parent, child); + for kid in kids.iter() { + do_insert((*kid).clone(), parent, child); } - for node in node.children() { - fire_observer_if_necessary(node, suppress_observers); + for kid in kids.into_iter() { + fire_observer_if_necessary(kid, suppress_observers); } } _ => {