Remove NodeListsNodeData when it's no longer needed

https://bugs.webkit.org/show_bug.cgi?id=107074 Reviewed by Darin Adler. PerformanceTests: Added a micro benchmark to see the benefit of removing NodeListsNodeData. The test traverses all elements in the html5 specification page and accesses childNodes. Don't enable this test for now since it's really a micro benchmark specifically designed to test this patch. * DOM/TraverseChildNodes.html: Added. * Skipped: Don't enable newly added test by default. * resources/results-template.html: Compare against the unscaled unit (e.g. "bytes") as opposed to scaled units such as "K bytes". * resources/runner.js: (.start): Moved the code to call currentTest.setup from measureRunsPerSecondOnce so that it'll be ran for all test types, namely of PerfTestRunner.measureTime. (.measureRunsPerSecondOnce): Source/WebCore: Remove NodeListsNodeData when the last node list is removed from it. If we detect that we have only one node list left in the data structure, we'll simply destroy the entire "this" object to free up the memory space. This reduced the memory usage of the micro benchmark by roughly 3%. Performance Tests: DOM/TraverseChildNodes.html * dom/Node.cpp: (WebCore::Node::clearNodeLists): Added. * dom/Node.h: * dom/NodeRareData.h: (WebCore::NodeListsNodeData::removeChildNodeList): (WebCore::NodeListsNodeData::removeCacheWithAtomicName): (WebCore::NodeListsNodeData::removeCacheWithName): (WebCore::NodeListsNodeData::removeCacheWithQualifiedName): (WebCore::NodeListsNodeData::deleteThisAndUpdateNodeRareDataIfAboutToRemoveLastList): Added. Removes "this" NodeListsNodeData if there is only one node list left. Tools: Generalize the warning a little so that it's also ignored on PerformanceTests/DOM/TraverseChildNodes.html * Scripts/webkitpy/performance_tests/perftest.py: (PerfTest): git-svn-id: svn://svn.chromium.org/blink/trunk@140070 bbb929c8-8fbe-4397-9dbb-9b2b20218538
marcosholgado · Jan 18, 2013 · 32776d2 · 32776d2
1 parent 49285bc
commit 32776d2
Show file tree

Hide file tree

Showing 11 changed files with 131 additions and 10 deletions.
diff --git a/third_party/WebKit/PerformanceTests/ChangeLog b/third_party/WebKit/PerformanceTests/ChangeLog
@@ -1,3 +1,25 @@
+2013-01-16  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Remove NodeListsNodeData when it's no longer needed
+        https://bugs.webkit.org/show_bug.cgi?id=107074
+
+        Reviewed by Darin Adler.
+
+        Added a micro benchmark to see the benefit of removing NodeListsNodeData.
+        The test traverses all elements in the html5 specification page and accesses childNodes.
+
+        Don't enable this test for now since it's really a micro benchmark specifically
+        designed to test this patch.
+
+        * DOM/TraverseChildNodes.html: Added.
+        * Skipped: Don't enable newly added test by default.
+        * resources/results-template.html: Compare against the unscaled unit (e.g. "bytes") as
+        opposed to scaled units such as "K bytes".
+        * resources/runner.js:
+        (.start): Moved the code to call currentTest.setup from measureRunsPerSecondOnce so that
+        it'll be ran for all test types, namely of PerfTestRunner.measureTime.
+        (.measureRunsPerSecondOnce):
+
 2013-01-17  Eric Seidel  <eric@webkit.org>
 
         Add a version of the html-parser benchmark which uses srcdoc instead of document.write so it tests the threaded parser

diff --git a/third_party/WebKit/PerformanceTests/DOM/TraverseChildNodes.html b/third_party/WebKit/PerformanceTests/DOM/TraverseChildNodes.html
@@ -0,0 +1,32 @@
+<!DOCTYPE html>
+<html>
+<body>
+<script src="../resources/runner.js"></script>
+<script>
+var spec = PerfTestRunner.loadFile("../Parser/resources/html5.html");
+var iframe;
+
+PerfTestRunner.measureTime({
+    setup: function () {
+        if (iframe)
+            document.body.removeChild(iframe);
+        iframe = document.createElement("iframe");
+        iframe.style.display = "none";  // Prevent creation of the rendering tree, so we only test HTML parsing.
+        iframe.sandbox = '';  // Prevent external script loads which could cause write() to return before completing the parse.
+        document.body.appendChild(iframe);
+        iframe.contentDocument.open();
+        iframe.contentDocument.write(spec);
+        iframe.contentDocument.close();
+    },
+    run: function() {
+        var elements = iframe.contentDocument.getElementsByTagName('*');
+        for (var i = 0; i < elements.length; i++) {
+            for (var j = 0; j < elements[i].childNodes.length; j++)
+                elements[i].childNodes[j];
+        }
+    },
+    done: function () { document.body.removeChild(iframe); }});
+
+</script>
+</body>
+</html>
diff --git a/third_party/WebKit/PerformanceTests/Skipped b/third_party/WebKit/PerformanceTests/Skipped
@@ -1,3 +1,6 @@
+# Micro benchmarks not worth running at the moment.
+DOM/child-node-traversal.html
+
 # Not enabled by default on some ports
 Mutation
 

diff --git a/third_party/WebKit/PerformanceTests/resources/results-template.html b/third_party/WebKit/PerformanceTests/resources/results-template.html
@@ -255,7 +255,7 @@
         computeScalingFactorIfNeeded();
         return cachedUnit;
     }
-    this.smallerIsBetter = function () { return this.unit() == 'ms' || this.unit() == 'bytes'; }
+    this.smallerIsBetter = function () { return testResults[0].unit() == 'ms' || testResults[0].unit() == 'bytes'; }
 }
 
 var plotColor = 'rgb(230,50,50)';

diff --git a/third_party/WebKit/PerformanceTests/resources/runner.js b/third_party/WebKit/PerformanceTests/resources/runner.js
@@ -166,6 +166,9 @@ if (window.testRunner) {
         PerfTestRunner.gc();
         window.setTimeout(function () {
             try {
+                if (currentTest.setup)
+                    currentTest.setup();
+
                 var measuredValue = runner();
             } catch (exception) {
                 logFatalError("Got an exception while running test.run with name=" + exception.name + ", message=" + exception.message);
@@ -275,9 +278,6 @@ if (window.testRunner) {
         var totalTime = 0;
         var numberOfRuns = 0;
 
-        if (currentTest.setup)
-            currentTest.setup();
-
         while (totalTime < timeToRun) {
             totalTime += callRunAndMeasureTime(callsPerIteration);
             numberOfRuns += callsPerIteration;

diff --git a/third_party/WebKit/Source/WebCore/ChangeLog b/third_party/WebKit/Source/WebCore/ChangeLog
@@ -1,3 +1,30 @@
+2013-01-16  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Remove NodeListsNodeData when it's no longer needed
+        https://bugs.webkit.org/show_bug.cgi?id=107074
+
+        Reviewed by Darin Adler.
+
+        Remove NodeListsNodeData when the last node list is removed from it.
+
+        If we detect that we have only one node list left in the data structure,
+        we'll simply destroy the entire "this" object to free up the memory space.
+
+        This reduced the memory usage of the micro benchmark by roughly 3%.
+
+        Performance Tests: DOM/TraverseChildNodes.html
+
+        * dom/Node.cpp:
+        (WebCore::Node::clearNodeLists): Added.
+        * dom/Node.h:
+        * dom/NodeRareData.h:
+        (WebCore::NodeListsNodeData::removeChildNodeList):
+        (WebCore::NodeListsNodeData::removeCacheWithAtomicName):
+        (WebCore::NodeListsNodeData::removeCacheWithName):
+        (WebCore::NodeListsNodeData::removeCacheWithQualifiedName):
+        (WebCore::NodeListsNodeData::deleteThisAndUpdateNodeRareDataIfAboutToRemoveLastList): Added.
+        Removes "this" NodeListsNodeData if there is only one node list left.
+
 2013-01-17  Abhishek Arya  <inferno@chromium.org>
 
         Heap-use-after-free in WebCore::RenderBlock::checkFloatsInCleanLine

diff --git a/third_party/WebKit/Source/WebCore/dom/Node.cpp b/third_party/WebKit/Source/WebCore/dom/Node.cpp
@@ -981,6 +981,11 @@ NodeListsNodeData* Node::nodeLists()
     return hasRareData() ? rareData()->nodeLists() : 0;
 }
 
+void Node::clearNodeLists()
+{
+    rareData()->clearNodeLists();
+}
+
 void Node::checkSetPrefix(const AtomicString& prefix, ExceptionCode& ec)
 {
     // Perform error checking as required by spec for setting Node.prefix. Used by

diff --git a/third_party/WebKit/Source/WebCore/dom/Node.h b/third_party/WebKit/Source/WebCore/dom/Node.h
@@ -582,6 +582,7 @@ class Node : public EventTarget, public ScriptWrappable, public TreeShared<Node>
 
     void invalidateNodeListCachesInAncestors(const QualifiedName* attrName = 0, Element* attributeOwnerElement = 0);
     NodeListsNodeData* nodeLists();
+    void clearNodeLists();
 
     PassRefPtr<NodeList> getElementsByTagName(const AtomicString&);
     PassRefPtr<NodeList> getElementsByTagNameNS(const AtomicString& namespaceURI, const AtomicString& localName);

diff --git a/third_party/WebKit/Source/WebCore/dom/NodeRareData.h b/third_party/WebKit/Source/WebCore/dom/NodeRareData.h
@@ -69,7 +69,9 @@ class NodeListsNodeData {
 
     void removeChildNodeList(ChildNodeList* list)
     {
-        ASSERT_UNUSED(list, m_childNodeList = list);
+        ASSERT(m_childNodeList = list);
+        if (deleteThisAndUpdateNodeRareDataIfAboutToRemoveLastList(list->ownerNode()))
+            return;
         m_childNodeList = 0;
     }
 
@@ -144,20 +146,26 @@ class NodeListsNodeData {
 
     void removeCacheWithAtomicName(LiveNodeListBase* list, CollectionType collectionType, const AtomicString& name = starAtom)
     {
-        ASSERT_UNUSED(list, list == m_atomicNameCaches.get(namedNodeListKey(collectionType, name)));
+        ASSERT(list == m_atomicNameCaches.get(namedNodeListKey(collectionType, name)));
+        if (deleteThisAndUpdateNodeRareDataIfAboutToRemoveLastList(list->ownerNode()))
+            return;
         m_atomicNameCaches.remove(namedNodeListKey(collectionType, name));
     }
 
     void removeCacheWithName(LiveNodeListBase* list, CollectionType collectionType, const String& name)
     {
-        ASSERT_UNUSED(list, list == m_nameCaches.get(namedNodeListKey(collectionType, name)));
+        ASSERT(list == m_nameCaches.get(namedNodeListKey(collectionType, name)));
+        if (deleteThisAndUpdateNodeRareDataIfAboutToRemoveLastList(list->ownerNode()))
+            return;
         m_nameCaches.remove(namedNodeListKey(collectionType, name));
     }
 
     void removeCacheWithQualifiedName(LiveNodeList* list, const AtomicString& namespaceURI, const AtomicString& localName)
     {
         QualifiedName name(nullAtom, localName, namespaceURI);
-        ASSERT_UNUSED(list, list == m_tagNodeListCacheNS.get(name));
+        ASSERT(list == m_tagNodeListCacheNS.get(name));
+        if (deleteThisAndUpdateNodeRareDataIfAboutToRemoveLastList(list->ownerNode()))
+            return;
         m_tagNodeListCacheNS.remove(name);
     }
 
@@ -218,6 +226,8 @@ class NodeListsNodeData {
         return std::pair<unsigned char, String>(type, name);
     }
 
+    bool deleteThisAndUpdateNodeRareDataIfAboutToRemoveLastList(Node*);
+
     // FIXME: m_childNodeList should be merged into m_atomicNameCaches or at least be shared with HTMLCollection returned by Element::children
     // but it's tricky because invalidateCaches shouldn't invalidate this cache and adoptTreeScope shouldn't call registerNodeList or unregisterNodeList.
     ChildNodeList* m_childNodeList;
@@ -322,6 +332,16 @@ class NodeRareData : public NodeRareDataBase {
 #endif
 };
 
+inline bool NodeListsNodeData::deleteThisAndUpdateNodeRareDataIfAboutToRemoveLastList(Node* ownerNode)
+{
+    ASSERT(ownerNode);
+    ASSERT(ownerNode->nodeLists() == this);
+    if ((m_childNodeList ? 1 : 0) + m_atomicNameCaches.size() + m_nameCaches.size() + m_tagNodeListCacheNS.size() != 1)
+        return false;
+    ownerNode->clearNodeLists();
+    return true;
+}
+
 } // namespace WebCore
 
 #endif // NodeRareData_h
diff --git a/third_party/WebKit/Tools/ChangeLog b/third_party/WebKit/Tools/ChangeLog
@@ -1,3 +1,15 @@
+2013-01-16  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Remove NodeListsNodeData when it's no longer needed
+        https://bugs.webkit.org/show_bug.cgi?id=107074
+
+        Reviewed by Darin Adler.
+
+        Generalize the warning a little so that it's also ignored on PerformanceTests/DOM/TraverseChildNodes.html
+
+        * Scripts/webkitpy/performance_tests/perftest.py:
+        (PerfTest):
+
 2013-01-17  Simon Fraser  <simon.fraser@apple.com>
 
         Ref test images are upside-down in WebKit2

diff --git a/third_party/WebKit/Tools/Scripts/webkitpy/performance_tests/perftest.py b/third_party/WebKit/Tools/Scripts/webkitpy/performance_tests/perftest.py
@@ -205,8 +205,7 @@ def _should_ignore_line_in_stderr(self, line):
         re.compile(re.escape("""frame "<!--framePath //<!--frame0-->/<!--frame0-->-->" - has 1 onunload handler(s)""")),
         # Following is for html5.html
         re.compile(re.escape("""Blocked access to external URL http://www.whatwg.org/specs/web-apps/current-work/""")),
-        # Following is for Parser/html-parser.html
-        re.compile(re.escape("""CONSOLE MESSAGE: Blocked script execution in 'html-parser.html' because the document's frame is sandboxed and the 'allow-scripts' permission is not set.""")),
+        re.compile(r"CONSOLE MESSAGE: Blocked script execution in '[A-Za-z0-9\-\.]+' because the document's frame is sandboxed and the 'allow-scripts' permission is not set."),
         # Dromaeo reports values for subtests. Ignore them for now.
         re.compile(r'(?P<name>.+): \[(?P<values>(\d+(.\d+)?,\s+)*\d+(.\d+)?)\]'),
     ]