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 06a1c4b4f79195d53d4d89ea2186a8857148a869..08d3a4b1f16b971a508b3b0aaaceb4b1f6acb64d 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 1fcf10f1a8a026be9e045a457dac4899f6926aa8..bf6ead075d7bd991f0724feda83c862a7105e22a 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 9fdb7b9f6b17d7daa2a82527cbcf294a1b484022..c6534eb17072be4591ac3c7484124a1bf35a8130 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 36ae5613ffad1d0f13bba76459256b9bb4b150f3..a8b397ec80c1c0c0ac6d1b6bbdbe2363d3278d30 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 0836ed46b63f345ee4ddff3bd9f84bb7e1830a24..67c113730752d2749fb4b32f60a39ee3404c8399 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 540bc17afb7507da967b0f7443812686b199ccd0..dc6dcde11922ac78dcc7e445f1ff452b3fe86cbe 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 0e01cee8558c5e82ae7ec8ed1556936481fe4199..ce505a29890dc2c318fc66a4fe3c667fefbae941 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 9228b76cb71ac0f61be3a030d00933ac528485d6..c25ec0bd43c7f453c8757d150d09612065041107 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 6c7eb7c820bd5d8101da6756af23d4fa93634363..584a0a5369b372be5634394dfd3cb9c56d38dcf6 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 057a736db2ba5ce997268b1a40b673846512ac67..8294547211414a0fa6d8678a85e5d10ebe15787d 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+)?)\]'),
     ]