Remove NodeListsNodeData when it's no longer needed
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 18 Jan 2013 00:35:57 +0000 (00:35 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 18 Jan 2013 00:35:57 +0000 (00:35 +0000)
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: https://svn.webkit.org/repository/webkit/trunk@140070 268f45cc-cd09-0410-ab3c-d52691b4dbfc

PerformanceTests/ChangeLog
PerformanceTests/DOM/TraverseChildNodes.html [new file with mode: 0644]
PerformanceTests/Skipped
PerformanceTests/resources/results-template.html
PerformanceTests/resources/runner.js
Source/WebCore/ChangeLog
Source/WebCore/dom/Node.cpp
Source/WebCore/dom/Node.h
Source/WebCore/dom/NodeRareData.h
Tools/ChangeLog
Tools/Scripts/webkitpy/performance_tests/perftest.py

index 06a1c4b..08d3a4b 100644 (file)
@@ -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/PerformanceTests/DOM/TraverseChildNodes.html b/PerformanceTests/DOM/TraverseChildNodes.html
new file mode 100644 (file)
index 0000000..d32e88e
--- /dev/null
@@ -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>
index 1fcf10f..bf6ead0 100644 (file)
@@ -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
 
index 9fdb7b9..c6534eb 100644 (file)
@@ -255,7 +255,7 @@ function PerfTest(name) {
         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)';
index 36ae561..a8b397e 100755 (executable)
@@ -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;
index 0836ed4..67c1137 100644 (file)
@@ -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
index 540bc17..dc6dcde 100644 (file)
@@ -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
index 0e01cee..ce505a2 100644 (file)
@@ -582,6 +582,7 @@ public:
 
     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);
index 9228b76..c25ec0b 100644 (file)
@@ -69,7 +69,9 @@ public:
 
     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 @@ public:
 
     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 @@ private:
         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 @@ private:
 #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
index 6c7eb7c..584a0a5 100644 (file)
@@ -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
index 057a736..8294547 100644 (file)
@@ -205,8 +205,7 @@ class PerfTest(object):
         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+)?)\]'),
     ]