- Revision
- 136076
- Author
- morr...@google.com
- Date
- 2012-11-28 16:51:45 -0800 (Wed, 28 Nov 2012)
Log Message
checkAcceptChild() needs fewer virtual calls
https://bugs.webkit.org/show_bug.cgi?id=103372
Reviewed by Ojan Vafai.
This change reorganizes checkAcceptChild family for speed.
- Made Node::checkAddChild and Node::checkReplaceChild() static inline functions
in ContainerNode.cpp. checkAcceptChild() was also moved to the same file. This allows us
more aggressive inlining.
- Added a fast path in checkAcceptChild(), where we can assume that the parent is element
and the new child is element or text. Under this assumption, we need no extra type checks
and just needs a cycle prevention through Node::contains(). This is faster than current generic version.
- Moved extra checks from checkAddChild() and checkReplaceChild() to
checkAcceptChild(). This allows the fast path skips even these extra checks.
- Node::canReplaceChild() is devirtualized. Since the only override is on
Document, we can check isDocumentNode() to call it directly.
- The default implemenation of Node::canReplaceChild() just calls isChildTypeAllowed().
That is what an extra check for checkAddChild() did. So we can share these code path in checkAcceptChild().
This gains 2-3% win on Dromaeo dom-modify.html.
No new tests, covered by existing tests.
* dom/ContainerNode.cpp:
(WebCore::isChildTypeAllowed): Moved from Node.cpp
(WebCore::checkAcceptChild): Moved from Node.cpp, Added a fast path.
(WebCore::checkAddChild): Moved from Node.cpp
(WebCore::checkReplaceChild): Moved from Node.cpp
(WebCore::ContainerNode::insertBefore): Followed the signature change.
(WebCore::ContainerNode::replaceChild): Followed the signature change, moved null check from checkReplaceChild to here.
(WebCore::ContainerNode::appendChild): Followed the signature change.
* dom/Document.h:
(Document):
* dom/Node.cpp:
* dom/Node.h:
(WebCore::Node::isDocumentTypeNode): Added for better readability of call sites.
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (136075 => 136076)
--- trunk/Source/WebCore/ChangeLog 2012-11-29 00:20:17 UTC (rev 136075)
+++ trunk/Source/WebCore/ChangeLog 2012-11-29 00:51:45 UTC (rev 136076)
@@ -1,3 +1,47 @@
+2012-11-28 Hajime Morrita <morr...@google.com>
+
+ checkAcceptChild() needs fewer virtual calls
+ https://bugs.webkit.org/show_bug.cgi?id=103372
+
+ Reviewed by Ojan Vafai.
+
+ This change reorganizes checkAcceptChild family for speed.
+
+ - Made Node::checkAddChild and Node::checkReplaceChild() static inline functions
+ in ContainerNode.cpp. checkAcceptChild() was also moved to the same file. This allows us
+ more aggressive inlining.
+
+ - Added a fast path in checkAcceptChild(), where we can assume that the parent is element
+ and the new child is element or text. Under this assumption, we need no extra type checks
+ and just needs a cycle prevention through Node::contains(). This is faster than current generic version.
+
+ - Moved extra checks from checkAddChild() and checkReplaceChild() to
+ checkAcceptChild(). This allows the fast path skips even these extra checks.
+
+ - Node::canReplaceChild() is devirtualized. Since the only override is on
+ Document, we can check isDocumentNode() to call it directly.
+
+ - The default implemenation of Node::canReplaceChild() just calls isChildTypeAllowed().
+ That is what an extra check for checkAddChild() did. So we can share these code path in checkAcceptChild().
+
+ This gains 2-3% win on Dromaeo dom-modify.html.
+
+ No new tests, covered by existing tests.
+
+ * dom/ContainerNode.cpp:
+ (WebCore::isChildTypeAllowed): Moved from Node.cpp
+ (WebCore::checkAcceptChild): Moved from Node.cpp, Added a fast path.
+ (WebCore::checkAddChild): Moved from Node.cpp
+ (WebCore::checkReplaceChild): Moved from Node.cpp
+ (WebCore::ContainerNode::insertBefore): Followed the signature change.
+ (WebCore::ContainerNode::replaceChild): Followed the signature change, moved null check from checkReplaceChild to here.
+ (WebCore::ContainerNode::appendChild): Followed the signature change.
+ * dom/Document.h:
+ (Document):
+ * dom/Node.cpp:
+ * dom/Node.h:
+ (WebCore::Node::isDocumentTypeNode): Added for better readability of call sites.
+
2012-11-28 Kenichi Ishibashi <ba...@chromium.org>
StyleResolver should not set NaN to font size
Modified: trunk/Source/WebCore/dom/ContainerNode.cpp (136075 => 136076)
--- trunk/Source/WebCore/dom/ContainerNode.cpp 2012-11-29 00:20:17 UTC (rev 136075)
+++ trunk/Source/WebCore/dom/ContainerNode.cpp 2012-11-29 00:51:45 UTC (rev 136076)
@@ -133,6 +133,70 @@
removeAllChildren();
}
+static inline bool isChildTypeAllowed(ContainerNode* newParent, Node* child)
+{
+ if (!child->isDocumentFragment())
+ return newParent->childTypeAllowed(child->nodeType());
+
+ for (Node* node = child->firstChild(); node; node = node->nextSibling()) {
+ if (!newParent->childTypeAllowed(node->nodeType()))
+ return false;
+ }
+ return true;
+}
+
+static inline ExceptionCode checkAcceptChild(ContainerNode* newParent, Node* newChild, Node* oldChild)
+{
+ // Not mentioned in spec: throw NOT_FOUND_ERR if newChild is null
+ if (!newChild)
+ return NOT_FOUND_ERR;
+
+ // Goes common casae fast path if possible.
+ if ((newChild->isElementNode() || newChild->isTextNode()) && newParent->isElementNode()) {
+ ASSERT(!newParent->isReadOnlyNode());
+ ASSERT(!newParent->isDocumentTypeNode());
+ ASSERT(isChildTypeAllowed(newParent, newChild));
+ if (newChild->contains(newParent))
+ return HIERARCHY_REQUEST_ERR;
+ return 0;
+ }
+
+ if (newParent->isReadOnlyNode())
+ return NO_MODIFICATION_ALLOWED_ERR;
+ if (newChild->inDocument() && newChild->isDocumentTypeNode())
+ return HIERARCHY_REQUEST_ERR;
+ if (newChild->contains(newParent))
+ return HIERARCHY_REQUEST_ERR;
+
+ if (oldChild && newParent->isDocumentNode()) {
+ if (!static_cast<Document*>(newParent)->canReplaceChild(newChild, oldChild))
+ return HIERARCHY_REQUEST_ERR;
+ } else if (!isChildTypeAllowed(newParent, newChild))
+ return HIERARCHY_REQUEST_ERR;
+
+ return 0;
+}
+
+static inline bool checkAddChild(ContainerNode* newParent, Node* newChild, ExceptionCode& ec)
+{
+ if (ExceptionCode code = checkAcceptChild(newParent, newChild, 0)) {
+ ec = code;
+ return false;
+ }
+
+ return true;
+}
+
+static inline bool checkReplaceChild(ContainerNode* newParent, Node* newChild, Node* oldChild, ExceptionCode& ec)
+{
+ if (ExceptionCode code = checkAcceptChild(newParent, newChild, oldChild)) {
+ ec = code;
+ return false;
+ }
+
+ return true;
+}
+
bool ContainerNode::insertBefore(PassRefPtr<Node> newChild, Node* refChild, ExceptionCode& ec, bool shouldLazyAttach)
{
// Check that this node is not "floating".
@@ -148,8 +212,7 @@
return appendChild(newChild, ec, shouldLazyAttach);
// Make sure adding the new child is OK.
- checkAddChild(newChild.get(), ec);
- if (ec)
+ if (!checkAddChild(this, newChild.get(), ec))
return false;
// NOT_FOUND_ERR: Raised if refChild is not a child of this node
@@ -264,13 +327,17 @@
if (oldChild == newChild) // nothing to do
return true;
+ if (!oldChild) {
+ ec = NOT_FOUND_ERR;
+ return false;
+ }
+
// Make sure replacing the old child with the new is ok
- checkReplaceChild(newChild.get(), oldChild, ec);
- if (ec)
+ if (!checkReplaceChild(this, newChild.get(), oldChild, ec))
return false;
// NOT_FOUND_ERR: Raised if oldChild is not a child of this node.
- if (!oldChild || oldChild->parentNode() != this) {
+ if (oldChild->parentNode() != this) {
ec = NOT_FOUND_ERR;
return false;
}
@@ -291,8 +358,7 @@
return true;
// Does this one more time because removeChild() fires a MutationEvent.
- checkReplaceChild(newChild.get(), oldChild, ec);
- if (ec)
+ if (!checkReplaceChild(this, newChild.get(), oldChild, ec))
return false;
NodeVector targets;
@@ -301,8 +367,7 @@
return false;
// Does this yet another check because collectChildrenAndRemoveFromOldParent() fires a MutationEvent.
- checkReplaceChild(newChild.get(), oldChild, ec);
- if (ec)
+ if (!checkReplaceChild(this, newChild.get(), oldChild, ec))
return false;
InspectorInstrumentation::willInsertDOMNode(document(), this);
@@ -560,8 +625,7 @@
ec = 0;
// Make sure adding the new child is ok
- checkAddChild(newChild.get(), ec);
- if (ec)
+ if (!checkAddChild(this, newChild.get(), ec))
return false;
if (newChild == m_lastChild) // nothing to do
Modified: trunk/Source/WebCore/dom/Document.h (136075 => 136076)
--- trunk/Source/WebCore/dom/Document.h 2012-11-29 00:20:17 UTC (rev 136075)
+++ trunk/Source/WebCore/dom/Document.h 2012-11-29 00:51:45 UTC (rev 136076)
@@ -732,6 +732,7 @@
void nodeChildrenWillBeRemoved(ContainerNode*);
// nodeWillBeRemoved is only safe when removing one node at a time.
void nodeWillBeRemoved(Node*);
+ bool canReplaceChild(Node* newChild, Node* oldChild);
void textInserted(Node*, unsigned offset, unsigned length);
void textRemoved(Node*, unsigned offset, unsigned length);
@@ -1196,7 +1197,6 @@
virtual NodeType nodeType() const;
virtual bool childTypeAllowed(NodeType) const;
virtual PassRefPtr<Node> cloneNode(bool deep);
- virtual bool canReplaceChild(Node* newChild, Node* oldChild);
virtual void refScriptExecutionContext() { ref(); }
virtual void derefScriptExecutionContext() { deref(); }
Modified: trunk/Source/WebCore/dom/Node.cpp (136075 => 136076)
--- trunk/Source/WebCore/dom/Node.cpp 2012-11-29 00:20:17 UTC (rev 136075)
+++ trunk/Source/WebCore/dom/Node.cpp 2012-11-29 00:51:45 UTC (rev 136076)
@@ -1152,82 +1152,6 @@
// Attribute-specific checks are in Attr::setPrefix().
}
-static bool isChildTypeAllowed(Node* newParent, Node* child)
-{
- if (child->nodeType() != Node::DOCUMENT_FRAGMENT_NODE) {
- if (!newParent->childTypeAllowed(child->nodeType()))
- return false;
- return true;
- }
-
- for (Node *n = child->firstChild(); n; n = n->nextSibling()) {
- if (!newParent->childTypeAllowed(n->nodeType()))
- return false;
- }
- return true;
-}
-
-bool Node::canReplaceChild(Node* newChild, Node*)
-{
- return isChildTypeAllowed(this, newChild);
-}
-
-static void checkAcceptChild(Node* newParent, Node* newChild, ExceptionCode& ec)
-{
- // Not mentioned in spec: throw NOT_FOUND_ERR if newChild is null
- if (!newChild) {
- ec = NOT_FOUND_ERR;
- return;
- }
-
- if (newParent->isReadOnlyNode()) {
- ec = NO_MODIFICATION_ALLOWED_ERR;
- return;
- }
-
- if (newChild->inDocument() && newChild->nodeType() == Node::DOCUMENT_TYPE_NODE) {
- ec = HIERARCHY_REQUEST_ERR;
- return;
- }
-
- // HIERARCHY_REQUEST_ERR: Raised if this node is of a type that does not allow children of the type of the
- // newChild node, or if the node to append is one of this node's ancestors.
-
- if (newChild->contains(newParent)) {
- ec = HIERARCHY_REQUEST_ERR;
- return;
- }
-}
-
-void Node::checkReplaceChild(Node* newChild, Node* oldChild, ExceptionCode& ec)
-{
- if (!oldChild) {
- ec = NOT_FOUND_ERR;
- return;
- }
-
- checkAcceptChild(this, newChild, ec);
- if (ec)
- return;
-
- if (!canReplaceChild(newChild, oldChild)) {
- ec = HIERARCHY_REQUEST_ERR;
- return;
- }
-}
-
-void Node::checkAddChild(Node *newChild, ExceptionCode& ec)
-{
- checkAcceptChild(this, newChild, ec);
- if (ec)
- return;
-
- if (!isChildTypeAllowed(this, newChild)) {
- ec = HIERARCHY_REQUEST_ERR;
- return;
- }
-}
-
bool Node::isDescendantOf(const Node *other) const
{
// Return true if other is an ancestor of this, otherwise false
Modified: trunk/Source/WebCore/dom/Node.h (136075 => 136076)
--- trunk/Source/WebCore/dom/Node.h 2012-11-29 00:20:17 UTC (rev 136075)
+++ trunk/Source/WebCore/dom/Node.h 2012-11-29 00:51:45 UTC (rev 136076)
@@ -456,6 +456,7 @@
}
bool isReadOnlyNode() const { return nodeType() == ENTITY_REFERENCE_NODE; }
+ bool isDocumentTypeNode() const { return nodeType() == DOCUMENT_TYPE_NODE; }
virtual bool childTypeAllowed(NodeType) const { return false; }
unsigned childNodeCount() const;
Node* childNode(unsigned index) const;
@@ -489,13 +490,6 @@
bool contains(const Node*) const;
bool containsIncludingShadowDOM(Node*);
- // This method is used to do strict error-checking when adding children via
- // the public DOM API (e.g., appendChild()).
- void checkAddChild(Node* newChild, ExceptionCode&); // Error-checking when adding via the DOM API
-
- void checkReplaceChild(Node* newChild, Node* oldChild, ExceptionCode&);
- virtual bool canReplaceChild(Node* newChild, Node* oldChild);
-
// Used to determine whether range offsets use characters or node indices.
virtual bool offsetInCharacters() const;
// Number of DOM 16-bit units contained in node. Note that rendered text length can be different - e.g. because of
